[PATCH net v3 0/4] net: dsa: microchip: Fix resource releases in error path

[PATCH net v3 0/4] net: dsa: microchip: Fix resource releases in error path

[Bastien Curutchet (Schneider Electric)]

Hi all,

I worked on adding PTP support for the KSZ8463. While doing so, I ran
into a few bugs in the resource release process that occur when things go
wrong arount IRQ initialization.

This small series fixes those bugs.

The next series, which will add the PTP support, depend on this one.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
Changes in v3:
- PATCH 1 and 3: Fix Fixes tags
- PATCH 3: Move the irq_dispose_mapping() behind the check that verifies that
  the domain is initialized
- Link to v2: https://lore.kernel.org/r/20251106-ksz-fix-v2-0-07188f608873@bootlin.com

Changes in v2:
- Add Fixes tag.
- Split PATCH 1 in two patches as it needed two different Fixes tags
- Add details in commit logs
- Link to v1: https://lore.kernel.org/r/20251031-ksz-fix-v1-0-7e46de999ed1@bootlin.com

---
Bastien Curutchet (Schneider Electric) (4):
      net: dsa: microchip: common: Fix checks on irq_find_mapping()
      net: dsa: microchip: ptp: Fix checks on irq_find_mapping()
      net: dsa: microchip: Ensure a ksz_irq is initialized before freeing it
      net: dsa: microchip: Immediately assing IRQ numbers

 drivers/net/dsa/microchip/ksz_common.c | 23 +++++++++++++----------
 drivers/net/dsa/microchip/ksz_ptp.c    | 17 +++++++++--------
 2 files changed, 22 insertions(+), 18 deletions(-)
---
base-commit: cd2f741f5aec1043b707070e7ea024e646262277
change-id: 20251031-ksz-fix-db345df7635f

Best regards,
-- 
Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

[PATCH net v3 1/4] net: dsa: microchip: common: Fix checks on irq_find_mapping()

[Bastien Curutchet (Schneider Electric)]

irq_find_mapping() returns a positive IRQ number or 0 if no IRQ is found
but it never returns a negative value. However, on each
irq_find_mapping() call, we verify that the returned value isn't
negative.

Fix the irq_find_mapping() checks to enter error paths when 0 is
returned. Return -EINVAL in such cases.

Fixes: c9cd961c0d43 ("net: dsa: microchip: lan937x: add interrupt support for port phy link")
Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index a962055bfdbd8fbfc135b2dec73c222a213985c4..3a4516d32aa5f99109853ed400e64f8f7e2d8016 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2583,8 +2583,8 @@ static int ksz_irq_phy_setup(struct ksz_device *dev)
 
 			irq = irq_find_mapping(dev->ports[port].pirq.domain,
 					       PORT_SRC_PHY_INT);
-			if (irq < 0) {
-				ret = irq;
+			if (!irq) {
+				ret = -EINVAL;
 				goto out;
 			}
 			ds->user_mii_bus->irq[phy] = irq;
@@ -2948,8 +2948,8 @@ static int ksz_pirq_setup(struct ksz_device *dev, u8 p)
 	snprintf(pirq->name, sizeof(pirq->name), "port_irq-%d", p);
 
 	pirq->irq_num = irq_find_mapping(dev->girq.domain, p);
-	if (pirq->irq_num < 0)
-		return pirq->irq_num;
+	if (!pirq->irq_num)
+		return -EINVAL;
 
 	return ksz_irq_common_setup(dev, pirq);
 }

-- 
2.51.1

[PATCH net v3 2/4] net: dsa: microchip: ptp: Fix checks on irq_find_mapping()

[Bastien Curutchet (Schneider Electric)]

irq_find_mapping() returns a positive IRQ number or 0 if no IRQ is found
but it never returns a negative value. However, during the PTP IRQ setup,
we verify that its returned value isn't negative.

Fix the irq_find_mapping() check to enter the error path when 0 is
returned. Return -EINVAL in such case.

