[PATCH net-next v2 0/2] hisilicon net: fix some bugs in HNS drivers

[PATCH net-next v2 0/2] hisilicon net: fix some bugs in HNS drivers

[huangdaode]

This patchset fixes the two bugs in HNS driver, one is change the code style 
according to the review comments from Joe Perches<joe@perches.com>, another
is fixing the wrong mac_id judgement bug which is found during internal tests.

change log:
v2
  1) remove first bug fix, which is fixed in another patch submitted by
     Arnd Bergmann <arnd@arndb.de>
  2) change the code sytyle according to Joe.
v1
 initial version.
 
huangdaode (2):
  hisilicon net: removes the once HANDEL_TX_MSG macro
  hisilicon net: fix a bug on Hisilicon Network Subsystem

 drivers/net/ethernet/hisilicon/hns/hnae.c         | 45 +++++++++++------------
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c |  2 +-
 2 files changed, 23 insertions(+), 24 deletions(-)

-- 
1.9.1

[PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[huangdaode]

This patch changes the code style to make the code more simple.
also removes the once used HNADEL_TX_MSG macro, according to the
review comments from Joe Perches.

Signed-off-by: huangdaode <huangdaode@hisilicon.com>
Reviewed-by: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hns/hnae.c | 45 +++++++++++++++----------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c b/drivers/net/ethernet/hisilicon/hns/hnae.c
index f52e99a..40f5779 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.c
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.c
@@ -439,40 +439,39 @@ EXPORT_SYMBOL(hnae_ae_unregister);
 static ssize_t handles_show(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
-	ssize_t s = 0;
+	char *s = buf;
 	struct hnae_ae_dev *hdev = cls_to_ae_dev(dev);
 	struct hnae_handle *h;
 	int i = 0, j;
 
 	list_for_each_entry_rcu(h, &hdev->handle_list, node) {
-		s += sprintf(buf + s, "handle %d (eport_id=%u from %s):\n",
+		s += sprintf(s, "handle %d (eport_id=%u from %s):\n",
 			    i++, h->eport_id, h->dev->name);
 		for (j = 0; j < h->q_num; j++) {
-			s += sprintf(buf + s, "\tqueue[%d] on %p\n",
+			s += sprintf(s, "\tqueue[%d] on %p\n",
 				     j, h->qs[i]->io_base);
-#define HANDEL_TX_MSG "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n"
-			s += sprintf(buf + s,
-				     HANDEL_TX_MSG,
-				     h->qs[i]->tx_ring.io_base,
-				     h->qs[i]->tx_ring.buf_size,
-				     h->qs[i]->tx_ring.desc_num,
-				     h->qs[i]->tx_ring.max_desc_num_per_pkt,
-				     h->qs[i]->tx_ring.max_raw_data_sz_per_desc,
-				     h->qs[i]->tx_ring.max_pkt_size,
-				 h->qs[i]->tx_ring.stats.sw_err_cnt,
-				 h->qs[i]->tx_ring.stats.io_err_cnt);
-			s += sprintf(buf + s,
-				"\t\trx_ring on %p:%u,%u,%llu,%llu,%llu\n",
-				h->qs[i]->rx_ring.io_base,
-				h->qs[i]->rx_ring.buf_size,
-				h->qs[i]->rx_ring.desc_num,
-				h->qs[i]->rx_ring.stats.sw_err_cnt,
-				h->qs[i]->rx_ring.stats.io_err_cnt,
-				h->qs[i]->rx_ring.stats.seg_pkt_cnt);
+			s += sprintf(s,
+				    "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n",
+				    h->qs[i]->tx_ring.io_base,
+				    h->qs[i]->tx_ring.buf_size,
+				    h->qs[i]->tx_ring.desc_num,
+				    h->qs[i]->tx_ring.max_desc_num_per_pkt,
+				    h->qs[i]->tx_ring.max_raw_data_sz_per_desc,
+				    h->qs[i]->tx_ring.max_pkt_size,
+				    h->qs[i]->tx_ring.stats.sw_err_cnt,
+				    h->qs[i]->tx_ring.stats.io_err_cnt);
+			s += sprintf(s,
+				    "\t\trx_ring on %p:%u,%u,%llu,%llu,%llu\n",
+				    h->qs[i]->rx_ring.io_base,
+				    h->qs[i]->rx_ring.buf_size,
+				    h->qs[i]->rx_ring.desc_num,
+				    h->qs[i]->rx_ring.stats.sw_err_cnt,
+				    h->qs[i]->rx_ring.stats.io_err_cnt,
+				    h->qs[i]->rx_ring.stats.seg_pkt_cnt);
 		}
 	}
 
-	return s;
+	return s - buf;
 }
 
 static DEVICE_ATTR_RO(handles);
-- 
1.9.1

Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Joe Perches]

Hello Huang.

On Mon, 2015-10-12 at 11:23 +0800, huangdaode wrote:
> This patch changes the code style to make the code more simple.
> also removes the once used HNADEL_TX_MSG macro, according to the

HANDEL_TX_MSG typo

> review comments from Joe Perches.
> 
> Signed-off-by: huangdaode <huangdaode@hisilicon.com>
> Reviewed-by: Joe Perches <joe@perches.com>

I didn't review this.

Please do not add signatures for another person when
not specifically added by that person.

Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[huangdaode]

On 2015/10/12 11:24, Joe Perches wrote:
> Hello Huang.
>
> On Mon, 2015-10-12 at 11:23 +0800, huangdaode wrote:
>> This patch changes the code style to make the code more simple.
>> also removes the once used HNADEL_TX_MSG macro, according to the
> HANDEL_TX_MSG typo
>
>> review comments from Joe Perches.
>>
>> Signed-off-by: huangdaode <huangdaode@hisilicon.com>
>> Reviewed-by: Joe Perches <joe@perches.com>
> I didn't review this.
> '
Hi Joe
please refer to http://lists.openwall.net/netdev/2015/10/11/61
thanks.

> Please do not add signatures for another person when
> not specifically added by that person.
>
>
>
> .
>


-- 
Best Regards
Daode Huang

Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Arnd Bergmann]

On Monday 12 October 2015 11:38:24 huangdaode wrote:
> On 2015/10/12 11:24, Joe Perches wrote:
> > Hello Huang.
> >
> > On Mon, 2015-10-12 at 11:23 +0800, huangdaode wrote:
> >> This patch changes the code style to make the code more simple.
> >> also removes the once used HNADEL_TX_MSG macro, according to the
> > HANDEL_TX_MSG typo
> >
> >> review comments from Joe Perches.
> >>
> >> Signed-off-by: huangdaode <huangdaode@hisilicon.com>
> >> Reviewed-by: Joe Perches <joe@perches.com>
> > I didn't review this.
> > '
> Hi Joe
> please refer to http://lists.openwall.net/netdev/2015/10/11/61

You should only add the "Reviewed-by" tag to a commit message
if the person you are adding replied with this explicitly.
Don't add it just because someone gave feedback.

	Arnd

Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Arnd Bergmann]

