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

[PATCH net v2 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 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 | 14 ++++++++------
 drivers/net/dsa/microchip/ksz_ptp.c    | 17 +++++++++--------
 2 files changed, 17 insertions(+), 14 deletions(-)
---
base-commit: cd2f741f5aec1043b707070e7ea024e646262277
change-id: 20251031-ksz-fix-db345df7635f

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

[PATCH net v2 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: ff319a644829 ("net: dsa: microchip: move interrupt handling logic from lan937x to ksz_common")
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.0

[PATCH net v2 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.0

[PATCH net v2 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: e1add7dd6183 ("net: dsa: microchip: use common irq routines for girq and pirq")
Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 3a4516d32aa5f99109853ed400e64f8f7e2d8016..4f5e2024442692adefc69d47e82381a3c3bda184 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2858,14 +2858,16 @@ 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);
 	}
 
-	irq_domain_remove(kirq->domain);
+	if (kirq->domain)
+		irq_domain_remove(kirq->domain);
 }
 
 static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id)

-- 
2.51.0

[PATCH net v2 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.0

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

[Jakub Kicinski]

On Thu, 06 Nov 2025 13:53:08 +0100 Bastien Curutchet (Schneider
Electric) wrote:
> Fixes: ff319a644829 ("net: dsa: microchip: move interrupt handling logic from lan937x to ksz_common")

This commit just moves the buggy code around, the fixes tag should
point at the commit which introduced the buggy code (first commit
in which the bug could be reproduced). I think other commits suffer
from a similar issue. Please look a little deeper into the history.
-- 
pw-bot:  cr

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

[Jakub Kicinski]

On Thu, 06 Nov 2025 13:53:10 +0100 Bastien Curutchet (Schneider
Electric) wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 3a4516d32aa5f99109853ed400e64f8f7e2d8016..4f5e2024442692adefc69d47e82381a3c3bda184 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2858,14 +2858,16 @@ 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++) {

if the domain may not be registered is it okay to try to find mappings
in it? From the init path it seems that kirq->nirqs is set to the port
count before registration so it will not be 0 if domain is NULL.

>  		virq = irq_find_mapping(kirq->domain, irq);
>  		irq_dispose_mapping(virq);
>  	}
>  
> -	irq_domain_remove(kirq->domain);
> +	if (kirq->domain)
> +		irq_domain_remove(kirq->domain);

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

[Bastien Curutchet]

Hi Jakub,

On 11/11/25 3:28 AM, Jakub Kicinski wrote:
> On Thu, 06 Nov 2025 13:53:10 +0100 Bastien Curutchet (Schneider
> Electric) wrote:
>> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
>> index 3a4516d32aa5f99109853ed400e64f8f7e2d8016..4f5e2024442692adefc69d47e82381a3c3bda184 100644
>> --- a/drivers/net/dsa/microchip/ksz_common.c
>> +++ b/drivers/net/dsa/microchip/ksz_common.c
>> @@ -2858,14 +2858,16 @@ 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++) {
> 
> if the domain may not be registered is it okay to try to find mappings
> in it? From the init path it seems that kirq->nirqs is set to the port
> count before registration so it will not be 0 if domain is NULL.
> 

I first thought it was fine because __irq_resolve_mapping() inside 
irq_find_mapping() verifies that the domain isn't NULL, then 
irq_find_mapping() returns 0 when it doesn't find an IRQ, and finally,
irq_dispose_mapping() returns immediately when virq is 0.

However, after taking a closer look at __irq_resolve_mapping(), I 
noticed that there is a global irq_default_domain that is used by some 
architectures (mainly MIPS and PowerPC) as a fallback when 
__irq_resolve_mapping() is given a NULL domain. In that case, it seems 
possible for the function to return a non-zero virq that has nothing to 
do with us, and which would then be incorrectly released afterward by 
irq_dispose_mapping().

This second case shouldn't occur often but I can move the for loop 
behind the `if (kirq->domain)` check to be safe.


>>   		virq = irq_find_mapping(kirq->domain, irq);
>>   		irq_dispose_mapping(virq);
>>   	}
>>   
>> -	irq_domain_remove(kirq->domain);
>> +	if (kirq->domain)
>> +		irq_domain_remove(kirq->domain);

Best regards,
Bastien

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

[Bastien Curutchet]

Hi Jakub,

On 11/11/25 3:27 AM, Jakub Kicinski wrote:
> On Thu, 06 Nov 2025 13:53:08 +0100 Bastien Curutchet (Schneider
> Electric) wrote:
>> Fixes: ff319a644829 ("net: dsa: microchip: move interrupt handling logic from lan937x to ksz_common")
> 
> This commit just moves the buggy code around, the fixes tag should
> point at the commit which introduced the buggy code (first commit
> in which the bug could be reproduced). I think other commits suffer
> from a similar issue. Please look a little deeper into the history.

Sorry about this, I'll take a closer look to find the relevant buggy commit.

Best regards,
Bastien