Fixes: cc13ab18b201 ("net: dsa: microchip: ptp: enable interrupt for timestamping")
Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 35fc21b1ee48a47daa278573bfe8749c7b42c731..c8bfbe5e2157323ecf29149d1907b77e689aa221 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -1139,8 +1139,8 @@ int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
 		irq_create_mapping(ptpirq->domain, irq);
 
 	ptpirq->irq_num = irq_find_mapping(port->pirq.domain, PORT_SRC_PTP_INT);
-	if (ptpirq->irq_num < 0) {
-		ret = ptpirq->irq_num;
+	if (!ptpirq->irq_num) {
+		ret = -EINVAL;
 		goto out;
 	}
 

-- 
2.51.1

[PATCH net v3 3/4] net: dsa: microchip: Ensure a ksz_irq is initialized before freeing it

[Bastien Curutchet (Schneider Electric)]

Sometimes ksz_irq_free() can be called on uninitialized ksz_irq (for
example when ksz_ptp_irq_setup() fails). It leads to freeing
uninitialized IRQ numbers and/or domains.

Ensure that IRQ numbers or domains aren't null before freeing them.
In our case the IRQ number of an initialized ksz_irq is never 0. Indeed,
it's either the device's IRQ number and we enter the IRQ setup only when
this dev->irq is strictly positive, or a virtual IRQ assigned with
irq_create_mapping() which returns strictly positive IRQ numbers.

Fixes: cc13ab18b201 ("net: dsa: microchip: ptp: enable interrupt for timestamping")
Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
--
Regarding the Fixes tag here, IMO before cc13ab18b201 it was safe to
not check the domain and the IRQ number because I don't see any path
where ksz_irq_free() would be called on a non-initialized ksz_irq
---
 drivers/net/dsa/microchip/ksz_common.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 3a4516d32aa5f99109853ed400e64f8f7e2d8016..c5f8821a3a0ab7b50ddc31cc0a2f28220fe57c84 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2858,14 +2858,17 @@ static void ksz_irq_free(struct ksz_irq *kirq)
 {
 	int irq, virq;
 
-	free_irq(kirq->irq_num, kirq);
+	if (kirq->irq_num)
+		free_irq(kirq->irq_num, kirq);
 
-	for (irq = 0; irq < kirq->nirqs; irq++) {
-		virq = irq_find_mapping(kirq->domain, irq);
-		irq_dispose_mapping(virq);
-	}
+	if (kirq->domain) {
+		for (irq = 0; irq < kirq->nirqs; irq++) {
+			virq = irq_find_mapping(kirq->domain, irq);
+			irq_dispose_mapping(virq);
+		}
 
-	irq_domain_remove(kirq->domain);
+		irq_domain_remove(kirq->domain);
+	}
 }
 
 static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id)

-- 
2.51.1

[PATCH net v3 4/4] net: dsa: microchip: Immediately assing IRQ numbers

[Bastien Curutchet (Schneider Electric)]

The IRQ numbers created through irq_create_mapping() are only assigned
to ptpmsg_irq[n].num at the end of the IRQ setup. So if an error occurs
between their creation and their assignment (for instance during the
request_threaded_irq() step), we enter the error path and fail to
release the newly created virtual IRQs because they aren't yet assigned
to ptpmsg_irq[n].num.

Assign the IRQ number at mapping creation.

Fixes: cc13ab18b201 ("net: dsa: microchip: ptp: enable interrupt for timestamping")
Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index c8bfbe5e2157323ecf29149d1907b77e689aa221..a8ad99c6ee35ff60fb56cc5770520a793c86ff66 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -1102,10 +1102,6 @@ static int ksz_ptp_msg_irq_setup(struct ksz_port *port, u8 n)
 
 	strscpy(ptpmsg_irq->name, name[n]);
 
-	ptpmsg_irq->num = irq_find_mapping(port->ptpirq.domain, n);
-	if (ptpmsg_irq->num < 0)
-		return ptpmsg_irq->num;
-
 	return request_threaded_irq(ptpmsg_irq->num, NULL,
 				    ksz_ptp_msg_thread_fn, IRQF_ONESHOT,
 				    ptpmsg_irq->name, ptpmsg_irq);
