| Message ID | 20210405231344.1403025-1-grundler@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | usbnet: speed reporting for devices without MDIO | expand |
On Mon, Apr 05, 2021 at 04:13:40PM -0700, Grant Grundler wrote: > This series introduces support for USB network devices that report > speed as a part of their protocol, not emulating an MII to be accessed > over MDIO. > > v2: rebased on recent upstream changes > v3: incorporated hints on naming and comments > v4: fix misplaced hunks; reword some commit messages; > add same change for cdc_ether > v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by > > I'm reposting Oliver Neukum's <oneukum@suse.com> patch series with > fix ups for "misplaced hunks" (landed in the wrong patches). > Please fixup the "author" if "git am" fails to attribute the > patches 1-3 (of 4) to Oliver. > > I've tested v4 series with "5.12-rc3+" kernel on Intel NUC6i5SYB > and + Sabrent NT-S25G. Google Pixelbook Go (chromeos-4.4 kernel) > + Alpha Network AUE2500C were connected directly to the NT-S25G > to get 2.5Gbps link rate: > # ethtool enx002427880815 > Settings for enx002427880815: > Supported ports: [ ] > Supported link modes: Not reported > Supported pause frame use: No > Supports auto-negotiation: No > Supported FEC modes: Not reported > Advertised link modes: Not reported > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Speed: 2500Mb/s > Duplex: Half > Auto-negotiation: off > Port: Twisted Pair > PHYAD: 0 > Transceiver: internal > MDI-X: Unknown > Current message level: 0x00000007 (7) > drv probe link > Link detected: yes > > > "Duplex" is a lie since we get no information about it. You can ask the PHY. At least those using mii or phylib. If you are using mii, then mii_ethtool_get_link_ksettings() should set it correctly. If you are using phylib, phy_ethtool_get_link_ksettings() will correctly set it. If you are not using either of these, you are on your own. Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only ever see 10 Half and occasionally 100 Half. Anything above that will be full duplex. It is probably best to admit the truth and use DUPLEX_UNKNOWN. > I expect "Auto-Negotiation" is always true for cdc_ncm and > cdc_ether devices and perhaps someone knows offhand how > to have ethtool report "true" instead. ethtool_link_ksettings contains three bitmaps: supported: The capabilities of this device. advertising: What this device is telling the link peer it can do. lp_advertising: What the link peer is telling us it can do. So to get Supports auto-negotiation to be true you need to set bit ETHTOOL_LINK_MODE_Autoneg_BIT in supported. For Advertised auto-negotiation: you need to set the same bit in advertising. Auto-negotiation: off is i think from base.autoneg. Andrew
On Tue, Apr 6, 2021 at 12:09 AM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Apr 05, 2021 at 04:13:40PM -0700, Grant Grundler wrote: > > This series introduces support for USB network devices that report > > speed as a part of their protocol, not emulating an MII to be accessed > > over MDIO. > > > > v2: rebased on recent upstream changes > > v3: incorporated hints on naming and comments > > v4: fix misplaced hunks; reword some commit messages; > > add same change for cdc_ether > > v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by > > > > I'm reposting Oliver Neukum's <oneukum@suse.com> patch series with > > fix ups for "misplaced hunks" (landed in the wrong patches). > > Please fixup the "author" if "git am" fails to attribute the > > patches 1-3 (of 4) to Oliver. > > > > I've tested v4 series with "5.12-rc3+" kernel on Intel NUC6i5SYB > > and + Sabrent NT-S25G. Google Pixelbook Go (chromeos-4.4 kernel) > > + Alpha Network AUE2500C were connected directly to the NT-S25G > > to get 2.5Gbps link rate: > > # ethtool enx002427880815 > > Settings for enx002427880815: > > Supported ports: [ ] > > Supported link modes: Not reported > > Supported pause frame use: No > > Supports auto-negotiation: No > > Supported FEC modes: Not reported > > Advertised link modes: Not reported > > Advertised pause frame use: No > > Advertised auto-negotiation: No > > Advertised FEC modes: Not reported > > Speed: 2500Mb/s > > Duplex: Half > > Auto-negotiation: off > > Port: Twisted Pair > > PHYAD: 0 > > Transceiver: internal > > MDI-X: Unknown > > Current message level: 0x00000007 (7) > > drv probe link > > Link detected: yes > > > > > > "Duplex" is a lie since we get no information about it. > > You can ask the PHY. At least those using mii or phylib. If you are > using mii, then mii_ethtool_get_link_ksettings() should set it > correctly. If you are using phylib, phy_ethtool_get_link_ksettings() > will correctly set it. If you are not using either of these, you are > on your own. > > Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only > ever see 10 Half and occasionally 100 Half. Anything above that will > be full duplex. > > It is probably best to admit the truth and use DUPLEX_UNKNOWN. Agreed. I didn't notice this "lie" until I was writing the commit message and wasn't sure off-hand how to fix it. Decided a follow on patch could fix it up once this series lands. You are right that DUPLEX_UNKNOWN is the safest (and usually correct) default. Additionally, if RX and TX speed are equal, I am willing to assume this is DUPLEX_FULL. I can propose something like this in a patch: grundler <1637>git diff diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 86eb1d107433..a7ad9a0fb6ae 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -978,6 +978,11 @@ int usbnet_get_link_ksettings_internal(struct net_device *net, else cmd->base.speed = SPEED_UNKNOWN; + if (dev->rx_speed == dev->tx_speed) + cmd->base.duplex = DUPLEX_FULL; + else + cmd->base.duplex =DUPLEX_UNKNOWN; + return 0; } EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal); Probably should check that link speed is > 100Mbps to be more certain about this assumption (based on your comments above). I can send this out later once this series lands or you are welcome to post this with additional checks if you like. The messy case is when RX != TX speed and I didn't want to delay landing the current series to figure out DUPLEX. > > I expect "Auto-Negotiation" is always true for cdc_ncm and > > cdc_ether devices and perhaps someone knows offhand how > > to have ethtool report "true" instead. > > ethtool_link_ksettings contains three bitmaps: > > supported: The capabilities of this device. > advertising: What this device is telling the link peer it can do. > lp_advertising: What the link peer is telling us it can do. > > So to get Supports auto-negotiation to be true you need to set bit > ETHTOOL_LINK_MODE_Autoneg_BIT in supported. > For Advertised auto-negotiation: you need to set the same bit in > advertising. > > Auto-negotiation: off is i think from base.autoneg. Thanks for explaining! :) I understand the three bitmaps. I just hadn't taken the time to figure out how to access/set those from link_ksettings API. If we want to assume autoneg is always on (regardless of which type of media cdc_ncm/cdc_ether are talking to), we could set both supported and advertising to AUTO and lp_advertising to UNKNOWN. But I would prefer to add more checks to prove this is correct (vs making "well intentioned" assumptions). cheers, grant
> > Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only > > ever see 10 Half and occasionally 100 Half. Anything above that will > > be full duplex. > > > > It is probably best to admit the truth and use DUPLEX_UNKNOWN. > > Agreed. I didn't notice this "lie" until I was writing the commit > message and wasn't sure off-hand how to fix it. Decided a follow on > patch could fix it up once this series lands. > > You are right that DUPLEX_UNKNOWN is the safest (and usually correct) default. > Additionally, if RX and TX speed are equal, I am willing to assume > this is DUPLEX_FULL. Is this same interface used by WiFi? Ethernet does not support different rates in each direction. So if RX and TX are different, i would actually say something is broken. 10 Half is still doing 10Mbps in each direction, it just cannot do both at the same time. WiFi can have asymmetric speeds. > I can propose something like this in a patch: > > grundler <1637>git diff > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 86eb1d107433..a7ad9a0fb6ae 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -978,6 +978,11 @@ int usbnet_get_link_ksettings_internal(struct > net_device *net, > else > cmd->base.speed = SPEED_UNKNOWN; > > + if (dev->rx_speed == dev->tx_speed) > + cmd->base.duplex = DUPLEX_FULL; > + else > + cmd->base.duplex =DUPLEX_UNKNOWN; > + > return 0; > } > EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal); So i would say this is wrong. I would just set DUPLEX_UNKNOWN and be done. > I can send this out later once this series lands or you are welcome to > post this with additional checks if you like. Yes, this discussion should not prevent this patchset from being merged. > If we want to assume autoneg is always on (regardless of which type of > media cdc_ncm/cdc_ether are talking to), we could set both supported > and advertising to AUTO and lp_advertising to UNKNOWN. I pretty much agree autoneg has to be on. If it is not, and it is using a forced speed, there would need to be an additional API to set what it is forced to. There could be such proprietary calls, but the generic cdc_ncm/cdc_ether won't support them. But i also don't know how setting autoneg actually helps the user. Everybody just assumes it is supported. If you really know auto-neg is not supported and you can reliably indicate that autoneg is not supported, that would be useful. But i expect most users want to know if their USB 2.0 device is just doing 100Mbps, or if their USB 3.0 device can do 2.5G. For that, you need to see what is actually supported. Andrew
[Key part of Andew's reply: "Yes, this discussion should not prevent this patchset from being merged."] On Tue, Apr 6, 2021 at 1:00 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only > > > ever see 10 Half and occasionally 100 Half. Anything above that will > > > be full duplex. > > > > > > It is probably best to admit the truth and use DUPLEX_UNKNOWN. > > > > Agreed. I didn't notice this "lie" until I was writing the commit > > message and wasn't sure off-hand how to fix it. Decided a follow on > > patch could fix it up once this series lands. > > > > You are right that DUPLEX_UNKNOWN is the safest (and usually correct) default. > > Additionally, if RX and TX speed are equal, I am willing to assume > > this is DUPLEX_FULL. > > Is this same interface used by WiFi? I doubt WiFi could work with this driver interface (though maybe with "SendEncapsulatedCommand"). All the Wifi Devices I'm familiar with need WPA support and communicate through 80211 kernel subsystem. I was thinking of just about everything else: Cellular modem (cdc_ether), xDSL, or other broadband. > Ethernet does not support > different rates in each direction. So if RX and TX are different, i > would actually say something is broken. Agreed. The question is: Is there data or some heuristics we can use to determine if the physical layer/link is ethernet? I'm pessimistic we will be able to since this is at odds with the intent of the CDC spec. > 10 Half is still doing 10Mbps > in each direction, it just cannot do both at the same time. > WiFi can have asymmetric speeds. *nod* > > I can propose something like this in a patch: > > > > grundler <1637>git diff > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > > index 86eb1d107433..a7ad9a0fb6ae 100644 > > --- a/drivers/net/usb/usbnet.c > > +++ b/drivers/net/usb/usbnet.c > > @@ -978,6 +978,11 @@ int usbnet_get_link_ksettings_internal(struct > > net_device *net, > > else > > cmd->base.speed = SPEED_UNKNOWN; > > > > + if (dev->rx_speed == dev->tx_speed) > > + cmd->base.duplex = DUPLEX_FULL; > > + else > > + cmd->base.duplex =DUPLEX_UNKNOWN; > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal); > > So i would say this is wrong. I would just set DUPLEX_UNKNOWN and be > done. Ok. > > I can send this out later once this series lands or you are welcome to > > post this with additional checks if you like. > > Yes, this discussion should not prevent this patchset from being > merged. Good. That's what I'm hoping for. > > If we want to assume autoneg is always on (regardless of which type of > > media cdc_ncm/cdc_ether are talking to), we could set both supported > > and advertising to AUTO and lp_advertising to UNKNOWN. > > I pretty much agree autoneg has to be on. If it is not, and it is > using a forced speed, there would need to be an additional API to set > what it is forced to. There could be such proprietary calls, but the > generic cdc_ncm/cdc_ether won't support them. Good observation. Agreed. > But i also don't know how setting autoneg actually helps the user. Just to let them know the link rate can change and is dynamically determined. > Everybody just assumes it is supported. If you really know auto-neg is > not supported and you can reliably indicate that autoneg is not > supported, that would be useful. But i expect most users want to know > if their USB 2.0 device is just doing 100Mbps, or if their USB 3.0 > device can do 2.5G. For that, you need to see what is actually > supported. Yes. Other than using a table to look up USB VID:PID, I don't see anything in the spec which provides "media-specific" information. I was curious about the "can do 2.5Gbps?" question by looking at the CDC Ethernet Networking Functional Descriptor (USBECM12) and other CDC specs. The spec feels like a "compatibility wrapper" to make a cellular modem look like an ethernet device. This statement in the ECM120.pdf I have suggests we can not determine media layer: The effect of a "reset" on the device physical layer is media-dependent and beyond the scope of this specification. cheers, grant
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Mon, 5 Apr 2021 16:13:40 -0700 you wrote: > This series introduces support for USB network devices that report > speed as a part of their protocol, not emulating an MII to be accessed > over MDIO. > > v2: rebased on recent upstream changes > v3: incorporated hints on naming and comments > v4: fix misplaced hunks; reword some commit messages; > add same change for cdc_ether > v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by > > [...] Here is the summary with links: - [net-next,v4,1/4] usbnet: add _mii suffix to usbnet_set/get_link_ksettings https://git.kernel.org/netdev/net-next/c/77651900cede - [net-next,v4,2/4] usbnet: add method for reporting speed without MII https://git.kernel.org/netdev/net-next/c/956baa99571b - [net-next,v4,3/4] net: cdc_ncm: record speed in status method https://git.kernel.org/netdev/net-next/c/eb47c274d8c4 - [net-next,v4,4/4] net: cdc_ether: record speed in status method https://git.kernel.org/netdev/net-next/c/d42ebcbb6353 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Am Dienstag, den 06.04.2021, 18:01 +0000 schrieb Grant Grundler: > > Ethernet does not support > > different rates in each direction. So if RX and TX are different, i > > would actually say something is broken. > > Agreed. The question is: Is there data or some heuristics we can use > to determine if the physical layer/link is ethernet? > I'm pessimistic we will be able to since this is at odds with the > intent of the CDC spec. Yes, CDC intends to hide that. We can conclude that an asymmetric link rules out ethernet, but anything else is difficult. Regards Oliver
This series introduces support for USB network devices that report speed as a part of their protocol, not emulating an MII to be accessed over MDIO. v2: rebased on recent upstream changes v3: incorporated hints on naming and comments v4: fix misplaced hunks; reword some commit messages; add same change for cdc_ether v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by I'm reposting Oliver Neukum's <oneukum@suse.com> patch series with fix ups for "misplaced hunks" (landed in the wrong patches). Please fixup the "author" if "git am" fails to attribute the patches 1-3 (of 4) to Oliver. I've tested v4 series with "5.12-rc3+" kernel on Intel NUC6i5SYB and + Sabrent NT-S25G. Google Pixelbook Go (chromeos-4.4 kernel) + Alpha Network AUE2500C were connected directly to the NT-S25G to get 2.5Gbps link rate: # ethtool enx002427880815 Settings for enx002427880815: Supported ports: [ ] Supported link modes: Not reported Supported pause frame use: No Supports auto-negotiation: No Supported FEC modes: Not reported Advertised link modes: Not reported Advertised pause frame use: No Advertised auto-negotiation: No Advertised FEC modes: Not reported Speed: 2500Mb/s Duplex: Half Auto-negotiation: off Port: Twisted Pair PHYAD: 0 Transceiver: internal MDI-X: Unknown Current message level: 0x00000007 (7) drv probe link Link detected: yes "Duplex" is a lie since we get no information about it. I expect "Auto-Negotiation" is always true for cdc_ncm and cdc_ether devices and perhaps someone knows offhand how to have ethtool report "true" instead. But this is good step in the right direction. base-commit: 1c273e10bc0cc7efb933e0ca10e260cdfc9f0b8c