Re: [PATCH] net: phy: dp83869: fix memory corruption when enabling fiber

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

Hi Ingo!

On Tue, 2024-10-01 at 15:31 +0200, Ingo van Lil wrote:
> On 10/1/24 12:40, Sverdlin, Alexander wrote:
> 
> > > diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> > > index d7aaefb5226b..9c5ac5d6a9fd 100644
> > > --- a/drivers/net/phy/dp83869.c
> > > +++ b/drivers/net/phy/dp83869.c
> > > @@ -645,7 +645,7 @@ static int dp83869_configure_fiber(struct phy_device *phydev,
> > >    		     phydev->supported);
> > > 
> > >    	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
> > > -	linkmode_set_bit(ADVERTISED_FIBRE, phydev->advertising);
> > > +	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->advertising);
> > 
> > Are you sure this linkmode_set_bit() is required at all?
> 
> You're right, it's probably not required. I just tracked a weird bug
> down to this clear mistake and wanted to change as little as possible.

As little as possible would be not to add yet another bit set.
Obviously it has been working (if it was at all) without a proper write,
but dispite the incorrect write.

> The logic of the function seems a bit odd to me: At the beginning,
> advertising is ANDed with supported, and at the end it's ORed again.
> Inside the function they are mostly manipulated together.
> 
> Couldn't that be replaced with a simple "phydev->advertising =
> phydev->supported;" at the end?

Yes, the function looks strange.
But as this is for -stable, maybe complete rework is undesired.
IMO, just delete the bogus write.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com