diff mbox series

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

Message ID 20221228164008.1653348-2-michael@walle.cc
State New
Headers
Series net: phy: mxl-gpy: broken interrupt fixes |

Commit Message

Michael Walle Dec. 28, 2022, 4:40 p.m. UTC 
  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@walle.cc>
---
 drivers/net/phy/phy_device.c | 7 +++++++
 include/linux/phy.h          | 2 ++
 2 files changed, 9 insertions(+)

Comments

Florian Fainelli Dec. 28, 2022, 4:49 p.m. UTC | #1 
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@walle.cc>
> ---
>   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 :)
Andrew Lunn Dec. 28, 2022, 4:54 p.m. UTC | #2 
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@walle.cc>
> > ---
> >   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?

It is actually to early. The interrupt is requested when the MAC
attaches the PHY. There are is at least one MAC driver which assigns
the phydev->irq just before attaching the PHY, a long time after the
PHY has probed.

    Andrew
Russell King (Oracle) Jan. 3, 2023, 10:27 a.m. UTC | #3 
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@walle.cc>
> > ---
> >   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.
diff mbox series

Patch

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;
+
 	/* 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
+
 /**
  * enum phy_interface_t - Interface Mode definitions
  *