Re: [PATCH RFC net-next v2 1/2] net: phy: allow a phy to opt-out of interrupt handling

[Russell King (Oracle)]

On Wed, Dec 28, 2022 at 08:49:35AM -0800, Florian Fainelli wrote:
> 
> 
> On 12/28/2022 8:40 AM, Michael Walle wrote:
> > Until now, it is not possible for a PHY driver to disable interrupts
> > during runtime. If a driver offers the .config_intr() as well as the
> > .handle_interrupt() ops, it is eligible for interrupt handling.
> > Introduce a new flag for the dev_flags property of struct phy_device, which
> > can be set by PHY driver to skip interrupt setup and fall back to polling
> > mode.
> > 
> > At the moment, this is used for the MaxLinear PHY which has broken
> > interrupt handling and there is a need to disable interrupts in some
> > cases.
> > 
> > Signed-off-by: Michael Walle <michael@xxxxxxxx>
> > ---
> >   drivers/net/phy/phy_device.c | 7 +++++++
> >   include/linux/phy.h          | 2 ++
> >   2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 716870a4499c..e4562859ac00 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1487,6 +1487,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> >   	phydev->interrupts = PHY_INTERRUPT_DISABLED;
> > +	/* PHYs can request to use poll mode even though they have an
> > +	 * associated interrupt line. This could be the case if they
> > +	 * detect a broken interrupt handling.
> > +	 */
> > +	if (phydev->dev_flags & PHY_F_NO_IRQ)
> > +		phydev->irq = PHY_POLL;
> 
> Cannot you achieve the same thing with the PHY driver mangling phydev->irq
> to a negative value, or is that too later already by the time your phy
> driver's probe function is running?
> 
> > +
> >   	/* Port is set to PORT_TP by default and the actual PHY driver will set
> >   	 * it to different value depending on the PHY configuration. If we have
> >   	 * the generic PHY driver we can't figure it out, thus set the old
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 71eeb4e3b1fd..f1566c7e47a8 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -82,6 +82,8 @@ extern const int phy_10gbit_features_array[1];
> >   #define PHY_POLL_CABLE_TEST	0x00000004
> >   #define MDIO_DEVICE_IS_PHY	0x80000000
> > +#define PHY_F_NO_IRQ		0x80000000
> 
> Kudos for using the appropriate namespace for dev_flags :)

But eww for placement.

PHY_IS_INTERNAL, PHY_RST_AFTER_CLK_EN, PHY_POLL_CABLE_TEST and
MDIO_DEVICE_IS_PHY are all used for the MDIO driver's flags
member.

This new flag is used for the .dev_flags of phy_device - I feel
that it should be separated from the above definitions. I also
think it could do with a comment, because it's not obvious for
future changes that PHY_F_NO_IRQ is used with .dev_flags.

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