[RFC net-next v3 1/2] net: phy: phy_device Prevent nullptr exceptions on ISR

[RFC net-next v3 1/2] net: phy: phy_device Prevent nullptr exceptions on ISR

[Andre Werner]

If phydev->irq is set unconditionally by MAC drivers, check
for valid interrupt handler or fall back to polling mode to prevent
nullptr exceptions.

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
---
v3:
- No changes to v2. Just to complete the series.
---
 drivers/net/phy/phy_device.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..3986e103d25e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1413,6 +1413,11 @@ int phy_sfp_probe(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(phy_sfp_probe);
 
+static bool phy_drv_supports_irq(struct phy_driver *phydrv)
+{
+	return phydrv->config_intr && phydrv->handle_interrupt;
+}
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1527,6 +1532,18 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (phydev->dev_flags & PHY_F_NO_IRQ)
 		phydev->irq = PHY_POLL;
 
+	/*
+	 * Some drivers may add IRQ numbers unconditionally to a phy device that does
+	 * not implement an interrupt handler after phy_probe is already done.
+	 * Reset to PHY_POLL to prevent nullptr exceptions in that case.
+	 */
+	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev)) {
+		phydev_warn(phydev,
+			    "No handler for IRQ=%d available. Falling back to polling mode\n",
+			    phydev->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
@@ -2992,11 +3009,6 @@ s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
 }
 EXPORT_SYMBOL(phy_get_internal_delay);
 
-static bool phy_drv_supports_irq(struct phy_driver *phydrv)
-{
-	return phydrv->config_intr && phydrv->handle_interrupt;
-}
-
 static int phy_led_set_brightness(struct led_classdev *led_cdev,
 				  enum led_brightness value)
 {
-- 
2.43.0

[RFC net-next v3 2/2] net: phy: adin1100: Add interrupt support for link change

[Andre Werner]

An interrupt handler was added to the driver as well as functions
to enable interrupts at the phy.

There are several interrupts maskable at the phy, but only link change
interrupts are handled by the driver yet.

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
---
v3:
- Correct rashly format error that was reported by checker.
---
 drivers/net/phy/adin1100.c | 61 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
index 7619d6185801..32700b44e486 100644
--- a/drivers/net/phy/adin1100.c
+++ b/drivers/net/phy/adin1100.c
@@ -18,6 +18,12 @@
 #define PHY_ID_ADIN1110				0x0283bc91
 #define PHY_ID_ADIN2111				0x0283bca1
 
+#define ADIN_PHY_SUBSYS_IRQ_MASK		0x0021
+#define   ADIN_LINK_STAT_CHNG_IRQ_EN		BIT(1)
+
+#define ADIN_PHY_SUBSYS_IRQ_STATUS		0x0011
+#define   ADIN_LINK_STAT_CHNG			BIT(1)
+
 #define ADIN_FORCED_MODE			0x8000
 #define   ADIN_FORCED_MODE_EN			BIT(0)
 
@@ -136,6 +142,59 @@ static int adin_config_aneg(struct phy_device *phydev)
 	return genphy_c45_config_aneg(phydev);
 }
 
+static int adin_phy_ack_intr(struct phy_device *phydev)
+{
+	/* Clear pending interrupts */
+	int rc = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+			      ADIN_PHY_SUBSYS_IRQ_STATUS);
+
+	return rc < 0 ? rc : 0;
+}
+
+static int adin_config_intr(struct phy_device *phydev)
+{
+	int ret, regval;
+
+	ret = adin_phy_ack_intr(phydev);
+
+	if (ret)
+		return ret;
+
+	regval = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+			      ADIN_PHY_SUBSYS_IRQ_MASK);
+	if (regval < 0)
+		return regval;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		regval |= ADIN_LINK_STAT_CHNG_IRQ_EN;
+	else
+		regval &= ~ADIN_LINK_STAT_CHNG_IRQ_EN;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+			    ADIN_PHY_SUBSYS_IRQ_MASK,
+			    regval);
+	return ret;
+}
+
+static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+				  ADIN_PHY_SUBSYS_IRQ_STATUS);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (!(irq_status & ADIN_LINK_STAT_CHNG))
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
 static int adin_set_powerdown_mode(struct phy_device *phydev, bool en)
 {
 	int ret;
@@ -275,6 +334,8 @@ static struct phy_driver adin_driver[] = {
 		.probe			= adin_probe,
 		.config_aneg		= adin_config_aneg,
 		.read_status		= adin_read_status,
+		.config_intr		= adin_config_intr,
+		.handle_interrupt	= adin_phy_handle_interrupt,
 		.set_loopback		= adin_set_loopback,
 		.suspend		= adin_suspend,
 		.resume			= adin_resume,
-- 
2.43.0

Re: [RFC net-next v3 2/2] net: phy: adin1100: Add interrupt support for link change

[Russell King (Oracle)]

On Fri, Jan 19, 2024 at 10:32:26AM +0100, Andre Werner wrote:
> An interrupt handler was added to the driver as well as functions
> to enable interrupts at the phy.
> 
> There are several interrupts maskable at the phy, but only link change
> interrupts are handled by the driver yet.
> 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>

NAK. Previous feedback not actioned.

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

Re: [RFC net-next v3 1/2] net: phy: phy_device Prevent nullptr exceptions on ISR

[Andrew Lunn]

On Fri, Jan 19, 2024 at 10:32:25AM +0100, Andre Werner wrote:
> If phydev->irq is set unconditionally by MAC drivers, check
> for valid interrupt handler or fall back to polling mode to prevent
> nullptr exceptions.

Hi Andre

A few more process things...

Please don't post a new version within 24 hours. Reviews need time to
review, and you could miss comments made on older versions of the
patches.

For a multi part patch set, its normal to include a clover
letter. When using git format-patch add --cover-letter and than edit
patch 0000-*.patch to describe the big picture of what the patchset
does. The text will be used for the merge commit message.
 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> ---
> v3:
> - No changes to v2. Just to complete the series.
> ---
>  drivers/net/phy/phy_device.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3611ea64875e..3986e103d25e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1413,6 +1413,11 @@ int phy_sfp_probe(struct phy_device *phydev,
>  }
>  EXPORT_SYMBOL(phy_sfp_probe);
>  
> +static bool phy_drv_supports_irq(struct phy_driver *phydrv)
> +{
> +	return phydrv->config_intr && phydrv->handle_interrupt;
> +}
> +
>  /**
>   * phy_attach_direct - attach a network device to a given PHY device pointer
>   * @dev: network device to attach
> @@ -1527,6 +1532,18 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	if (phydev->dev_flags & PHY_F_NO_IRQ)
>  		phydev->irq = PHY_POLL;
>  
> +	/*
> +	 * Some drivers may add IRQ numbers unconditionally to a phy device that does
> +	 * not implement an interrupt handler after phy_probe is already done.
> +	 * Reset to PHY_POLL to prevent nullptr exceptions in that case.
> +	 */
> +	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev)) {
> +		phydev_warn(phydev,
> +			    "No handler for IRQ=%d available. Falling back to polling mode\n",
> +			    phydev->irq);
> +		phydev->irq = PHY_POLL;
> +	}

Please drop the phydev_warn(). Interrupt handling has always been
optional, and we have always silently dropped back to polling if
interrupts are not supported.

The comment wording is also not great. The MAC driver is not supposed
to have any idea what the PHY driver is. It just uses the phylib API,
which is PHY driver independent. So the MAC driver cannot tell if the
PHY driver supports interrupts or not.

I don't think a comment is needed. The code is pretty readable as is.

    Andrew

---
pw-bot: cr