[PATCH net] net: ti: icss-iep: Cancel compare work before releasing device resources

[PATCH net] net: ti: icss-iep: Cancel compare work before releasing device resources

[Myeonghun Pak]

The compare interrupt can queue work that accesses the IEP state and
registers. The managed IRQ is released when the driver is detached, but
pending or running work is not drained before the IEP resources are freed.

Initialize the work with devm_work_autocancel() after its resources are
ready and before requesting the IRQ. This makes devres release the IRQ,
cancel the work synchronously, and then release the resources used by the
handler and worker. Moving the IRQ request also ensures the handler cannot
run before the platform data and register mapping are initialized.

This issue was identified during our ongoing static-analysis research
while reviewing kernel code.

Fixes: f18ad402cd8b ("net: ti: icss-iep: Enable compare events")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Validated with static source review, apply checks and strict checkpatch.
No build or runtime testing was performed.

 drivers/net/ethernet/ti/icssg/icss_iep.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index ec085897edf090e816b05880286a3f43c69225c8..fa45538f1faa6c8b44e7dd9a4ebb7269843b60bb 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -8,6 +8,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/devm-helpers.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -857,18 +858,6 @@ static int icss_iep_probe(struct platform_device *pdev)
 	if (irq == -EPROBE_DEFER)
 		return irq;
 
-	if (irq > 0) {
-		ret = devm_request_irq(dev, irq, icss_iep_cap_cmp_irq,
-				       IRQF_TRIGGER_HIGH, "iep_cap_cmp", iep);
-		if (ret) {
-			dev_info(iep->dev, "cap_cmp irq request failed: %x\n",
-				 ret);
-		} else {
-			iep->cap_cmp_irq = irq;
-			INIT_WORK(&iep->work, icss_iep_cap_cmp_work);
-		}
-	}
-
 	iep_clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(iep_clk))
 		return PTR_ERR(iep_clk);
@@ -895,9 +884,24 @@ static int icss_iep_probe(struct platform_device *pdev)
 
 	iep->ptp_info = icss_iep_ptp_info;
 	mutex_init(&iep->ptp_clk_mutex);
-	dev_set_drvdata(dev, iep);
 	icss_iep_disable(iep);
 
+	if (irq > 0) {
+		ret = devm_work_autocancel(dev, &iep->work, icss_iep_cap_cmp_work);
+		if (ret)
+			return ret;
+
+		ret = devm_request_irq(dev, irq, icss_iep_cap_cmp_irq,
+				       IRQF_TRIGGER_HIGH, "iep_cap_cmp", iep);
+		if (ret)
+			dev_info(iep->dev, "cap_cmp irq request failed: %x\n",
+				 ret);
+		else
+			iep->cap_cmp_irq = irq;
+	}
+
+	dev_set_drvdata(dev, iep);
+
 	return 0;
 }
 
-- 
2.47.1

Re: [PATCH net] net: ti: icss-iep: Cancel compare work before releasing device resources

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the compare work is still not drained before the
  PTP clock it uses is torn down.

Pre-existing issues:
- [High] Deadlock: `icss_iep_pps_enable()`
  (drivers/net/ethernet/ti/icssg/icss_iep.c:579-618) calls…
- [High] Fix scope is narrower than the commit message implies: detach of
  the icss-iep device remains fundamentally unsafe.

--

net: ti: icss-iep: Cancel compare work before releasing device resources

The patch moves the iep_cap_cmp IRQ request in icss_iep_probe() to after the
platform data and regmap are set up, and switches the compare work from
INIT_WORK() to devm_work_autocancel() so that devres frees the IRQ, then
cancels the work, before the rest of the devm resources are released.

No Link: tags in the commit header.

> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index ec085897edf09..fa45538f1faa6 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c

[ ... ]

