Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Nov 08, 2021 at 04:06:53PM +0100, Benedikt Spranger wrote:
> On Mon, 8 Nov 2021 14:25:48 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Mon, Nov 08, 2021 at 03:18:34PM +0100, bage@linutronix.de wrote:
> > > From: Bastian Germann <bage@linutronix.de>
> > > 
> > > Take the return of phy_start_aneg into account so that ethtool will
> > > handle negotiation errors and not silently accept invalid input.
> > 
> > I don't think this description is accurate. If we get to call
> > phy_start_aneg() with invalid input, then something has already
> > gone wrong.
> The MDI/MDIX/auto-MDIX settings are not checked before calling
> phy_start_aneg(). If the PHY supports forcing MDI and auto-MDIX, but
> not forcing MDIX _phy_start_aneg() returns a failure, which is silently
> ignored.

That would be very bad if it were to happen.

ksettings_set() would be called. If phy_is_started() returns true, we
trigger the state machine after forcing PHY_UP state. The state machine
calls phy_start_aneg() and gets an error, calling phy_error(), and
the state machine is forced into PHY_HALTED mode with a kernel warning
and stack trace.

That is not nice behaviour for a user.

> Just to be clear: The PHY driver should check the settings and return
> an error before calling phy_ethtool_ksettings_set() ?

The PHY driver doesn't get an opportunity to validate the settings
prior to calling phy_ethtool_ksettings_set() - because the PHY driver
never calls this function. The MAC driver does. However, the MAC driver
doesn't know what the PHY will accept.

This clearly needs to be fixed in phylib.

As a result of your explanation, I now believe your patch to be
fundamentally incorrect given the current state, and it should not be
applied.

We should not accept a call to phy_ethtool_ksettings_set(), modify some
state (setting phydev->advertising/autoneg/master_slave_set/mdix_ctrl/
speed/duplex) and then return an error to the user indicating that the
call failed because we didn't like some parameter - e.g. the mdix_ctrl
value that we've writtent o phydev->mdix_ctrl.

Userspace validation should always happen _prior_ to accepting any
state from userland. If there's an issue with it, the call should fail
without modifying any state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!