Re: [net-next PATCH 06/14] net: phy: at803x: move at8031 specific data out of generic at803x_priv

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 12:08:52PM +0100, Christian Marangi wrote:
> On Wed, Nov 29, 2023 at 09:35:37AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 29, 2023 at 03:12:11AM +0100, Christian Marangi wrote:
> > > Rework everything related to specific at8031 function to specific
> > > function and allocate the 2 bool, is_1000basex and is_fiber and the
> > > regulator structs to a dedicated qca8031_data struct.
> > > 
> > > This is needed to keep at803x functions more generic and detach them
> > > from specific check of at8031/33 PHY.
> > > 
> > > Out of all the reworked functions, only config_aneg required some code
> > > duplication with how the mdix config is handled.
> > > 
> > > This also reduces the generic at803x_priv struct by removing variables
> > > only used by at8031 PHY.
> > 
> > You are changing the order that register writes happen, e.g. for the
> > set_wol() method. at803x_set_wol() very clearly does stuff like
> > configuring the ethernet MAC address _before_ enabling WoL, and that
> > can fail. Your new code enables WoL and then calls at803x_set_wol().
> > If at803x_set_wol() fails (e.g. because of an invalid MAC address)
> > you leave WoL enabled. This is a change of behaviour.
> >
> 
> Have to think about it, changing the order of the WoL module enable and
> setting the MAC should not change anything as the real enablement is the
> WoL interrupt. (I guess this is why the WoL module is enabled by default
> as the interrupt is disabled by default resulting in the module doing
> nothing)

The AR8031 has two hardware pins for signalling WoL. One of them is the
main INT pin, which is controlled by register 0x12 bit 0. This is an
interrupt enable bit, and it only affects the INT pin.

The second is the WOL_INT pin, which is _not_ controlled by register
0x12 bit 0. This can only be controlled via the AT803X_WOL_EN in the
1588 register.

You have moved the control of AT803X_WOL_EN before the setup of the MAC,
setting and clearing it before calling the other function. This means
that if the MAC is invalid, AT803X_WOL_EN can be set, but the MAC
address has not been programmed, which can leave the machine vulnerable
to spurious wakeups if the WOL_INT pin is used for that purpose.

The original code gets the order correct. Your replacement code breaks
this ordering, thus making it less correct.

> Would it be better to split this in more smaller commit? One for moving
> the at8031 function and the other for refactor of some function? 

Given how big the series already is, you're in danger of going over the
15 patch limit for netdev submissions, so I think careful thought on
that would be needed (e.g. possibly splitting this series.) Wait until
Andrew has also reviewed the series before you decide on that though.

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