[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 <and...@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