[PATCH net v4 0/5] net: dsa: microchip: Fix resource releases in error path

[PATCH net v4 0/5] 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 v4:
- PATCH 1 & 2: Add Andrew's Reviewed-By.
- PATCH 3: Ensure ksz_irq is initialized outside of ksz_irq_free()
- Add PATCH 4
- PATCH 5: Fix symetry issues in ksz_ptp_msg_irq_{setup/free}()
- Link to v3: https://lore.kernel.org/r/20251114-ksz-fix-v3-0-acbb3b9cc32f@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) (5):
      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: Free previously initialized ports on init failures
      net: dsa: microchip: Fix symetry in ksz_ptp_msg_irq_{setup/free}()

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

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

[PATCH net v4 1/5] 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")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
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 2251ab23859b93a4f8af30a7d9c209ca64121408..c724f5af5d98bf3ff784e36393dd5b3fa7b37c13 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2584,8 +2584,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;
@@ -2949,8 +2949,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 v4 2/5] 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")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
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 v4 3/5] net: dsa: microchip: Ensure a ksz_irq is initialized before freeing it

[Bastien Curutchet (Schneider Electric)]

If something goes wrong at setup, 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 the ksz_irq is initialized before calling ksz_irq_free().

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index c724f5af5d98bf3ff784e36393dd5b3fa7b37c13..a622416d966330187ee062b2f44051ddf4ce2a78 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3078,9 +3078,12 @@ static int ksz_setup(struct dsa_switch *ds)
 		dsa_switch_for_each_user_port(dp, dev->ds)
 			ksz_ptp_irq_free(ds, dp->index);
 out_pirq:
-	if (dev->irq > 0)
-		dsa_switch_for_each_user_port(dp, dev->ds)
-			ksz_irq_free(&dev->ports[dp->index].pirq);
+	if (dev->irq > 0) {
+		dsa_switch_for_each_user_port(dp, dev->ds) {
+			if (dev->ports[dp->index].pirq.domain)
+				ksz_irq_free(&dev->ports[dp->index].pirq);
+		}
+	}
 out_girq:
 	if (dev->irq > 0)
 		ksz_irq_free(&dev->girq);

-- 
2.51.1

[PATCH net v4 4/5] net: dsa: microchip: Free previously initialized ports on init failures

[Bastien Curutchet (Schneider Electric)]

If ksz_pirq_setup() fails after at least one successful port
initialization, the goto jumps directly to the global irq freeing,
leaking the resources of the previously initialized ports.

Fix the goto jump to release all the potentially initialized ports.
Remove the no-longer used out_girq label.

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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index a622416d966330187ee062b2f44051ddf4ce2a78..2b6f7abea00776fafff0c1774cab297a7ef261da 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3035,7 +3035,7 @@ static int ksz_setup(struct dsa_switch *ds)
 		dsa_switch_for_each_user_port(dp, dev->ds) {
 			ret = ksz_pirq_setup(dev, dp->index);
 			if (ret)
-				goto out_girq;
+				goto out_pirq;
 
 			if (dev->info->ptp_capable) {
 				ret = ksz_ptp_irq_setup(ds, dp->index);
@@ -3083,10 +3083,8 @@ static int ksz_setup(struct dsa_switch *ds)
 			if (dev->ports[dp->index].pirq.domain)
 				ksz_irq_free(&dev->ports[dp->index].pirq);
 		}
-	}
-out_girq:
-	if (dev->irq > 0)
 		ksz_irq_free(&dev->girq);
+	}
 
 	return ret;
 }

-- 
2.51.1

Re: [PATCH net v4 4/5] net: dsa: microchip: Free previously initialized ports on init failures

[Maxime Chevallier]

Hi Bastien,

On 17/11/2025 14:05, Bastien Curutchet (Schneider Electric) wrote:
> If ksz_pirq_setup() fails after at least one successful port
> initialization, the goto jumps directly to the global irq freeing,
> leaking the resources of the previously initialized ports.
> 
> Fix the goto jump to release all the potentially initialized ports.
> Remove the no-longer used out_girq label.
> 
> 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 | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index a622416d966330187ee062b2f44051ddf4ce2a78..2b6f7abea00776fafff0c1774cab297a7ef261da 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -3035,7 +3035,7 @@ static int ksz_setup(struct dsa_switch *ds)
>  		dsa_switch_for_each_user_port(dp, dev->ds) {
>  			ret = ksz_pirq_setup(dev, dp->index);
>  			if (ret)
> -				goto out_girq;
> +				goto out_pirq;
>  
>  			if (dev->info->ptp_capable) {
>  				ret = ksz_ptp_irq_setup(ds, dp->index);
> @@ -3083,10 +3083,8 @@ static int ksz_setup(struct dsa_switch *ds)
>  			if (dev->ports[dp->index].pirq.domain)
>  				ksz_irq_free(&dev->ports[dp->index].pirq);
>  		}
> -	}
> -out_girq:
> -	if (dev->irq > 0)
>  		ksz_irq_free(&dev->girq);
> +	}
>  
>  	return ret;
>  }
> 