> @@ -895,9 +884,24 @@ static int icss_iep_probe(struct platform_device *pdev)
>  
>  	iep->ptp_info = icss_iep_ptp_info;
>  	mutex_init(&iep->ptp_clk_mutex);
> -	dev_set_drvdata(dev, iep);
>  	icss_iep_disable(iep);
>  
> +	if (irq > 0) {
> +		ret = devm_work_autocancel(dev, &iep->work, icss_iep_cap_cmp_work);
> +		if (ret)
> +			return ret;

[Severity: High]

Does this cover the teardown path that actually releases the resource the
worker touches?  devm_work_autocancel() only drains the work when devres
runs, i.e. at icss-iep driver detach.  The resource the worker dereferences,
iep->ptp_clock, is released much earlier by icss_iep_exit(), which the
consumer calls on interface stop while the icss-iep device stays bound:

drivers/net/ethernet/ti/icssg/icssg_prueth.c:prueth_emac_common_stop() {
	...
	icss_iep_exit(emac->iep);
	...
}

icss_iep_exit() drops the clock first and only then looks at the enable
state:

int icss_iep_exit(struct icss_iep *iep)
{
	if (iep->ptp_clock) {
		ptp_clock_unregister(iep->ptp_clock);
		iep->ptp_clock = NULL;
	}
	icss_iep_disable(iep);

	if (iep->pps_enabled)
		icss_iep_pps_enable(iep, false);
	else if (iep->perout_enabled)
		icss_iep_perout_enable(iep, NULL, false);

	return 0;
}

icss_iep_perout_enable() has no cancel_work_sync(), so on the perout branch
the work is never drained here at all.

Meanwhile icss_iep_cap_cmp_irq() keeps queueing while either flag is set:

	if (iep->pps_enabled || iep->perout_enabled)
		schedule_work(&iep->work);

and icss_iep_cap_cmp_work() uses the pointer unconditionally:

	pevent.pps_times.ts_real = ns_to_timespec64(ns);
	pevent.type = PTP_CLOCK_PPSUSR;
	pevent.index = 0;
	ptp_clock_event(iep->ptp_clock, &pevent);

For PTP_CLOCK_PPSUSR, ptp_clock_event() reads ptp->pps_source and hands it
to pps_event(), which dereferences it without a NULL check.

So with PEROUT enabled on /dev/ptpN and a CMP1 interrupt already having
queued the work, can an ifdown reach ptp_clock_event() with iep->ptp_clock
already NULL (or pointing at the just-unregistered clock)?  Would stopping
event production, synchronizing the IRQ and draining the work before
ptp_clock_unregister() in icss_iep_exit() be the placement that closes the
window described in the commit message?

> +
> +		ret = devm_request_irq(dev, irq, icss_iep_cap_cmp_irq,
> +				       IRQF_TRIGGER_HIGH, "iep_cap_cmp", iep);
> +		if (ret)
> +			dev_info(iep->dev, "cap_cmp irq request failed: %x\n",
> +				 ret);
> +		else
> +			iep->cap_cmp_irq = irq;
> +	}
> +
> +	dev_set_drvdata(dev, iep);
> +
>  	return 0;
>  }

[Severity: High]

This is a pre-existing issue and not introduced by this patch, but since the
patch adds a second cancel site for the same work, it may be worth looking
at the existing one in icss_iep_pps_enable().

The disable branch calls cancel_work_sync() with ptp_clk_mutex still held,
as the mutex is only dropped at the exit label:

static int icss_iep_pps_enable(struct icss_iep *iep, int on)
{
	...
	mutex_lock(&iep->ptp_clk_mutex);
	...
	} else {
		ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
		if (iep->cap_cmp_irq)
			cancel_work_sync(&iep->work);
	}
	...
exit:
	mutex_unlock(&iep->ptp_clk_mutex);

and the work handler takes the same mutex as its first action:

static void icss_iep_cap_cmp_work(struct work_struct *work)
{
	...
	mutex_lock(&iep->ptp_clk_mutex);

If the work was already queued by an earlier CMP1 interrupt and is blocked
on mutex_lock(&iep->ptp_clk_mutex), can cancel_work_sync() then wait
forever for a worker that is waiting for the lock the canceller holds?

Disabling the compare hardware just before the cancel narrows the window,
but does it close it for a work item that is already queued or running?

[Severity: High]

This is also a pre-existing gap rather than something this patch introduces,
but the commit message frames the change as making driver detach safe, so
the scope may be worth clarifying for stable backporters.

icss_iep_driver has no .remove callback:

static struct platform_driver icss_iep_driver = {
	.driver = {
		.name = "icss-iep",
		.of_match_table = icss_iep_of_match,
	},
	.probe = icss_iep_probe,
};

and icss_iep_get_idx() hands the devm-allocated iep out to the consumer
while only taking a device reference:

	pdev = of_find_device_by_node(iep_np);
	...
	iep = platform_get_drvdata(pdev);
	...
	iep->client_np = np;

Nothing there pins the icss-iep driver, so on a sysfs unbind while prueth is
still bound, devres frees iep and unmaps iep->base while the PTP clock
registered from &iep->ptp_info in icss_iep_init() is still registered, and
while prueth still calls icss_iep_exit()/icss_iep_get_count_*() on that
pointer.  After unbind, does a PHC ioctl such as icss_iep_ptp_gettimeex()
still reach container_of(ptp_info) on freed memory and readl() on the
unmapped window?

Should the commit message say that the change drains the work item only, and
that unbind of the icss-iep device remains unsafe for other reasons?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917193847.63849-1-mhun512%40gmail.com