On Monday 12 October 2015 11:23:44 huangdaode wrote:
> +                       s += sprintf(s,
> +                                   "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n",
> +                                   h->qs[i]->tx_ring.io_base,
> +                                   h->qs[i]->tx_ring.buf_size,
> +                                   h->qs[i]->tx_ring.desc_num,
> +                                   h->qs[i]->tx_ring.max_desc_num_per_pkt,
> +                                   h->qs[i]->tx_ring.max_raw_data_sz_per_desc,
> +                                   h->qs[i]->tx_ring.max_pkt_size,
> +                                   h->qs[i]->tx_ring.stats.sw_err_cnt,
> +                                   h->qs[i]->tx_ring.stats.io_err_cnt);

There is actually a more significant problem with this code, which I
failed to notice when doing the original bugfix: 

You have a sysfs interface here that exports internal data of the
device that should not be visible like this. One problem is that
the io_base is a kernel pointer that must not be visible to non-root
users (so we don't easily create an attack surface for exploits).
Another problem is that the format is not documented in Documentation/ABI/
and that you have multiple values in one sysfs file here.

It would probably be better to completely remove that sysfs interface, and
to use the ethtool netlink interface to export them.

	Arnd

Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Joe Perches]

On Mon, 2015-10-12 at 13:59 +0200, Arnd Bergmann wrote:
> On Monday 12 October 2015 11:23:44 huangdaode wrote:
> > +                       s += sprintf(s,
> > +                                   "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n",
> > +                                   h->qs[i]->tx_ring.io_base,
[]
> There is actually a more significant problem with this code, which I
> failed to notice when doing the original bugfix: 
> 
> You have a sysfs interface here that exports internal data of the
> device that should not be visible like this. One problem is that
> the io_base is a kernel pointer that must not be visible to non-root
> users (so we don't easily create an attack surface for exploits).

Using %pK might have been appropriate.

> It would probably be better to completely remove that sysfs interface, and
> to use the ethtool netlink interface to export them.

But this would be better.

Re: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Kenneth Lee]

On Tue, Oct 13, 2015 at 03:06:21PM +0800, huangdaode wrote:
> Date: Tue, 13 Oct 2015 15:06:21 +0800
> From: huangdaode <huangdaode@hisilicon.com>
> To: Kenneth Lee <kenneth_lee_2012@126.com>
> Subject: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once
>  HANDEL_TX_MSG macro
> Message-ID: <561CAD6D.2060105@hisilicon.com>
> 
>    -------- Forwarded Message --------
> 
>     Subject: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once      
>              HANDEL_TX_MSG macro                                              
>        Date: Mon, 12 Oct 2015 13:59:39 +0200                                  
>        From: Arnd Bergmann <arnd@arndb.de>                                    
>          To: huangdaode <huangdaode@hisilicon.com>                            
>          CC: davem@davemloft.net, joe@perches.com, liguozhu@hisilicon.com,    
>              Yisen.Zhuang@huawei.com, netdev@vger.kernel.org,                 
>              linuxarm@huawei.com, salil.mehta@huawei.com,                     
>              kenneth-lee-2012@foxmail.com, xuwei5@hisilicon.com,              
>              lisheng011@huawei.com, linux-kernel@vger.kernel.org              
> 
>  On Monday 12 October 2015 11:23:44 huangdaode wrote:
>  > +                       s += sprintf(s,
>  > +                                   "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n",
>  > +                                   h->qs[i]->tx_ring.io_base,
>  > +                                   h->qs[i]->tx_ring.buf_size,
>  > +                                   h->qs[i]->tx_ring.desc_num,
>  > +                                   h->qs[i]->tx_ring.max_desc_num_per_pkt,
>  > +                                   h->qs[i]->tx_ring.max_raw_data_sz_per_desc,
>  > +                                   h->qs[i]->tx_ring.max_pkt_size,
>  > +                                   h->qs[i]->tx_ring.stats.sw_err_cnt,
>  > +                                   h->qs[i]->tx_ring.stats.io_err_cnt);
> 
>  There is actually a more significant problem with this code, which I
>  failed to notice when doing the original bugfix:
> 
>  You have a sysfs interface here that exports internal data of the
>  device that should not be visible like this. One problem is that
>  the io_base is a kernel pointer that must not be visible to non-root
>  users (so we don't easily create an attack surface for exploits).
>  Another problem is that the format is not documented in Documentation/ABI/
>  and that you have multiple values in one sysfs file here.
> 
>  It would probably be better to completely remove that sysfs interface, and
>  to use the ethtool netlink interface to export them.
> 
>          Arnd
> 
>  .

Hi, Arnd,

Thank you for the comment. Yes, the io_base is a security problem, we
will fix it in coming patch soon.

But can we keep the sysfs? The interface from hnae is not used only by
ethernet driver but also by Open Data Plane driver. If we more it to
upper layers. Both drivers will have the same logic. 

So how about we just add documents to Documention/ABI?

Thanks
-- 
			-Kenneth Lee (Hisilicon)

Re: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Arnd Bergmann]

On Tuesday 13 October 2015 21:27:12 Kenneth Lee wrote:
> 
> Hi, Arnd,
> 
> Thank you for the comment. Yes, the io_base is a security problem, we
> will fix it in coming patch soon.
> 
> But can we keep the sysfs? The interface from hnae is not used only by
> ethernet driver but also by Open Data Plane driver. If we more it to
> upper layers. Both drivers will have the same logic. 
> 
> So how about we just add documents to Documention/ABI?

Hi Kenneth,

In the end this is up to David Miller of course, but I'd say we are
better off not introducing any ABIs for ODP prematurely.

We are talking about very generic statistics data, and you should
already provide them for the ethernet driver using the standard
interfaces.

I have not seen any discussion about adding an ODP subsystem for
the Linux kernel, or what the API will be, but I think we should
not export any interfaces from a particular device driver directly
but always go through a common layer here and use an extensible
interface that can be implemented by everyone.

The API has not been part of a release yet, so I'd say we should
remove it for now. Once we have a net/odp/ directory, we can
add a driver-independent implementation there and call it from
the hisi driver.

	Arnd

Re: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Kenneth Lee]