Looking at the code, I think it's still not enough, but feel free
to correct me.

In ksz_setup(), in one single loop we do :

dsa_switch_for_each_user_port(dp, dev->ds) {
	ksz_pirq_setup();

	ksz_ptp_irq_setup();
}

However when anything fails in the above loop, we jump straight
to out_pirq, which doesn't clean the ptpirq :(

Maxime

Re: [PATCH net v4 4/5] net: dsa: microchip: Free previously initialized ports on init failures

[Bastien Curutchet]

Hi Maxime,

On 11/17/25 5:28 PM, Maxime Chevallier wrote:
> Hi Bastien,
> 
> On 17/11/2025 14:05, Bastien Curutchet (Schneider Electric) wrote:
>> If ksz_pirq_setup() fails after at least one successful port
>> initialization, the goto jumps directly to the global irq freeing,
>> leaking the resources of the previously initialized ports.
>>
>> Fix the goto jump to release all the potentially initialized ports.
>> Remove the no-longer used out_girq label.
>>
>> 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 | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
>> index a622416d966330187ee062b2f44051ddf4ce2a78..2b6f7abea00776fafff0c1774cab297a7ef261da 100644
>> --- a/drivers/net/dsa/microchip/ksz_common.c
>> +++ b/drivers/net/dsa/microchip/ksz_common.c
>> @@ -3035,7 +3035,7 @@ static int ksz_setup(struct dsa_switch *ds)
>>   		dsa_switch_for_each_user_port(dp, dev->ds) {
>>   			ret = ksz_pirq_setup(dev, dp->index);
>>   			if (ret)
>> -				goto out_girq;
>> +				goto out_pirq;
>>   
>>   			if (dev->info->ptp_capable) {
>>   				ret = ksz_ptp_irq_setup(ds, dp->index);
>> @@ -3083,10 +3083,8 @@ static int ksz_setup(struct dsa_switch *ds)
>>   			if (dev->ports[dp->index].pirq.domain)
>>   				ksz_irq_free(&dev->ports[dp->index].pirq);
>>   		}
>> -	}
>> -out_girq:
>> -	if (dev->irq > 0)
>>   		ksz_irq_free(&dev->girq);
>> +	}
>>   
>>   	return ret;
>>   }
>>
> 
> Looking at the code, I think it's still not enough, but feel free
> to correct me.
> 
> In ksz_setup(), in one single loop we do :
> 
> dsa_switch_for_each_user_port(dp, dev->ds) {
> 	ksz_pirq_setup();
> 
> 	ksz_ptp_irq_setup();
> }
> 
> However when anything fails in the above loop, we jump straight
> to out_pirq, which doesn't clean the ptpirq :(

Good catch, I should also merge together the out_ptpirq and out_pirq 
labels then. And ensure that the PTP IRQ is initialized before freeing it.

I'll respin with that.


Best regards,
Bastien

[PATCH net v4 5/5] net: dsa: microchip: Fix symetry in ksz_ptp_msg_irq_{setup/free}()

[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.

Move the mapping creation to ksz_ptp_msg_irq_setup() to ensure symetry
with what's released by ksz_ptp_msg_irq_free().
In the error path, move the irq_dispose_mapping to the out_ptp_msg label
so it will be called only on created IRQs.

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 | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index c8bfbe5e2157323ecf29149d1907b77e689aa221..997e4a76d0a68448b0ebc76169150687bbc79673 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -1093,19 +1093,19 @@ static int ksz_ptp_msg_irq_setup(struct ksz_port *port, u8 n)
 	static const char * const name[] = {"pdresp-msg", "xdreq-msg",
 					    "sync-msg"};
 	const struct ksz_dev_ops *ops = port->ksz_dev->dev_ops;
+	struct ksz_irq *ptpirq = &port->ptpirq;
 	struct ksz_ptp_irq *ptpmsg_irq;
 
 	ptpmsg_irq = &port->ptpmsg_irq[n];
+	ptpmsg_irq->num = irq_create_mapping(ptpirq->domain, n);
+	if (!ptpmsg_irq->num)
+		return -EINVAL;
 
 	ptpmsg_irq->port = port;
 	ptpmsg_irq->ts_reg = ops->get_port_addr(port->num, ts_reg[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,9 +1135,6 @@ 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);
-
 	ptpirq->irq_num = irq_find_mapping(port->pirq.domain, PORT_SRC_PTP_INT);
 	if (!ptpirq->irq_num) {
 		ret = -EINVAL;
@@ -1159,12 +1156,11 @@ int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
 
 out_ptp_msg:
 	free_irq(ptpirq->irq_num, ptpirq);
-	while (irq--)
+	while (irq--) {
 		free_irq(port->ptpmsg_irq[irq].num, &port->ptpmsg_irq[irq]);
-out:
-	for (irq = 0; irq < ptpirq->nirqs; irq++)
 		irq_dispose_mapping(port->ptpmsg_irq[irq].num);
-
+	}
+out:
 	irq_domain_remove(ptpirq->domain);
 
 	return ret;

-- 
2.51.1