netdev - Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <44c85449-1a9b-4a5e-8962-1d2c37138f97@lunn.ch>
Date: Sat, 25 May 2024 19:12:03 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Kamil Horák, 2N <kamilh@...s.com>
Cc: netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

> As far I understand it, the chip is not capable of attempting IEEE and
> BroadR-Reach modes at the same time, not even the BCM54810, which is capable
> of autoneg in BRR. One has to choose IEEE or BRR first then start the
> auto-negotiation (or attempt the link with forced master-slave/speed setting
> for BRR). There are two separate "link is up" bits, one if the IEEE
> registers, second in the BRR one. Theoretically, it should be possible to
> have kind of auto-detection of hardware - for example start with IEEE, if
> there is no link after a timeout, try BRR as well. But as for the circuitry
> necessary to do that, there would have to be something like hardware
> plug-in, as I was told by our HW team. In other words, it is not probable to
> have a device capable of both (IEEE+BRR) modes at once. Thus, having the
> driver to choose from a set containing IEEE and BRR modes makes little
> sense.

So IEEE and BRR autoneg are mutually exclusive. It would be good to
see if 802.3 actually says or implies that. Generic functions like
ksetting_set/get should be based on 802.3, so when designing the API
we should focus on that, not what the particular devices you are
interested in support.

We probably want phydev->supports listing all modes, IEEE and BRR. Is
there a bit equivalent to BMSR_ANEGCAPABLE indicating the hardware can
do BRR autoneg? If there is, we probably want to add a
ETHTOOL_LINK_MODE_Autoneg_BRR_BIT.

ksetting_set should enforce this mutual exclusion. So
phydev->advertise should never be set containing invalid combination,
ksetting_set() should return an error.

I guess we need to initialize phydev->advertise to IEEE link modes in
order to not cause regressions. However, if the PHY does not support
any IEEE modes, it can then default to BRR link modes. It would also
make sense to have a standardized DT property to indicate BRR should
be used by default.

> Our use case is fixed master/slave and fixed speed (10/100), and BRR on 1
> pair only with BCM54811.  I can imagine autoneg master/slave and 10/100 in
> the same physical media (one pair) but that would require BCM54810.

> Not sure about how many other drivers
> regularly used fit this scheme, it seems that vast majority prefers
> auto-negotiation... However, it could be even made so that direct linkmode
> selection would work everywhere, leaving to the phy driver the choice of
> whether start autoneg with only one option or force that option directly
> when there is no aneg at all (BCM54811 in BRR mode).

No, this is not correct. There is a difference between autoneg with a
single mode, and forced. forced does not attempt to perform autoneg,
the hardware is just configured to a particular link mode. autoneg
with a single link mode does perform autoneg, you get to see what the
link partner is advertising etc. Either you can resolve to a common
link code and autoneg is successful, or there are no common modes and
autoneg fails.

A driver does not have a choice here, it need to do what it is told to
do.

	Andrew