Re: [PATCH v2 2/2] net: phy: microchip: enable downshift by default on LAN88xx

[Nicolai Buchwitz]

On 31.3.2026 14:01, Russell King (Oracle) wrote:
> On Tue, Mar 31, 2026 at 01:58:55PM +0200, Nicolai Buchwitz wrote:
> > On 31.3.2026 13:32, Russell King (Oracle) wrote:
> > > On Tue, Mar 31, 2026 at 12:46:27AM +0200, Nicolai Buchwitz wrote:
> > > > Enable auto-downshift from 1000BASE-T to 100BASE-TX after 2 failed
> > > > auto-negotiation attempts by default. This ensures that links with
> > > > faulty or missing cable pairs (C and D) fall back to 100Mbps without
> > > > requiring userspace configuration.
> > > >
> > > > Users can override or disable downshift at runtime:
> > > >
> > > >   ethtool --set-phy-tunable eth0 downshift off
> > > >
> > > > Signed-off-by: Nicolai Buchwitz <nb@xxxxxxxxxxx>
> > > > Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> > >
> > > I'm slightly concerned by this commit. ->config_init() is called when
> > > the netdev attaches the PHY, and also during the resume path - and it's
> > > the second one which I believe is a problem here.
> > >
> > > If the user has configured the downshift, it is reasonable for the user
> > > to expect the setting to be preserved over a suspend/resume. However,
> > > by placing this code in ->config_init(), you will overwrite the user's
> > > setting when the system resumes.
> >
> > You have a valid point. Looking at other drivers, Marvell has the
> > same issue: m88e1112_config_init() unconditionally sets downshift to 3
> > on every config_init call.
> >
> > I see two options:
> >
> > 1. Save the user's setting in the driver's priv struct and restore it
> > in config_init instead of blindly applying the default.
> >
> > 2. Handle it generically in the PHY core, saving/restoring tunable
> > state across suspend/resume for all drivers.
> >
> > I'd lean towards (1) to keep this series simple. (2) could be a
> > follow-up that fixes Marvell and others too. What do you think?
>
> Or (3) configure the default it in the probe function?

Unfortunately the driver has .soft_reset = genphy_soft_reset which
runs before config_init on resume and would clear the setting? So
config_init still needs to set it.

Nicolai