Re: [PATCH net-next 00/23] net: hns3: HNS3 bug fixes & code improvements

[lipeng (Y)]

On 2018/3/8 13:00, David Miller wrote:

Sorry, this is way too large of a patch series.

Please keep your series to about a dozen or so changes.

Anything longer puts an unreasonable burdon upon patch
reviewers, and such a large series will often make it
so that nearly all reviewers are discouraged from taking
a look at all.

Thank you.

I will split this patchset into 3 patchset:
VF driver related patchset(6 patches) + reset related patchset(9 
patches) + PF bugfix patchset(8 patches).

I think this should be better for review.

Thanks


.

Re: [PATCH net-next 3/5] net: hns3: add ethtool -p support for phy device

[lipeng (Y)]

On 2018/1/18 22:25, Andrew Lunn wrote:

+static int hclge_set_led_status_phy(struct phy_device *phydev, int value)
+{
+   int ret, cur_page;
+
+   mutex_lock(>lock);
+
+   ret = phy_read(phydev, HCLGE_PHY_PAGE_REG);
+   if (ret < 0)
+   goto out;
+   else
+   cur_page = ret;
+
+   ret = phy_write(phydev, HCLGE_PHY_PAGE_REG, HCLGE_PHY_PAGE_LED);
+   if (ret)
+   goto out;
+
+   ret = phy_write(phydev, HCLGE_LED_FC_REG, value);
+   if (ret)
+   goto out;
+
+   ret = phy_write(phydev, HCLGE_PHY_PAGE_REG, cur_page);
+
+out:
+   mutex_unlock(>lock);
+   return ret;
+}

Sorry, but NACK.

Please add an interface to phylib and the phy driver you are using to
do this.


  #define HCLGE_PHY_PAGE_MDIX   0
  #define HCLGE_PHY_PAGE_COPPER 0
+#define HCLGE_PHY_PAGE_LED 3
  
  /* Page Selection Reg. */

  #define HCLGE_PHY_PAGE_REG22
@@ -73,6 +74,15 @@
  /* Copper Specific Status Register */
  #define HCLGE_PHY_CSS_REG 17
  
+/* LED Function Control Register */

+#define HCLGE_LED_FC_REG   16
+
+/* LED Polarity Control Register */
+#define HCLGE_LED_PC_REG   17
+
+#define HCLGE_LED_FORCE_ON 9
+#define HCLGE_LED_FORCE_OFF8
+

By the looks of these defines, you assume you have a Marvell PHY.
Please make this generic so anybody with a Marvell PHY can use it.

Andrew

Hi  Andrw,

As your suggestion, we need add  interface to  phylib and the phy driver.
We will consider your suggestion and push this patch after we fix your 
comments.


so we will remove this patch  in V2 patch-set.

Thanks
Peng Li


.

Re: [PATCH net-next 00/11] add some new features and fix some bugs

[lipeng (Y)]

On 2018/1/12 1:07, David Miller wrote:

From: Peng Li 
Date: Thu, 11 Jan 2018 19:45:55 +0800


This patchset adds some new features and fixes some bugs:
[patch 1/11] adds ethtool_ops.get_channels support for VF.
[patch 2/11] removes TSO config command from VF driver.
[patch 3/11] adds ethtool_ops.get_coalesce support to PF.
[patch 4/11] adds ethtool_ops.set_coalesce support to PF.
[patch 5/11 - 11/11] do some code improvements and fix some bugs.

Can you please write a real commit message in your header postings
please?

Don't just copy the subject lines from the patches, and add one
sentence with a brief description.

Really write real paragraphs describing what the patch series
is doing, how it is doing it, and why it is doing it that
way.

A real explanation that tells the reader what exactly to
expect when they review the patches themselves.

Thanks for your advice.
A detail explanation is better for review, I will write
the "real explanation" in V2 patch-set.

Peng Li

Thank you.

.

Re: [PATCH net-next 12/20] net: hns3: Add packet statistics of netdev

[lipeng (Y)]

On 2018/1/9 11:06, David Miller wrote:

From: "lipeng (Y)" <lipeng...@huawei.com>
Date: Tue, 9 Jan 2018 10:48:04 +0800


So I think it is OK if you can revert [patch 12/20 ]("net: hns3: Add
packet statistics of netdev").

I think it is OK if you send the revert patch, which is what I
am asking for :-)

.

sure,  i will send the revert patch.
I have tested it in my local branch.

Thanks
Peng Li

Re: [PATCH net-next 12/20] net: hns3: Add packet statistics of netdev

[lipeng (Y)]

On 2018/1/9 9:54, David Miller wrote:

