From: Grant Grundler <grundler@chromium.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Grant Grundler <grundler@chromium.org>,
Oliver Neukum <oneukum@suse.com>,
Jakub Kicinski <kuba@kernel.org>,
Roland Dreier <roland@kernel.org>,
nic_swsd <nic_swsd@realtek.com>, netdev <netdev@vger.kernel.org>,
"David S . Miller" <davem@davemloft.net>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO
Date: Tue, 6 Apr 2021 04:47:43 +0000 [thread overview]
Message-ID: <CANEJEGsYQm9EhqVLA4oedP2fuKrP=3bOUDV9=7owfdZzX7SpUA@mail.gmail.com> (raw)
In-Reply-To: <YGumuzcPl+9l5ZHV@lunn.ch>
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
next prev parent reply other threads:[~2021-04-06 4:48 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-05 23:13 [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO Grant Grundler
2021-04-05 23:13 ` [PATCH net-next v4 1/4] usbnet: add _mii suffix to usbnet_set/get_link_ksettings Grant Grundler
2021-04-05 23:13 ` [PATCH net-next v4 2/4] usbnet: add method for reporting speed without MII Grant Grundler
2021-04-05 23:13 ` [PATCH net-next v4 3/4] net: cdc_ncm: record speed in status method Grant Grundler
2021-04-05 23:13 ` [PATCH net-next v4 4/4] net: cdc_ether: " Grant Grundler
2021-04-06 0:09 ` [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO Andrew Lunn
2021-04-06 4:47 ` Grant Grundler [this message]
2021-04-06 13:00 ` Andrew Lunn
2021-04-06 18:01 ` Grant Grundler
2021-04-07 11:36 ` Oliver Neukum
2021-04-06 23:30 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANEJEGsYQm9EhqVLA4oedP2fuKrP=3bOUDV9=7owfdZzX7SpUA@mail.gmail.com' \
--to=grundler@chromium.org \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nic_swsd@realtek.com \
--cc=oneukum@suse.com \
--cc=roland@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).