Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1244359 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro huangdaode <huangdaode@hisilicon.com> - 2015-10-12 05:10 +0200
    Re: [PATCH net-next v2 1/2] hisilicon net: removes the once  HANDEL_TX_MSG macro Joe Perches <joe@perches.com> - 2015-10-12 05:30 +0200
      Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG  macro huangdaode <huangdaode@hisilicon.com> - 2015-10-12 05:40 +0200
        Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro Arnd Bergmann <arnd@arndb.de> - 2015-10-12 14:00 +0200
    Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro Arnd Bergmann <arnd@arndb.de> - 2015-10-12 14:10 +0200
      Re: [PATCH net-next v2 1/2] hisilicon net: removes the once  HANDEL_TX_MSG macro Joe Perches <joe@perches.com> - 2015-10-12 18:20 +0200
      Re: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once  HANDEL_TX_MSG macro Kenneth Lee <Kenneth-Lee-2012@foxmail.com> - 2015-10-13 15:30 +0200
        Re: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro Arnd Bergmann <arnd@arndb.de> - 2015-10-13 16:30 +0200
          Re: Fwd: Re: [PATCH net-next v2 1/2] hisilicon net: removes the once  HANDEL_TX_MSG macro Kenneth Lee <Kenneth-Lee-2012@foxmail.com> - 2015-10-15 17:40 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244621

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244633

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245780

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web