@@ -1135,8 +1131,13 @@ int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
 	if (!ptpirq->domain)
 		return -ENOMEM;
 
-	for (irq = 0; irq < ptpirq->nirqs; irq++)
-		irq_create_mapping(ptpirq->domain, irq);
+	for (irq = 0; irq < ptpirq->nirqs; irq++) {
+		port->ptpmsg_irq[irq].num = irq_create_mapping(ptpirq->domain, irq);
+		if (!port->ptpmsg_irq[irq].num) {
+			ret = -EINVAL;
+			goto out;
+		}
+	}
 
 	ptpirq->irq_num = irq_find_mapping(port->pirq.domain, PORT_SRC_PTP_INT);
 	if (!ptpirq->irq_num) {

-- 
2.51.1

Re: [PATCH net v3 1/4] net: dsa: microchip: common: Fix checks on irq_find_mapping()

[Andrew Lunn]

On Fri, Nov 14, 2025 at 08:20:20AM +0100, Bastien Curutchet (Schneider Electric) wrote:
> irq_find_mapping() returns a positive IRQ number or 0 if no IRQ is found
> but it never returns a negative value. However, on each
> irq_find_mapping() call, we verify that the returned value isn't
> negative.
> 
> Fix the irq_find_mapping() checks to enter error paths when 0 is
> returned. Return -EINVAL in such cases.
> 
> Fixes: c9cd961c0d43 ("net: dsa: microchip: lan937x: add interrupt support for port phy link")
> Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

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

    Andrew

Re: [PATCH net v3 2/4] net: dsa: microchip: ptp: Fix checks on irq_find_mapping()

[Andrew Lunn]

On Fri, Nov 14, 2025 at 08:20:21AM +0100, Bastien Curutchet (Schneider Electric) wrote:
> irq_find_mapping() returns a positive IRQ number or 0 if no IRQ is found
> but it never returns a negative value. However, during the PTP IRQ setup,
> we verify that its returned value isn't negative.
> 
> Fix the irq_find_mapping() check to enter the error path when 0 is
> returned. Return -EINVAL in such case.
> 
> Fixes: cc13ab18b201 ("net: dsa: microchip: ptp: enable interrupt for timestamping")
> Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

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

    Andrew

Re: [PATCH net v3 3/4] net: dsa: microchip: Ensure a ksz_irq is initialized before freeing it

[Andrew Lunn]

On Fri, Nov 14, 2025 at 08:20:22AM +0100, Bastien Curutchet (Schneider Electric) wrote:
> Sometimes ksz_irq_free() can be called on uninitialized ksz_irq (for
> example when ksz_ptp_irq_setup() fails). It leads to freeing
> uninitialized IRQ numbers and/or domains.
> 
> Ensure that IRQ numbers or domains aren't null before freeing them.
> In our case the IRQ number of an initialized ksz_irq is never 0. Indeed,
> it's either the device's IRQ number and we enter the IRQ setup only when
> this dev->irq is strictly positive, or a virtual IRQ assigned with
> irq_create_mapping() which returns strictly positive IRQ numbers.
> 
> Fixes: cc13ab18b201 ("net: dsa: microchip: ptp: enable interrupt for timestamping")
> Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
> --
> Regarding the Fixes tag here, IMO before cc13ab18b201 it was safe to
> not check the domain and the IRQ number because I don't see any path
> where ksz_irq_free() would be called on a non-initialized ksz_irq

I would say the caller is wrong, not ksz_irq_free().

Functions like this come in pairs: ksz_irq_setup() & ksz_irq_free().
_free() should not be called if _setup() was not successful.

Please take a look if you can fix the caller. If the change is big,
maybe we need this as a minimal fix for net, but make the bigger
change in net-next?

Thanks
	Andrew

Re: [PATCH net v3 4/4] net: dsa: microchip: Immediately assing IRQ numbers

[Andrew Lunn]

On Fri, Nov 14, 2025 at 08:20:23AM +0100, Bastien Curutchet (Schneider Electric) wrote:
> The IRQ numbers created through irq_create_mapping() are only assigned
> to ptpmsg_irq[n].num at the end of the IRQ setup. So if an error occurs
> between their creation and their assignment (for instance during the
> request_threaded_irq() step), we enter the error path and fail to
> release the newly created virtual IRQs because they aren't yet assigned
> to ptpmsg_irq[n].num.
> 
> Assign the IRQ number at mapping creation.
> 
> Fixes: cc13ab18b201 ("net: dsa: microchip: ptp: enable interrupt for timestamping")
> Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
> ---
>  drivers/net/dsa/microchip/ksz_ptp.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index c8bfbe5e2157323ecf29149d1907b77e689aa221..a8ad99c6ee35ff60fb56cc5770520a793c86ff66 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -1102,10 +1102,6 @@ static int ksz_ptp_msg_irq_setup(struct ksz_port *port, u8 n)
>  
>  	strscpy(ptpmsg_irq->name, name[n]);
>  
> -	ptpmsg_irq->num = irq_find_mapping(port->ptpirq.domain, n);
> -	if (ptpmsg_irq->num < 0)
> -		return ptpmsg_irq->num;
> -
>  	return request_threaded_irq(ptpmsg_irq->num, NULL,
>  				    ksz_ptp_msg_thread_fn, IRQF_ONESHOT,
>  				    ptpmsg_irq->name, ptpmsg_irq);

static void ksz_ptp_msg_irq_free(struct ksz_port *port, u8 n)
{
	struct ksz_ptp_irq *ptpmsg_irq;

	ptpmsg_irq = &port->ptpmsg_irq[n];

	free_irq(ptpmsg_irq->num, ptpmsg_irq);
	irq_dispose_mapping(ptpmsg_irq->num);
}

This is supposed to be the opposite of ksz_ptp_msg_irq_setup()? The
opposite of irq_dispose_mapping() is irq_create_mapping()? But that
does not happen in ksz_ptp_msg_irq_setup()? 

Maybe this change is enough to fix the issue, but it seems like there
is more asymmetry to correct in this code.

	Andrew

Re: [PATCH net v3 3/4] net: dsa: microchip: Ensure a ksz_irq is initialized before freeing it

[Bastien Curutchet]

Hi Andrew,

On 11/14/25 4:00 PM, Andrew Lunn wrote:
> On Fri, Nov 14, 2025 at 08:20:22AM +0100, Bastien Curutchet (Schneider Electric) wrote:
>> Sometimes ksz_irq_free() can be called on uninitialized ksz_irq (for
>> example when ksz_ptp_irq_setup() fails). It leads to freeing
>> uninitialized IRQ numbers and/or domains.
>>
>> Ensure that IRQ numbers or domains aren't null before freeing them.
>> In our case the IRQ number of an initialized ksz_irq is never 0. Indeed,
>> it's either the device's IRQ number and we enter the IRQ setup only when
>> this dev->irq is strictly positive, or a virtual IRQ assigned with
>> irq_create_mapping() which returns strictly positive IRQ numbers.
>>
>> Fixes: cc13ab18b201 ("net: dsa: microchip: ptp: enable interrupt for timestamping")
>> Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
>> --
>> Regarding the Fixes tag here, IMO before cc13ab18b201 it was safe to
>> not check the domain and the IRQ number because I don't see any path
>> where ksz_irq_free() would be called on a non-initialized ksz_irq
> 
> I would say the caller is wrong, not ksz_irq_free().
> 
> Functions like this come in pairs: ksz_irq_setup() & ksz_irq_free().
> _free() should not be called if _setup() was not successful.
> 
> Please take a look if you can fix the caller. If the change is big,
> maybe we need this as a minimal fix for net, but make the bigger
> change in net-next?

Ok, I'll look at it, I think the change won't be that big.

Best regards,
Bastien