On Tue, Oct 13, 2015 at 04:18:23PM +0200, Arnd Bergmann wrote:
> Date: Tue, 13 Oct 2015 16:18:23 +0200
> From: Arnd Bergmann <arnd@arndb.de>
> To: Kenneth Lee <Kenneth-Lee-2012@foxmail.com>
> Cc: davem@davemloft.net, joe@perches.com, liguozhu@hisilicon.com,
>  Yisen.Zhuang@huawei.com, netdev@vger.kernel.org, linuxarm@huawei.com,
>  salil.mehta@huawei.com, kenneth-lee-2012@foxmail.com,
>  xuwei5@hisilicon.com, lisheng011@huawei.com, linux-kernel@vger.kernel.org,
>  huangdaode <huangdaode@hisilicon.com>
> Subject: Re: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the
>  once HANDEL_TX_MSG macro
> Message-ID: <6914069.XLdT9Eli48@wuerfel>
> 
> On Tuesday 13 October 2015 21:27:12 Kenneth Lee wrote:
> > 
> > Hi, Arnd,
> > 
> > Thank you for the comment. Yes, the io_base is a security problem, we
> > will fix it in coming patch soon.
> > 
> > But can we keep the sysfs? The interface from hnae is not used only by
> > ethernet driver but also by Open Data Plane driver. If we more it to
> > upper layers. Both drivers will have the same logic. 
> > 
> > So how about we just add documents to Documention/ABI?
> 
> Hi Kenneth,
> 
> In the end this is up to David Miller of course, but I'd say we are
> better off not introducing any ABIs for ODP prematurely.
> 
> We are talking about very generic statistics data, and you should
> already provide them for the ethernet driver using the standard
> interfaces.
> 
> I have not seen any discussion about adding an ODP subsystem for
> the Linux kernel, or what the API will be, but I think we should
> not export any interfaces from a particular device driver directly
> but always go through a common layer here and use an extensible
> interface that can be implemented by everyone.
> 
> The API has not been part of a release yet, so I'd say we should
> remove it for now. Once we have a net/odp/ directory, we can
> add a driver-independent implementation there and call it from
> the hisi driver.
> 
> 	Arnd

Hi, Arnd,

Agree. We will remove the interface for the time being. Thank you.

-- 
			-Kenneth Lee (Hisilicon)

[PATCH net-next v2 2/2] hisilicon net: fix a bug on Hisilicon Network Subsystem

[huangdaode]

This patch fixes the wrong judgement of mac_id when get port num.

Signed-off-by: huangdaode <huangdaode@hisilicon.com>
Signed-off-by: yankejian <yankejian@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
index 95bf42a..e162e4a 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -179,7 +179,7 @@ static int hns_mac_get_inner_port_num(struct hns_mac_cb *mac_cb,
 			return -EINVAL;
 		}
 	} else if (mac_cb->dsaf_dev->dsaf_mode < DSAF_MODE_MAX) {
-		if (mac_cb->mac_id <= DSAF_MAX_PORT_NUM_PER_CHIP) {
+		if (mac_cb->mac_id >= DSAF_MAX_PORT_NUM_PER_CHIP) {
 			dev_err(mac_cb->dev,
 				"input invalid,%s mac%d vmid%d!\n",
 				mac_cb->dsaf_dev->ae_dev.name,
-- 
1.9.1