2019-08-06 11:20:05

by Hayes Wang

[permalink] [raw]
Subject: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
through the sysfs.

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 119 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 115 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 1615900c8592..0b4d439f603a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,7 +26,7 @@
#include <linux/acpi.h>

/* Information for net-next */
-#define NETNEXT_VERSION "09"
+#define NETNEXT_VERSION "10"

/* Information for net */
#define NET_VERSION "10"
@@ -756,6 +756,9 @@ struct r8152 {
u32 tx_qlen;
u32 coalesce;
u32 rx_buf_sz;
+ u32 rx_frag_head_sz;
+ u32 rx_max_agg_num;
+
u16 ocp_base;
u16 speed;
u8 *intr_buff;
@@ -1992,7 +1995,7 @@ static struct rx_agg *rtl_get_free_rx(struct r8152 *tp, gfp_t mflags)

spin_unlock_irqrestore(&tp->rx_lock, flags);

- if (!agg_free && atomic_read(&tp->rx_count) < RTL8152_MAX_RX_AGG)
+ if (!agg_free && atomic_read(&tp->rx_count) < tp->rx_max_agg_num)
agg_free = alloc_rx_agg(tp, mflags);

return agg_free;
@@ -2072,10 +2075,10 @@ static int rx_bottom(struct r8152 *tp, int budget)
pkt_len -= ETH_FCS_LEN;
rx_data += sizeof(struct rx_desc);

- if (!agg_next || RTL8152_RXFG_HEADSZ > pkt_len)
+ if (!agg_next || tp->rx_frag_head_sz > pkt_len)
rx_frag_head_sz = pkt_len;
else
- rx_frag_head_sz = RTL8152_RXFG_HEADSZ;
+ rx_frag_head_sz = tp->rx_frag_head_sz;

skb = napi_alloc_skb(napi, rx_frag_head_sz);
if (!skb) {
@@ -5383,6 +5386,101 @@ static u8 rtl_get_version(struct usb_interface *intf)
return version;
}

+static ssize_t rx_frag_head_sz_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct r8152 *tp = usb_get_intfdata(intf);
+
+ sprintf(buf, "%u\n", tp->rx_frag_head_sz);
+
+ return strlen(buf);
+}
+
+static ssize_t rx_frag_head_sz_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct usb_interface *intf;
+ u32 rx_frag_head_sz;
+ struct r8152 *tp;
+
+ intf = to_usb_interface(dev);
+ tp = usb_get_intfdata(intf);
+
+ if (sscanf(buf, "%u\n", &rx_frag_head_sz) != 1)
+ return -EINVAL;
+
+ if (rx_frag_head_sz < ETH_ZLEN)
+ return -EINVAL;
+
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ return -ENODEV;
+
+ if (tp->rx_frag_head_sz != rx_frag_head_sz) {
+ napi_disable(&tp->napi);
+ tp->rx_frag_head_sz = rx_frag_head_sz;
+ napi_enable(&tp->napi);
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(rx_frag_head_sz);
+
+static ssize_t rx_max_agg_num_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct r8152 *tp = usb_get_intfdata(intf);
+
+ sprintf(buf, "%u\n", tp->rx_max_agg_num);
+
+ return strlen(buf);
+}
+
+static ssize_t rx_max_agg_num_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct usb_interface *intf;
+ u32 rx_max_agg_num;
+ struct r8152 *tp;
+
+ intf = to_usb_interface(dev);
+ tp = usb_get_intfdata(intf);
+
+ if (sscanf(buf, "%u\n", &rx_max_agg_num) != 1)
+ return -EINVAL;
+
+ if (rx_max_agg_num < (RTL8152_MAX_RX * 2))
+ return -EINVAL;
+
+ if (test_bit(RTL8152_UNPLUG, &tp->flags))
+ return -ENODEV;
+
+ if (tp->rx_max_agg_num != rx_max_agg_num) {
+ napi_disable(&tp->napi);
+ tp->rx_max_agg_num = rx_max_agg_num;
+ napi_enable(&tp->napi);
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(rx_max_agg_num);
+
+static struct attribute *rtk_adv_attrs[] = {
+ &dev_attr_rx_frag_head_sz.attr,
+ &dev_attr_rx_max_agg_num.attr,
+ NULL
+};
+
+static struct attribute_group rtk_adv_grp = {
+ .name = "rtl_adv",
+ .attrs = rtk_adv_attrs,
+};
+
static int rtl8152_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -5487,6 +5585,9 @@ static int rtl8152_probe(struct usb_interface *intf,
tp->speed = tp->mii.supports_gmii ? SPEED_1000 : SPEED_100;
tp->duplex = DUPLEX_FULL;

+ tp->rx_frag_head_sz = RTL8152_RXFG_HEADSZ;
+ tp->rx_max_agg_num = RTL8152_MAX_RX_AGG;
+
intf->needs_remote_wakeup = 1;

tp->rtl_ops.init(tp);
@@ -5513,8 +5614,16 @@ static int rtl8152_probe(struct usb_interface *intf,

netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);

+ ret = sysfs_create_group(&intf->dev.kobj, &rtk_adv_grp);
+ if (ret < 0) {
+ netif_err(tp, probe, netdev, "creat rtk_adv_grp fail\n");
+ goto out2;
+ }
+
return 0;

+out2:
+ unregister_netdev(netdev);
out1:
netif_napi_del(&tp->napi);
usb_set_intfdata(intf, NULL);
@@ -5527,6 +5636,8 @@ static void rtl8152_disconnect(struct usb_interface *intf)
{
struct r8152 *tp = usb_get_intfdata(intf);

+ sysfs_remove_group(&intf->dev.kobj, &rtk_adv_grp);
+
usb_set_intfdata(intf, NULL);
if (tp) {
rtl_set_unplug(tp);
--
2.21.0


2019-08-06 22:11:28

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

On Tue, 6 Aug 2019 19:18:04 +0800, Hayes Wang wrote:
> Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
> through the sysfs.
>
> Signed-off-by: Hayes Wang <[email protected]>

Please don't expose those via sysfs. Ethtool's copybreak and descriptor
count should be applicable here, I think.

2019-08-07 07:18:02

by Hayes Wang

[permalink] [raw]
Subject: RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

Jakub Kicinski [mailto:[email protected]]
> Sent: Wednesday, August 07, 2019 6:10 AM
[...]
> Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> count should be applicable here, I think.

Excuse me.
I find struct ethtool_tunable for ETHTOOL_RX_COPYBREAK.
How about the descriptor count?


Best Regards,
Hayes


2019-08-07 13:52:56

by Maciej Fijalkowski

[permalink] [raw]
Subject: Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

On Wed, 7 Aug 2019 07:12:32 +0000
Hayes Wang <[email protected]> wrote:

> Jakub Kicinski [mailto:[email protected]]
> > Sent: Wednesday, August 07, 2019 6:10 AM
> [...]
> > Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> > count should be applicable here, I think.
>
> Excuse me.
> I find struct ethtool_tunable for ETHTOOL_RX_COPYBREAK.
> How about the descriptor count?

Look how drivers implement ethtool's set_ringparam ops.

Thanks,
Maciej

>
>
> Best Regards,
> Hayes
>
>

2019-08-08 01:41:38

by Hayes Wang

[permalink] [raw]
Subject: RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

Maciej Fijalkowski [mailto:[email protected]]
> Sent: Wednesday, August 07, 2019 8:44 PM
[...]
> > Excuse me.
> > I find struct ethtool_tunable for ETHTOOL_RX_COPYBREAK.
> > How about the descriptor count?
>
> Look how drivers implement ethtool's set_ringparam ops.

I would check it. Thanks.


Best Regards,
Hayes


2019-08-08 09:47:17

by Hayes Wang

[permalink] [raw]
Subject: RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

Jakub Kicinski [mailto:[email protected]]
> Sent: Wednesday, August 07, 2019 6:10 AM
[...]
> On Tue, 6 Aug 2019 19:18:04 +0800, Hayes Wang wrote:
> > Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
> > through the sysfs.
> >
> > Signed-off-by: Hayes Wang <[email protected]>
>
> Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> count should be applicable here, I think.

Excuse me again.
I find the kernel supports the copybreak of Ethtool.
However, I couldn't find a command of Ethtool to use it.
Do I miss something?

Best Regards,
Hayes

2019-08-08 11:51:21

by Maciej Fijalkowski

[permalink] [raw]
Subject: Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

On Thu, 8 Aug 2019 08:52:51 +0000
Hayes Wang <[email protected]> wrote:

> Jakub Kicinski [mailto:[email protected]]
> > Sent: Wednesday, August 07, 2019 6:10 AM
> [...]
> > On Tue, 6 Aug 2019 19:18:04 +0800, Hayes Wang wrote:
> > > Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
> > > through the sysfs.
> > >
> > > Signed-off-by: Hayes Wang <[email protected]>
> >
> > Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> > count should be applicable here, I think.
>
> Excuse me again.
> I find the kernel supports the copybreak of Ethtool.
> However, I couldn't find a command of Ethtool to use it.

Ummm there's set_tunable ops. Amazon's ena driver is making use of it from what
I see. Look at ena_set_tunable() in
drivers/net/ethernet/amazon/ena/ena_ethtool.c.

Maciej

> Do I miss something?
>
> Best Regards,
> Hayes
>

2019-08-08 12:24:30

by Hayes Wang

[permalink] [raw]
Subject: RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

Maciej Fijalkowski [mailto:[email protected]]
> Sent: Thursday, August 08, 2019 7:50 PM
[...]
> > Excuse me again.
> > I find the kernel supports the copybreak of Ethtool.
> > However, I couldn't find a command of Ethtool to use it.
>
> Ummm there's set_tunable ops. Amazon's ena driver is making use of it from
> what
> I see. Look at ena_set_tunable() in
> drivers/net/ethernet/amazon/ena/ena_ethtool.c.

The kernel could support it. And I has finished it.
However, when I want to test it by ethtool, I couldn't find suitable command.
I couldn't find relative feature in the source code of ethtool, either.


Best Regards,
Hayes


2019-08-08 18:45:08

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

On Thu, 8 Aug 2019 12:16:50 +0000, Hayes Wang wrote:
> Maciej Fijalkowski [mailto:[email protected]]
> > Sent: Thursday, August 08, 2019 7:50 PM
> > > Excuse me again.
> > > I find the kernel supports the copybreak of Ethtool.
> > > However, I couldn't find a command of Ethtool to use it.
> >
> > Ummm there's set_tunable ops. Amazon's ena driver is making use of it from
> > what
> > I see. Look at ena_set_tunable() in
> > drivers/net/ethernet/amazon/ena/ena_ethtool.c.
>
> The kernel could support it. And I has finished it.
> However, when I want to test it by ethtool, I couldn't find suitable command.
> I couldn't find relative feature in the source code of ethtool, either.

It's possible it's not implemented in the user space tool ????

Looks like it got posted here:

https://www.spinics.net/lists/netdev/msg299877.html

But perhaps never finished?

It should be fairly straightforward to implement by looking at how
phy-tunables are handled.

2019-08-09 03:39:57

by Hayes Wang

[permalink] [raw]
Subject: RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

Jakub Kicinski [[email protected]]
[..]> The kernel could support it. And I has finished it.
> > However, when I want to test it by ethtool, I couldn't find suitable command.
> > I couldn't find relative feature in the source code of ethtool, either.

> It's possible it's not implemented in the user space tool ????
>
> Looks like it got posted here:
>
> https://www.spinics.net/lists/netdev/msg299877.html
>
> But perhaps never finished?

May I implement both sysfs and set_tunalbe for copybreak first
before the user space tool is ready? Otherwise, the user couldn't
change the copybreak now.

Best Regards,
Hayes

2019-08-09 04:53:38

by David Miller

[permalink] [raw]
Subject: Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

From: Hayes Wang <[email protected]>
Date: Fri, 9 Aug 2019 03:38:53 +0000

> Jakub Kicinski [[email protected]]
> [..]> The kernel could support it. And I has finished it.
>> > However, when I want to test it by ethtool, I couldn't find suitable command.
>> > I couldn't find relative feature in the source code of ethtool, either.
>
>> It's possible it's not implemented in the user space tool ????
>>
>> Looks like it got posted here:
>>
>> https://www.spinics.net/lists/netdev/msg299877.html
>>
>> But perhaps never finished?
>
> May I implement both sysfs and set_tunalbe for copybreak first
> before the user space tool is ready? Otherwise, the user couldn't
> change the copybreak now.

No, fix the tool please.