[RFC net-next v4 0/2] Prevent nullptr exceptions in ISR

[RFC net-next v4 0/2] Prevent nullptr exceptions in ISR

[Andre Werner]

In case phydev->irq is modified unconditionally to a valid IRQ, handling
the IRQ may lead to a nullptr exception if no interrupt handler is
registered to the phy driver. phy_interrupt calls a
phy_device->handle_interrupt unconditionally. And interrupts are enabled
in phy_connect_direct if phydev->irq is not equal to PHY_POLL or
PHY_MAC_INTERRUPT, so it does not check for a phy driver providing an ISR.

Adding an additonal check for a valid interrupt handler in phy_attach_direct
function, and falling back to polling mode if not, should prevent for
such nullptr exceptions.

Moreover, the ADIN1100 phy driver is extended with an interrupt handler
for changes in the link status.

Andre Werner (2):
  net: phy: phy_device: Prevent nullptr exceptions on ISR
  net: phy: adin1100: Add interrupt support for link change

 drivers/net/phy/adin1100.c   | 56 ++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c | 13 +++++----
 2 files changed, 64 insertions(+), 5 deletions(-)

-- 
2.43.0

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

[Andre Werner]

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

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
---
v4:
- Delete phydev_warn and comment as suggested.
---
 drivers/net/phy/phy_device.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..7cbe7c714177 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,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (phydev->dev_flags & PHY_F_NO_IRQ)
 		phydev->irq = PHY_POLL;
 
+	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))
+		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 +3000,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

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

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:54:48PM +0100, Andre Werner wrote:
> If phydev->irq is set unconditionally, check
> for valid interrupt handler or fall back to polling mode to prevent
> nullptr exceptions in interrupt service routine.
> 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:54:48PM +0100, Andre Werner wrote:
> If phydev->irq is set unconditionally, check
> for valid interrupt handler or fall back to polling mode to prevent
> nullptr exceptions in interrupt service routine.
> 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[RFC net-next v4 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>
---
v4:
- Change read-modify-write behavior as suggested to phy_modify_mmd.
---
 drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
index 7619d6185801..7c82384e5d30 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,54 @@ 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;
+	u16 irq_mask;
+
+	ret = adin_phy_ack_intr(phydev);
+
+	if (ret)
+		return ret;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN;
+	else
+		irq_mask = 0;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+			      ADIN_PHY_SUBSYS_IRQ_MASK,
+			      ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask);
+}
+
+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 +329,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 v4 2/2] net: phy: adin1100: Add interrupt support for link change

[Horatiu Vultur]

The 01/21/2024 20:54, Andre Werner wrote:

Hi Andre,

 
> 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>
> ---
> v4:
> - Change read-modify-write behavior as suggested to phy_modify_mmd.

Usually it is good to keep the change log also from the previous
versions, so it is easier to see what has been changed.

> ---
>  drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
> index 7619d6185801..7c82384e5d30 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,54 @@ 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;
> +       u16 irq_mask;

Please use reverse x-mas notation here.

> +
> +       ret = adin_phy_ack_intr(phydev);
> +

No new line here, between ret and if.

> +       if (ret)
> +               return ret;
> +
> +       if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +               irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN;
> +       else
> +               irq_mask = 0;
> +
> +       return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +                             ADIN_PHY_SUBSYS_IRQ_MASK,
> +                             ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask);
> +}
> +
> +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 +329,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
> 
> 

-- 
/Horatiu

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

[Andre Werner]

Dear Maintainers,

I'm a bit confused about the patch submitting process for net-next.
Do I need to send the patchset again, if the merge window is opened again and
the patchset was previously submitted as RFC?

Best regards.
Andre


On Mon, 22 Jan 2024, Horatiu Vultur wrote:

> The 01/21/2024 20:54, Andre Werner wrote:
>
> Hi Andre,
>
>
>> 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>
>> ---
>> v4:
>> - Change read-modify-write behavior as suggested to phy_modify_mmd.
>
> Usually it is good to keep the change log also from the previous
> versions, so it is easier to see what has been changed.
>
>> ---
>>  drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
>> index 7619d6185801..7c82384e5d30 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,54 @@ 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;
>> +       u16 irq_mask;
>
> Please use reverse x-mas notation here.
>
>> +
>> +       ret = adin_phy_ack_intr(phydev);
>> +
>
> No new line here, between ret and if.
>
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>> +               irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN;
>> +       else
>> +               irq_mask = 0;
>> +
>> +       return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>> +                             ADIN_PHY_SUBSYS_IRQ_MASK,
>> +                             ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask);
>> +}
>> +
>> +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 +329,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
>>
>>
>
> -- 
> /Horatiu
>

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

[Andrew Lunn]

On Mon, Jan 29, 2024 at 01:51:11PM +0100, Andre Werner wrote:
> Dear Maintainers,
> 
> I'm a bit confused about the patch submitting process for net-next.
> Do I need to send the patchset again, if the merge window is opened again and
> the patchset was previously submitted as RFC?

RFC patches should in theory never be merged. So that alone is
sufficient to require a resend. Anything sent during the merge window
is also not eligible for merging.

So please rebase on net-next, collect and add any Acked-by:
Reviewed-by: etc and resend.

	Andrew