From: Jakub Kicinski 
Date: Mon, 8 Jan 2018 17:50:21 -0800


Oh, I only noticed this extra misleading comment now.  Unless each queue
has a netdev, I don't see how these are per-queue.

If it isn't per-queue I want this change reverted.


[patch 12/20 ] add statistics of netdev for ethtool -S, netdev may have 
multi queue.


As discussion here,  it is duplicate to add this patch.


I revert  [patch 12/20 ] , and then test on my board, HNS3 basic function and 
ethtool -S work well.

So I think it is OK if you can revert  [patch 12/20 ]("net: hns3: Add packet 
statistics of netdev").


Thanks
Peng Li


.

Re: [PATCH net-next 06/20] net: hns3: Modify the update period of packet statistics

[lipeng (Y)]

On 2018/1/5 22:54, Andrew Lunn wrote:

--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -1126,6 +1126,7 @@ static int hns3_nic_set_features(struct net_device 
*netdev,
  {
struct hns3_nic_priv *priv = netdev_priv(netdev);
int queue_num = priv->ae_handle->kinfo.num_tqps;
+   struct hnae3_handle *handle = priv->ae_handle;
struct hns3_enet_ring *ring;
unsigned int start;
unsigned int idx;
@@ -1134,6 +1135,8 @@ static int hns3_nic_set_features(struct net_device 
*netdev,
u64 tx_pkts = 0;
u64 rx_pkts = 0;
  
+	handle->ae_algo->ops->update_stats(handle, >stats);

+
for (idx = 0; idx < queue_num; idx++) {
/* fetch the tx stats */
ring = priv->ring_data[idx].ring;

There is something odd going on with patch here. Notice how it says
hns3_nic_set_features(). This is not the function being patched, it is
actually the next one, hns3_nic_get_stats64(), which makes a lot more
sense.

Is it because the static void is on the previous line?

Yes, it is because the static void is on the previous line.

I can add one patch to fix the  previous line ,  and this patch will 
correct  automatically.


do it need V2 patchset? or push a new patch after this patchset?



It would be nice if the function was correctly reported. It makes it
easier to review the patch.

Andrew

.

Re: [PATCH V3 net-next 00/17] add some features and fix some bugs for HNS3 driver

[lipeng (Y)]

On 2017/12/21 12:04, David Miller wrote:

From: "lipeng (Y)" <lipeng...@huawei.com>
Date: Thu, 21 Dec 2017 09:30:01 +0800



On 2017/12/21 3:28, David Miller wrote:

From: Lipeng <lipeng...@huawei.com>
Date: Wed, 20 Dec 2017 16:43:02 +0800


This patchset adds some new feature support and fixes some bugs:
[Patch 1/17 - 5/17] add the support to modify/query the tqp number
through ethtool -L/l command, and also fix some related bugs for
change tqp number.
[Patch 6/17 - 9-17] add support vlan tag offload on tx& direction
for pf, and fix some related bugs.
[patch 10/17 - 11/17] fix bugs for auto negotiation.
[patch 12/17] adds support for ethtool command set_pauseparam.
[patch 13/17 - 14/17] add support to update flow control settings
after
autoneg.
[patch 15/17 - 17/17] fix some other bugs in net-next.

In your From: email field, as well as you and your colleagues signoffs
you are not using your full, real, name:

From: Lipeng <lipeng...@huawei.com>

My first name is 'Peng' and Surname is 'Li'.
But it is usually spelled as 'Lipeng' in one go when referring.
This is quite usual convention with full names originating from
China. Surnames comes first, followed by the first name and they
both are inseparable while they are written as well.
"Lipeng" is my full real name,  and my sign-off's appear like above.

The same applies to "qumingguang"?  Why is it not capitalized?
It seems to just be a copy of the email address username.


have checked with him, and will fix his name spelling to  "Mingguang Qu".

Thanks
Lipeng


.

Re: [PATCH V3 net-next 00/17] add some features and fix some bugs for HNS3 driver

[lipeng (Y)]

On 2017/12/21 3:28, David Miller wrote:

From: Lipeng 
Date: Wed, 20 Dec 2017 16:43:02 +0800


This patchset adds some new feature support and fixes some bugs:
[Patch 1/17 - 5/17] add the support to modify/query the tqp number
through ethtool -L/l command, and also fix some related bugs for
change tqp number.
[Patch 6/17 - 9-17] add support vlan tag offload on tx& direction
for pf, and fix some related bugs.
[patch 10/17 - 11/17] fix bugs for auto negotiation.
[patch 12/17] adds support for ethtool command set_pauseparam.
[patch 13/17 - 14/17] add support to update flow control settings after
autoneg.
[patch 15/17 - 17/17] fix some other bugs in net-next.

In your From: email field, as well as you and your colleagues signoffs
you are not using your full, real, name:


From: Lipeng 


My first name is 'Peng' and Surname is 'Li'.
But it is usually spelled as 'Lipeng' in one go when referring.
This is quite usual convention with full names originating from
China. Surnames comes first, followed by the first name and they
both are inseparable while they are written as well.
"Lipeng" is my full real name,  and my sign-off's appear like above.


The name "lipeng"  is very common in china, many guys in HuaWei have the
same name "lipeng",
So my email is lipeng...@huawei.com,  to Distinguish from others.

other guys who named "lipeng" in huawei ,  their email have an id too,
such as lipeng...@huawei.com.

Wish the explanation is clear.

Thanks,
Lipeng




Signed-off-by: qumingguang 
Signed-off-by: Lipeng 


Please fix this.

.

Re: [PATCH V3 net-next 00/17] add some features and fix some bugs for HNS3 driver

[lipeng (Y)]

On 2017/12/21 3:28, David Miller wrote:

From: Lipeng 
Date: Wed, 20 Dec 2017 16:43:02 +0800


This patchset adds some new feature support and fixes some bugs:
[Patch 1/17 - 5/17] add the support to modify/query the tqp number
through ethtool -L/l command, and also fix some related bugs for
change tqp number.
[Patch 6/17 - 9-17] add support vlan tag offload on tx& direction
for pf, and fix some related bugs.
[patch 10/17 - 11/17] fix bugs for auto negotiation.
[patch 12/17] adds support for ethtool command set_pauseparam.
[patch 13/17 - 14/17] add support to update flow control settings after
autoneg.
[patch 15/17 - 17/17] fix some other bugs in net-next.

In your From: email field, as well as you and your colleagues signoffs
you are not using your full, real, name:

From: Lipeng 


My first name is 'Peng' and Surname is 'Li'.
But it is usually spelled as 'Lipeng' in one go when referring.
This is quite usual convention with full names originating from
China. Surnames comes first, followed by the first name and they
both are inseparable while they are written as well.
"Lipeng" is my full real name,  and my sign-off's appear like above.


Thanks,
Lipeng



Signed-off-by: qumingguang 
Signed-off-by: Lipeng 


Please fix this.

.

Re: [PATCH V2 net-next 02/17] net: hns3: add support to modify tqps number

[lipeng (Y)]

On 2017/12/20 3:18, David Miller wrote:

From: Lipeng 
Date: Tue, 19 Dec 2017 12:02:24 +0800


@@ -2651,6 +2651,19 @@ static int hns3_get_ring_config(struct hns3_nic_priv 
*priv)
return ret;
  }
  
+static void hns3_put_ring_config(struct hns3_nic_priv *priv)

+{
+   struct hnae3_handle *h = priv->ae_handle;
+   u16 i;
+
+   for (i = 0; i < h->kinfo.num_tqps; i++) {

Please use a plain "int" for index iteration loops like this since
that is the canonical type to use.

will check and fix this , Thanks.

+static void hclge_release_tqp(struct hclge_vport *vport)
+{
+   struct hnae3_knic_private_info *kinfo = >nic.kinfo;
+   struct hclge_dev *hdev = vport->back;
+   u16 i;
+
+   for (i = 0; i < kinfo->num_tqps; i++) {

Likewise.

.

Re: [PATCH V2 net-next 01/17] net: hns3: add support to query tqps number

[lipeng (Y)]

On 2017/12/20 3:16, David Miller wrote:

From: Lipeng 
Date: Tue, 19 Dec 2017 12:02:23 +0800


@@ -5002,6 +5002,26 @@ static void hclge_uninit_ae_dev(struct hnae3_ae_dev 
*ae_dev)
ae_dev->priv = NULL;
  }
  
+static u32 hclge_get_max_channels(struct hnae3_handle *handle)

+{
+   struct hclge_vport *vport = hclge_get_vport(handle);
+   struct hnae3_knic_private_info *kinfo = >kinfo;
+   struct hclge_dev *hdev = vport->back;
+

Please order local variables from longest to shortest line.

Please audit your entire submission for this problem.

.

will check this patch-set about this problem. Thanks

Re: [PATCH net-next 17/17] net: hns3: change TM sched mode to TC-based mode when SRIOV enabled

[lipeng (Y)]

On 2017/12/18 17:08, Sergei Shtylyov wrote:

On 12/18/2017 12:31 PM, Lipeng wrote:


TC-based sched mode supports SRIOV enabled and SRIOV disabled. This
patch change the TM sched mode to TC-based mode in initialization
process.

Fixes: cc9bb43 (net: hns3: Add tc-based TM support for sriov enabled 
port)


   Need at least 12 hex digits.



agree , may lost some hex digits,  will fix it.


Signed-off-by: Lipeng 

[...]

MBR, Sergei

Re: [PATCH net-next 14/17] net: hns3: add Asym Pause support to phy default features

[lipeng (Y)]

On 2017/12/18 17:07, Sergei Shtylyov wrote:

Hello!

On 12/18/2017 12:31 PM, Lipeng wrote:


From: Fuyun Liang 

commit c4fb2cdf575d (net: hns3: fix a bug for phy supported feature
initialization) adds default supported features for phy, but our 
hardware


   Ten cited commit's summary needs to be enclosed in (""), not just 
()...



Thanks , will fix it.


also supports Asym Pause. This patch adds Asym Pause support to phy
default features to prevent Asym Pause can not be advertised when the 
phy

negotiates flow control.

Fixes: c4fb2cdf575d (net: hns3: fix a bug for phy supported feature 
initialization)


   Here as well...


will fix here too.

Thanks


Signed-off-by: Fuyun Liang 
Signed-off-by: Lipeng 

[...]

MBR, Sergei

Re: [PATCH net-next 1/2] net: hns3: fix a bug when getting phy address from NCL_config file

[lipeng (Y)]

On 2017/11/8 22:30, Andrew Lunn wrote:

On Wed, Nov 08, 2017 at 03:52:22PM +0800, Lipeng wrote:

From: Fuyun Liang 

Driver gets phy address from NCL_config file and uses the phy address
to initialize phydev. There are 5 bits for phy address. And C22 phy
address has 5 bits. So 0-31 are all valid address for phy. If there
is no phy, it will crash. Because driver always get a valid phy address.

Hi Lipeng

Any plans for C45? The PHY address is still 5 bits, but do you need to
get the device type from your configuration file?

 Andrew

Hi Andrew

 C45 support is in Plan,   now only support C22.
 This bugfix patch is valid for C22 and C45.

 As plan , will get the device type(NULL, C22, C45) from configuration 
file after support C45.

 Do you think we must do it now? or can add it along with C45 support?

Lipeng

.

Re: [PATCH net-next 0/2] net: hns3: Bug fixes & Code improvements in HNS3 driver

[lipeng (Y)]

please ignore this patch-set.

I should remove  "{topost}" from the subject.

sorry for that, I will resend the patch-set.


On 2017/11/8 15:31, Lipeng wrote:

This patch-set introduces some bug fixes and code improvements.
As [patch 1/2] depends on the patch {5392902 net: hns3: Consistently using
GENMASK in hns3 driver}, which exists in net-next, not exists in net, so
push this serise to nex-next.

Fuyun Liang (2):
   {topost} net: hns3: fix a bug when getting phy address from NCL_config
 file
   {topost} net: hns3: cleanup mac auto-negotiation state query in
 hclge_update_speed_duplex

  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h  |  2 +-
  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 13 +
  2 files changed, 2 insertions(+), 13 deletions(-)

Re: [PATCH net-next 4/6] net: hns3: add support for set_link_ksettings

[lipeng (Y)]

On 2017/11/4 3:52, Florian Fainelli wrote:

On 11/02/2017 09:18 PM, Lipeng wrote:

From: Fuyun Liang 

This patch adds set_link_ksettings support for ethtool cmd.

Signed-off-by: Fuyun Liang 
Signed-off-by: Lipeng 
---
  drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c | 11 +++
  1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c 
b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
index c7b8ebd..7fe193b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_ethtool.c
@@ -653,6 +653,16 @@ static int hns3_get_link_ksettings(struct net_device 
*netdev,
return 0;
  }
  
+static int hns3_set_link_ksettings(struct net_device *netdev,

+  const struct ethtool_link_ksettings *cmd)
+{
+   /* Only support ksettings_set for netdev with phy attached for now */
+   if (netdev->phydev)
+   return phy_ethtool_ksettings_set(netdev->phydev, cmd);
+
+   return -EOPNOTSUPP;

Consider using phy_ethtool_get_link_ksettings() which already checks for
netdev->phydev.

agree, Thanks for your comment.

as this patch has been applied to  net-next, we will push another 
cleanup patch.

Re: [PATCH net v4 3/3] net: hns: fixed bug that skb used after kfree

[lipeng (Y)]

On 2017/4/28 1:38, Florian Fainelli wrote:

On 04/26/2017 07:44 PM, Yankejian wrote:

struct hns_nic_priv *priv = netdev_priv(ndev);
struct hnae_ring *ring = ring_data->ring;
@@ -361,6 +361,10 @@ int hns_nic_net_xmit_hw(struct net_device *ndev,
dev_queue = netdev_get_tx_queue(ndev, skb->queue_mapping);
netdev_tx_sent_queue(dev_queue, skb->len);
  
+	netif_trans_update(ndev);

+   ndev->stats.tx_bytes += skb->len;
+   ndev->stats.tx_packets++;

This is still wrong though, you should not update your TX statistics
until you get a TX completion interrupt that confirms these packets were
actually transmitted. This has the advantage of not causing use after
free in your ndo_start_xmit() function (current bug), and also allows
feeding information into BQL where it is appropriate, and in a central
location: the TX completion handler.


Agree, I will fix this, and will delete this patch from patchset and 
refloat it later.

Re: [PATCH net v4 1/3] net: hns: support deferred probe when can not obtain irq

[lipeng (Y)]

On 2017/4/27 19:58, Matthias Brugger wrote:



On 27/04/17 04:44, Yankejian wrote:

From: lipeng 

In the hip06 and hip07 SoCs, the interrupt lines from the
DSAF controllers are connected to mbigen hw module.
The mbigen module is probed with module_init, and, as such,
is not guaranteed to probe before the HNS driver. So we need
to support deferred probe.

Signed-off-by: lipeng 
Reviewed-by: Yisen Zhuang 
Reviewed-by: Matthias Brugger 
---
change log:
V3 -> V4:
1. Delete redundant commit message;
2. add Reviewed-by: Matthias Brugger ;

V2 -> V3:
1. Check return value when  platform_get_irq in hns_rcb_get_cfg;
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c | 4 +++-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c | 8 +++-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h | 2 +-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c

index 6ea8722..a41cf95 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c
@@ -510,7 +510,9 @@ int hns_ppe_init(struct dsaf_device *dsaf_dev)

 hns_ppe_get_cfg(dsaf_dev->ppe_common[i]);

-hns_rcb_get_cfg(dsaf_dev->rcb_common[i]);
+ret = hns_rcb_get_cfg(dsaf_dev->rcb_common[i]);
+if (ret)
+goto get_rcb_cfg_fail;


a2185587ade7 ("net: hns: Simplify the exception sequence in 
hns_ppe_init()")
deleted get_rcb_cfg_fail label. This does not compile. Please rebase 
against net-next.



Thanks, this patch should float on net-next



Best regards,
Matthias


 }

 for (i = 0; i < HNS_PPE_COM_NUM; i++)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c

index f0ed80d6..673a5d3 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c
@@ -452,7 +452,7 @@ static int hns_rcb_get_base_irq_idx(struct 
rcb_common_cb *rcb_common)

  *hns_rcb_get_cfg - get rcb config
  *@rcb_common: rcb common device
  */
-void hns_rcb_get_cfg(struct rcb_common_cb *rcb_common)
+int hns_rcb_get_cfg(struct rcb_common_cb *rcb_common)
 {
 struct ring_pair_cb *ring_pair_cb;
 u32 i;
@@ -477,10 +477,16 @@ void hns_rcb_get_cfg(struct rcb_common_cb 
*rcb_common)

 ring_pair_cb->virq[HNS_RCB_IRQ_IDX_RX] =
 is_ver1 ? platform_get_irq(pdev, base_irq_idx + i * 2 + 1) :
   platform_get_irq(pdev, base_irq_idx + i * 3);
+if ((ring_pair_cb->virq[HNS_RCB_IRQ_IDX_TX] == 
-EPROBE_DEFER) ||

+(ring_pair_cb->virq[HNS_RCB_IRQ_IDX_RX] == -EPROBE_DEFER))
+return -EPROBE_DEFER;
+
 ring_pair_cb->q.phy_base =
 RCB_COMM_BASE_TO_RING_BASE(rcb_common->phy_base, i);
 hns_rcb_ring_pair_get_cfg(ring_pair_cb);
 }
+
+return 0;
 }

 /**
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h

index 99b4e1b..3d7b484 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h
@@ -110,7 +110,7 @@ struct rcb_common_cb {
 void hns_rcb_common_free_cfg(struct dsaf_device *dsaf_dev, u32 
comm_index);

 int hns_rcb_common_init_hw(struct rcb_common_cb *rcb_common);
 void hns_rcb_start(struct hnae_queue *q, u32 val);
-void hns_rcb_get_cfg(struct rcb_common_cb *rcb_common);
+int hns_rcb_get_cfg(struct rcb_common_cb *rcb_common);
 void hns_rcb_get_queue_mode(enum dsaf_mode dsaf_mode,
 u16 *max_vfn, u16 *max_q_per_vf);



.

Re: [PATCH net v3 1/3] net: hns: support deferred probe when can not obtain irq

[lipeng (Y)]

On 2017/4/26 22:03, Matthias Brugger wrote:



On 26/04/17 09:00, Yankejian wrote:

From: lipeng 

In the hip06 and hip07 SoCs, the interrupt lines from the
DSAF controllers are connected to mbigen hw module.
The mbigen module is probed with module_init, and, as such,
is not guaranteed to probe before the HNS driver. So we need
to support deferred probe.

We check for probe deferral in the hw layer probe, so we not
probe into the main layer and memories, etc., to later learn
that we need to defer the probe.


This paragraph does not hold any more and should be deleted.

OK , will delete it.


Other then this:
Reviewed-by: Matthias Brugger 



Thanks for your review



Signed-off-by: lipeng 
Reviewed-by: Yisen Zhuang 
---
V2 -> V3:
1. Check return value when  platform_get_irq in hns_rcb_get_cfg;
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c | 4 +++-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c | 8 +++-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h | 2 +-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c

index 6ea8722..a41cf95 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c
@@ -510,7 +510,9 @@ int hns_ppe_init(struct dsaf_device *dsaf_dev)

 hns_ppe_get_cfg(dsaf_dev->ppe_common[i]);

-hns_rcb_get_cfg(dsaf_dev->rcb_common[i]);
+ret = hns_rcb_get_cfg(dsaf_dev->rcb_common[i]);
+if (ret)
+goto get_rcb_cfg_fail;
 }

 for (i = 0; i < HNS_PPE_COM_NUM; i++)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c

index f0ed80d6..673a5d3 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c
@@ -452,7 +452,7 @@ static int hns_rcb_get_base_irq_idx(struct 
rcb_common_cb *rcb_common)

  *hns_rcb_get_cfg - get rcb config
  *@rcb_common: rcb common device
  */
-void hns_rcb_get_cfg(struct rcb_common_cb *rcb_common)
+int hns_rcb_get_cfg(struct rcb_common_cb *rcb_common)
 {
 struct ring_pair_cb *ring_pair_cb;
 u32 i;
@@ -477,10 +477,16 @@ void hns_rcb_get_cfg(struct rcb_common_cb 
*rcb_common)

 ring_pair_cb->virq[HNS_RCB_IRQ_IDX_RX] =
 is_ver1 ? platform_get_irq(pdev, base_irq_idx + i * 2 + 1) :
   platform_get_irq(pdev, base_irq_idx + i * 3);
+if ((ring_pair_cb->virq[HNS_RCB_IRQ_IDX_TX] == 
-EPROBE_DEFER) ||

+(ring_pair_cb->virq[HNS_RCB_IRQ_IDX_RX] == -EPROBE_DEFER))
+return -EPROBE_DEFER;
+
 ring_pair_cb->q.phy_base =
 RCB_COMM_BASE_TO_RING_BASE(rcb_common->phy_base, i);
 hns_rcb_ring_pair_get_cfg(ring_pair_cb);
 }
+
+return 0;
 }

 /**
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h

index 99b4e1b..3d7b484 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h
@@ -110,7 +110,7 @@ struct rcb_common_cb {
 void hns_rcb_common_free_cfg(struct dsaf_device *dsaf_dev, u32 
comm_index);

 int hns_rcb_common_init_hw(struct rcb_common_cb *rcb_common);
 void hns_rcb_start(struct hnae_queue *q, u32 val);
-void hns_rcb_get_cfg(struct rcb_common_cb *rcb_common);
+int hns_rcb_get_cfg(struct rcb_common_cb *rcb_common);
 void hns_rcb_get_queue_mode(enum dsaf_mode dsaf_mode,
 u16 *max_vfn, u16 *max_q_per_vf);



.

Re: [PATCH net v2 0/3] net: hns: bug fix for HNS driver

[lipeng (Y)]

On 2017/4/25 0:24, David Miller wrote:

From: Yankejian 
Date: Fri, 21 Apr 2017 15:44:41 +0800


From: lipeng 

This series adds support defered probe when mdio or mbigen module
insmod behind HNS driver, and fixes a bug that a skb has been
freed, but it may be still used in driver.

change log:
V1 -> V2:
1. Return appropriate errno in hns_mac_register_phy;

At a minimum you're going to have to expand your commit message in
patch #1 so that it has more detail and explains the situation better.
.

OK, will add more detail and explains for #1 patch.

thanks

Re: [PATCH net v2 1/3] net: hns: support deferred probe when can not obtain irq

[lipeng (Y)]

On 2017/4/24 22:47, Matthias Brugger wrote:



On 24/04/17 13:43, lipeng (Y) wrote:



On 2017/4/24 18:28, Matthias Brugger wrote:

On 21/04/17 09:44, Yankejian wrote:

From: lipeng <lipeng...@huawei.com>

In the hip06 and hip07 SoCs, the interrupt lines from the
DSAF controllers are connected to mbigen hw module.
The mbigen module is probed with module_init, and, as such,
is not guaranteed to probe before the HNS driver. So we need
to support deferred probe.

We check for probe deferral in the hw layer probe, so we not
probe into the main layer and memories, etc., to later learn
that we need to defer the probe.



Why? This looks like a hack.
From what I see, we can handle EPROBE_DEFER easily inside hns_ppe_init
checking the return value of hns_rcb_get_cfg. Like you do in 2/3 of
this series.

Regards,
Matthias

Hi Matthias,

mdio && phy is not necessary condition, and port can work well  for port
+ SFP (without mdio &).

BUT irq is the necessary condition,  port can not work well without irq.

So, I check IRQ first,and do not probe dsaf if can't obtain irq(1/3 of
this series),   and check mdio only when there is phy(2/3 of this 
series).


And thanks for your review.


I think I didn't explained myself good enough.
I was suggesting the following (not even compile tested):

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c

index eba406bea52f..be38d47bc399 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c
@@ -510,7 +510,9 @@ int hns_ppe_init(struct dsaf_device *dsaf_dev)

hns_ppe_get_cfg(dsaf_dev->ppe_common[i]);

-   hns_rcb_get_cfg(dsaf_dev->rcb_common[i]);
+   ret = hns_rcb_get_cfg(dsaf_dev->rcb_common[i]);
+   if (reg < 0)
+   goto get_cfg_fail;
}

for (i = 0; i < HNS_PPE_COM_NUM; i++)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c

index c20a0f4f8f02..c7e801d0c3b7 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c
@@ -492,7 +492,7 @@ static int hns_rcb_get_base_irq_idx(struct 
rcb_common_cb *rcb_common)

  *hns_rcb_get_cfg - get rcb config
  *@rcb_common: rcb common device
  */
-void hns_rcb_get_cfg(struct rcb_common_cb *rcb_common)
+int hns_rcb_get_cfg(struct rcb_common_cb *rcb_common)
 {
struct ring_pair_cb *ring_pair_cb;
u32 i;
@@ -517,10 +517,18 @@ void hns_rcb_get_cfg(struct rcb_common_cb 
*rcb_common)

ring_pair_cb->virq[HNS_RCB_IRQ_IDX_RX] =
is_ver1 ? platform_get_irq(pdev, base_irq_idx + i * 2 
+ 1) :

  platform_get_irq(pdev, base_irq_idx + i * 3);
+
+   if ((ring_pair_cb->virq[HNS_RCB_IRQ_IDX_TX] == 
-EPROBE_DEFER) ||
+   (ring_pair_cb->virq[HNS_RCB_IRQ_IDX_RX] == 
-EPROBE_DEFER)) {

+   return -EPROBE_DEFER;
+   }
+
ring_pair_cb->q.phy_base =

RCB_COMM_BASE_TO_RING_BASE(rcb_common->phy_base, i);
hns_rcb_ring_pair_get_cfg(ring_pair_cb);
}
+
+   return 0;
 }

 /**


Regards,
Matthias

Thanks,  I will take your advice and test it.





lipeng




Signed-off-by: lipeng <lipeng...@huawei.com>
Reviewed-by: Yisen Zhuang <yisen.zhu...@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
index 403ea9d..2da5b42 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
@@ -2971,6 +2971,18 @@ static int hns_dsaf_probe(struct
platform_device *pdev)
 struct dsaf_device *dsaf_dev;
 int ret;

+/*
+ * Check if we should defer the probe before we probe the
+ * dsaf, as it's hard to defer later on.
+ */
+ret = platform_get_irq(pdev, 0);
+if (ret < 0) {
+if (ret != -EPROBE_DEFER)
+dev_err(>dev, "Cannot obtain irq\n");
+
+return ret;
+}
+
 dsaf_dev = hns_dsaf_alloc_dev(>dev, sizeof(struct
dsaf_drv_priv));
 if (IS_ERR(dsaf_dev)) {
 ret = PTR_ERR(dsaf_dev);



.




.

Re: [PATCH net-next 2/3] net: hns: support deferred probe when no mdio

[lipeng (Y)]

On 2017/4/20 19:13, Matthias Brugger wrote:

On 18/04/17 12:12, Yankejian wrote:

From: lipeng 

In the hip06 and hip07 SoCs, phy connect to mdio bus.The mdio
module is probed with module_init, and, as such,
is not guaranteed to probe before the HNS driver. So we need
to support deferred probe.

We check for probe deferral in the mac init, so we not init DSAF
when there is no mdio, and free all resource, to later learn that
we need to defer the probe.

Signed-off-by: lipeng 


on which kernel version is this patch based?
I checked against next-20170420 and it does not apply.



Will refloat this patchset on net.

thanks.
lipeng



---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 34 
+++

 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c

index bdd8cdd..284ebfe 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -735,6 +735,8 @@ static int hns_mac_init_ex(struct hns_mac_cb 
*mac_cb)

 rc = phy_device_register(phy);
 if (rc) {
 phy_device_free(phy);
+dev_err(>dev, "registered phy fail at address %i\n",
+addr);
 return -ENODEV;
 }

@@ -745,7 +747,7 @@ static int hns_mac_init_ex(struct hns_mac_cb 
*mac_cb)

 return 0;
 }

-static void hns_mac_register_phy(struct hns_mac_cb *mac_cb)
+static int hns_mac_register_phy(struct hns_mac_cb *mac_cb)
 {
 struct acpi_reference_args args;
 struct platform_device *pdev;
@@ -755,24 +757,39 @@ static void hns_mac_register_phy(struct 
hns_mac_cb *mac_cb)


 /* Loop over the child nodes and register a phy_device for each 
one */

 if (!to_acpi_device_node(mac_cb->fw_port))
-return;
+return 0;


Please return appropriate errno.



 rc = acpi_node_get_property_reference(
 mac_cb->fw_port, "mdio-node", 0, );
 if (rc)
-return;
+return 0;


Please return appropriate errno.



 addr = hns_mac_phy_parse_addr(mac_cb->dev, mac_cb->fw_port);
 if (addr < 0)
-return;
+return 0;


Shouldn't we just error out here by returning addr?



 /* dev address in adev */
 pdev = 
hns_dsaf_find_platform_device(acpi_fwnode_handle(args.adev));

+if (!pdev) {
+dev_err(mac_cb->dev, "mac%d mdio pdev is NULL\n",
+mac_cb->mac_id);
+return 0;


Please return appropriate errno.

Regards,
Matthias


+}
+
 mii_bus = platform_get_drvdata(pdev);
+if (!mii_bus) {
+dev_err(mac_cb->dev,
+"mac%d mdio is NULL, dsaf will probe again later\n",
+mac_cb->mac_id);
+return -EPROBE_DEFER;
+}
+
 rc = hns_mac_register_phydev(mii_bus, mac_cb, addr);
 if (!rc)
 dev_dbg(mac_cb->dev, "mac%d register phy addr:%d\n",
 mac_cb->mac_id, addr);
+
+return rc;
 }

 #define MAC_MEDIA_TYPE_MAX_LEN16
@@ -793,7 +810,7 @@ static void hns_mac_register_phy(struct 
hns_mac_cb *mac_cb)

  *@np:device node
  * return: 0 --success, negative --fail
  */
-static int  hns_mac_get_info(struct hns_mac_cb *mac_cb)
+static int hns_mac_get_info(struct hns_mac_cb *mac_cb)
 {
 struct device_node *np;
 struct regmap *syscon;
@@ -903,7 +920,10 @@ static int  hns_mac_get_info(struct hns_mac_cb 
*mac_cb)

 }
 }
 } else if (is_acpi_node(mac_cb->fw_port)) {
-hns_mac_register_phy(mac_cb);
+ret = hns_mac_register_phy(mac_cb);
+
+if (ret)
+return ret;
 } else {
 dev_err(mac_cb->dev, "mac%d cannot find phy node\n",
 mac_cb->mac_id);
@@ -1065,6 +1085,7 @@ int hns_mac_init(struct dsaf_device *dsaf_dev)
 dsaf_dev->mac_cb[port_id] = mac_cb;
 }
 }
+
 /* init mac_cb for all port */
 for (port_id = 0; port_id < max_port_num; port_id++) {
 mac_cb = dsaf_dev->mac_cb[port_id];
@@ -1074,6 +1095,7 @@ int hns_mac_init(struct dsaf_device *dsaf_dev)
 ret = hns_mac_get_cfg(dsaf_dev, mac_cb);
 if (ret)
 return ret;
+
 ret = hns_mac_init_ex(mac_cb);
 if (ret)
 return ret;



.