| Message ID | 20210218102038.2996-4-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | usbnet: speed reporting for devices without MDIO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: davem@davemloft.net oliver@neukum.org linux-usb@vger.kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote: > > The driver has a status method for receiving speed updates. > The framework, however, had support functions only for devices > that reported their speed upon an explicit query over a MDIO > interface. > CDC_NCM however gets direct notifications from the device. > As new support functions have become available, we shall now > record such notifications and tell the usbnet framework > to make direct use of them without going through the PHY layer. Since this patch is missing the hunks that landed in the previous patch and needs a v4, I'll offer my version of the commit message in case you like it better: net: cdc_ncm: record speed in status method Until very recently, the usbnet framework only had support functions for devices which reported the link speed by explicitly querying the PHY over a MDIO interface. However, the cdc_ncm devices send notifications when the link state or link speeds change and do not expose the PHY (or modem) directly. Support functions (e.g. usbnet_get_link_ksettings_internal()) to directly query state recorded by the cdc_ncm driver were added in a previous patch. So instead of cdc_ncm spewing the link speed into the dmesg buffer, record the link speed encoded in these notifications and tell the usbnet framework to use the new functions to get link speed. This is especially useful given all existing RTL8156 devices emit a connection/speed status notification every 32ms and this would fill the dmesg buffer. This implementation replaces the one recently submitted in de658a195ee23ca6aaffe197d1d2ea040beea0a2 : "net: usb: cdc_ncm: don't spew notifications" cheers, grant > v2: rebased on upstream > v3: changed variable names > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > Tested-by: Roland Dreier <roland@kernel.org> > --- > drivers/net/usb/cdc_ncm.c | 23 +---------------------- > 1 file changed, 1 insertion(+), 22 deletions(-) > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > index 0d26cbeb6e04..74c1a86b1a71 100644 > --- a/drivers/net/usb/cdc_ncm.c > +++ b/drivers/net/usb/cdc_ncm.c > @@ -1829,30 +1829,9 @@ cdc_ncm_speed_change(struct usbnet *dev, > uint32_t rx_speed = le32_to_cpu(data->DLBitRRate); > uint32_t tx_speed = le32_to_cpu(data->ULBitRate); > > - /* if the speed hasn't changed, don't report it. > - * RTL8156 shipped before 2021 sends notification about every 32ms. > - */ > - if (dev->rx_speed == rx_speed && dev->tx_speed == tx_speed) > - return; > - > + /* RTL8156 shipped before 2021 sends notification about every 32ms. */ > dev->rx_speed = rx_speed; > dev->tx_speed = tx_speed; > - > - /* > - * Currently the USB-NET API does not support reporting the actual > - * device speed. Do print it instead. > - */ > - if ((tx_speed > 1000000) && (rx_speed > 1000000)) { > - netif_info(dev, link, dev->net, > - "%u mbit/s downlink %u mbit/s uplink\n", > - (unsigned int)(rx_speed / 1000000U), > - (unsigned int)(tx_speed / 1000000U)); > - } else { > - netif_info(dev, link, dev->net, > - "%u kbit/s downlink %u kbit/s uplink\n", > - (unsigned int)(rx_speed / 1000U), > - (unsigned int)(tx_speed / 1000U)); > - } > } > > static void cdc_ncm_status(struct usbnet *dev, struct urb *urb) > -- > 2.26.2 >
Oliver, Can you include a 4th patch in the series to bring cdc_ether driver in line with the cdc_ncm behavior? I apologize for not including the patch inline - but it's late and I don't want to fight with gmail at this point. Patch is attached. Not tested. cheers, grant On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote: > > The driver has a status method for receiving speed updates. > The framework, however, had support functions only for devices > that reported their speed upon an explicit query over a MDIO > interface. > CDC_NCM however gets direct notifications from the device. > As new support functions have become available, we shall now > record such notifications and tell the usbnet framework > to make direct use of them without going through the PHY layer. > > v2: rebased on upstream > v3: changed variable names > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > Tested-by: Roland Dreier <roland@kernel.org> > --- > drivers/net/usb/cdc_ncm.c | 23 +---------------------- > 1 file changed, 1 insertion(+), 22 deletions(-) > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > index 0d26cbeb6e04..74c1a86b1a71 100644 > --- a/drivers/net/usb/cdc_ncm.c > +++ b/drivers/net/usb/cdc_ncm.c > @@ -1829,30 +1829,9 @@ cdc_ncm_speed_change(struct usbnet *dev, > uint32_t rx_speed = le32_to_cpu(data->DLBitRRate); > uint32_t tx_speed = le32_to_cpu(data->ULBitRate); > > - /* if the speed hasn't changed, don't report it. > - * RTL8156 shipped before 2021 sends notification about every 32ms. > - */ > - if (dev->rx_speed == rx_speed && dev->tx_speed == tx_speed) > - return; > - > + /* RTL8156 shipped before 2021 sends notification about every 32ms. */ > dev->rx_speed = rx_speed; > dev->tx_speed = tx_speed; > - > - /* > - * Currently the USB-NET API does not support reporting the actual > - * device speed. Do print it instead. > - */ > - if ((tx_speed > 1000000) && (rx_speed > 1000000)) { > - netif_info(dev, link, dev->net, > - "%u mbit/s downlink %u mbit/s uplink\n", > - (unsigned int)(rx_speed / 1000000U), > - (unsigned int)(tx_speed / 1000000U)); > - } else { > - netif_info(dev, link, dev->net, > - "%u kbit/s downlink %u kbit/s uplink\n", > - (unsigned int)(rx_speed / 1000U), > - (unsigned int)(tx_speed / 1000U)); > - } > } > > static void cdc_ncm_status(struct usbnet *dev, struct urb *urb) > -- > 2.26.2 >
Am Freitag, den 19.02.2021, 07:30 +0000 schrieb Grant Grundler: > On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote: Hi, > Since this patch is missing the hunks that landed in the previous > patch and needs a v4, I'll offer my version of the commit message in That is bad. I will have to search for that. > case you like it better: Something written by a native speaker with knowledge in the field is always better. I will take it, wait two weeks and then resubmit. Regards Oliver
Hi Oliver, On Mon, Feb 22, 2021 at 10:14 AM Oliver Neukum <oneukum@suse.com> wrote: > > Am Freitag, den 19.02.2021, 07:30 +0000 schrieb Grant Grundler: > > On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote: > > Hi, > > > Since this patch is missing the hunks that landed in the previous > > patch and needs a v4, I'll offer my version of the commit message in > > That is bad. I will have to search for that. No worries - it happens. To make this easier for you, let me point out what I've observed. I just noticed there are two hunks that landed in the wrong (posted) patches. 1) In "[PATCHv3 2/3] usbnet: add method for reporting speed without MDIO", this hunk is "repairing" the part that failed to build for Jakub: diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index f3e5ad9befd0..368428a4290b 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -971,10 +971,10 @@ int usbnet_get_link_ksettings_internal(struct net_device *net, * For wireless stuff it is not true. * We assume that rxspeed matters more. */ - if (dev->rxspeed != SPEED_UNKNOWN) - cmd->base.speed = dev->rxspeed / 1000000; - else if (dev->txspeed != SPEED_UNKNOWN) - cmd->base.speed = dev->txspeed / 1000000; + if (dev->rx_speed != SPEED_UNSET) + cmd->base.speed = dev->rx_speed / 1000000; + else if (dev->tx_speed != SPEED_UNSET) + cmd->base.speed = dev->tx_speed / 1000000; else cmd->base.speed = SPEED_UNKNOWN; Just this hunk should have been folded into the previous patch: "[PATCHv3 1/3] usbnet: specify naming of usbnet_set/get_link_ksettings" 2) "[PATCHv3 1/3]" has the hunk to override .get_link_ksettings and .set_link_ksettings fields for cdc_ncm.c: diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 4087c9e33781..0d26cbeb6e04 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -142,8 +142,8 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = { .get_sset_count = cdc_ncm_get_sset_count, .get_strings = cdc_ncm_get_strings, .get_ethtool_stats = cdc_ncm_get_ethtool_stats, - .get_link_ksettings = usbnet_get_link_ksettings, - .set_link_ksettings = usbnet_set_link_ksettings, + .get_link_ksettings = usbnet_get_link_ksettings_internal, + .set_link_ksettings = usbnet_set_link_ksettings_mii, }; This hunk should have been included in "[PATCHv3 3/3] CDC-NCM: record speed in status method" (email thread I'm replying to). Also, I believe this hunk is incorrect: .set_link_ksettings should be set to NULL since speed can't be changed by cdc_ncm driver (AFAIK). Swap around where those hunks landed and you'll be golden. :) > > case you like it better: > > Something written by a native speaker with knowledge in the field is > always better. "knowledge in the field" sounds quite generous. I'll claim I understand this particular problem reasonably well. :) > I will take it, wait two weeks and then resubmit. Excellent! Just to be clear, I'm understanding you will resubmit next week. SGTM. Also, please add the cdc_ether patch (attached) to the series (since it depends on the work you are doing). Andrew Lunn also expected cdc_ether to be updated in the same series (in reply to "[PATCHv2 0/3] usbnet: speed reporting..."). cheers, grant
On Mon, Feb 22, 2021 at 10:14 AM Oliver Neukum <oneukum@suse.com> wrote: > > Am Freitag, den 19.02.2021, 07:30 +0000 schrieb Grant Grundler: > > On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote: > > Hi, > > > Since this patch is missing the hunks that landed in the previous > > patch and needs a v4, I'll offer my version of the commit message in > > That is bad. I will have to search for that. > > > case you like it better: > > Something written by a native speaker with knowledge in the field is > always better. I will take it, wait two weeks and then resubmit. 3 weeks are up. Oliver, did you want to post V4? cheers, grant > > Regards > Oliver > >
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 0d26cbeb6e04..74c1a86b1a71 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -1829,30 +1829,9 @@ cdc_ncm_speed_change(struct usbnet *dev, uint32_t rx_speed = le32_to_cpu(data->DLBitRRate); uint32_t tx_speed = le32_to_cpu(data->ULBitRate); - /* if the speed hasn't changed, don't report it. - * RTL8156 shipped before 2021 sends notification about every 32ms. - */ - if (dev->rx_speed == rx_speed && dev->tx_speed == tx_speed) - return; - + /* RTL8156 shipped before 2021 sends notification about every 32ms. */ dev->rx_speed = rx_speed; dev->tx_speed = tx_speed; - - /* - * Currently the USB-NET API does not support reporting the actual - * device speed. Do print it instead. - */ - if ((tx_speed > 1000000) && (rx_speed > 1000000)) { - netif_info(dev, link, dev->net, - "%u mbit/s downlink %u mbit/s uplink\n", - (unsigned int)(rx_speed / 1000000U), - (unsigned int)(tx_speed / 1000000U)); - } else { - netif_info(dev, link, dev->net, - "%u kbit/s downlink %u kbit/s uplink\n", - (unsigned int)(rx_speed / 1000U), - (unsigned int)(tx_speed / 1000U)); - } } static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)