Re: [PATCH net] dm9000: free the wake IRQ before releasing board resources

[netdev-bot+sashiko]

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

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

- [High] The shared wake IRQ handler is live before its MMIO mappings
  exist, and the patch's new free_irq() makes that window…
- [Medium] dm9000_release_board() now frees the wake IRQ action
  (drivers/net/ethernet/davicom/dm9000.c:806-807) but nothing ever…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but dm9000_probe()
  ignores register_netdev() failure: at…
- [Medium] This problem wasn't introduced by this patch, but
  dm9000_probe() treats an -EPROBE_DEFER result for the optional wake…

--

dm9000: free the wake IRQ before releasing board resources

The patch adds a wake_irq_requested bit to struct board_info so that a
successful request_irq() for the optional wake interrupt is tracked
separately from wake_supported, and frees that IRQ from
dm9000_release_board() before the register windows are unmapped.  This
covers both the probe unwind path and driver removal.

> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index b87eaf0c250ce..c8ff07fe31a5b 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c

[ ... ]

> @@ -802,6 +803,9 @@ dm9000_poll_work(struct work_struct *w)
>  static void
>  dm9000_release_board(struct platform_device *pdev, struct board_info *db)
>  {
> +	if (db->wake_irq_requested)
> +		free_irq(db->irq_wake, db->ndev);
> +
>  	/* unmap our resources */
>  
>  	iounmap(db->io_addr);

[Severity: High]
Can this free_irq() call dm9000_wol_interrupt() while db->io_addr and
db->io_data are still NULL?

In dm9000_probe() the wake IRQ is requested well before the register
windows are mapped:

	ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
			  IRQF_SHARED, dev_name(db->dev), ndev);
	...
	db->addr_req = request_mem_region(...);
	db->io_addr = ioremap(db->addr_res->start, iosize);
	...
	db->io_data = ioremap(db->data_res->start, iosize);

db comes from netdev_priv() of a freshly alloc_etherdev()'d device, so
both __iomem pointers are NULL at request time, and the handler starts
with MMIO access:

dm9000_wol_interrupt() {
	nsr = ior(db, DM9000_NSR);
	wcr = ior(db, DM9000_WCR);

ior() {
	writeb(reg, db->io_addr);
	return readb(db->io_data);
}

With CONFIG_DEBUG_SHIRQ, __free_irq() calls the handler once after
unregistering a shared action:

kernel/irq/manage.c:__free_irq() {
	if (action->flags & IRQF_SHARED) {
		local_irq_save(flags);
		action->handler(irq, dev_id);
		local_irq_restore(flags);
	}

So every probe failure that reaches the out: label before or between the
two ioremap() calls (addr_req == NULL, io_addr == NULL, data_req == NULL,
io_data == NULL) now runs dm9000_wol_interrupt() -> ior() ->
writeb(reg, NULL) from the new cleanup.

Independently of the debug config, since the action is IRQF_SHARED, can
the line be asserted by a co-owner (or by a DM9000 whose NSR_WAKEST is
already latched across a warm reboot) during the sleeping
request_mem_region()/ioremap() calls, with the same result?  The same
window also lets the handler's index/data two-cycle register protocol
interleave with the unlocked ior()/iow() sequences in dm9000_reset(), the
VID/PID retry loop and the EEPROM reads, since dm9000_probe() never takes
db->lock.

Would it be preferable to move the request_irq() and the
irq_set_irq_wake() capability test to after the register windows are
mapped, dm9000_set_io() has run and the chip has been reset?  That
ordering also makes the unwind natural and removes the need for the new
wake_irq_requested bit in the shared cleanup helper.

[Severity: Medium]
Does this teardown leak the interrupt wake reference taken by
dm9000_set_wol()?

When userspace enables WAKE_MAGIC, the reference is taken here:

dm9000_set_wol() {
	if (dm->wake_state != opts) {
		/* change in wol state, update IRQ state */

		if (!dm->wake_state)
			irq_set_irq_wake(dm->irq_wake, 1);
		else if (dm->wake_state && !opts)
			irq_set_irq_wake(dm->irq_wake, 0);
	}

That is the only place the reference is dropped, and only on a
wake_state 1 -> 0 transition driven by ethtool.  dm9000_stop(),
dm9000_shutdown(), dm9000_release_board() and dm9000_drv_remove() do not
clear db->wake_state and do not call irq_set_irq_wake(db->irq_wake, 0).

desc->wake_depth and IRQD_WAKEUP_STATE are maintained only by
irq_set_irq_wake(), and __free_irq() does not touch them, so after
	ethtool -s ethX wol g
	echo ... > unbind   (or rmmod)
the descriptor keeps wake_depth > 0 and IRQD_WAKEUP_STATE set with no
handler attached.  Because the line was requested IRQF_SHARED, a co-owner
that later does its own enable/disable pair only brings wake_depth back
to 1, so set_irq_wake_real(irq, 0) is never reached and the co-owner can
no longer turn the wake source off.

Should the new cleanup drop that reference, for example
irq_set_irq_wake(db->irq_wake, 0) when db->wake_state is set, before the
free_irq()?

> @@ -1519,6 +1523,7 @@ dm9000_probe(struct platform_device *pdev)
>  		if (ret) {
>  			dev_err(db->dev, "cannot get wakeup irq (%d)\n", ret);
>  		} else {
> +			db->wake_irq_requested = 1;
>  
>  			/* test to see if irq is really wakeup capable */
>  			ret = irq_set_irq_wake(db->irq_wake, 1);

[Severity: High]
This isn't a bug introduced by this patch, but since the patch extends the
out: unwind in dm9000_probe(), is the registration failure path reaching it
at all?  Near the end of dm9000_probe():

	platform_set_drvdata(pdev, ndev);
	ret = register_netdev(ndev);

	if (ret == 0) {
		...
	}
	return 0;

The return value is only used to gate the success printk and then 0 is
returned unconditionally, so a failed register_netdev() reports probe
success.  The driver core records a successful bind, the out: unwind that
now calls free_irq()/dm9000_release_board() is never taken, and the board
keeps its mapped registers plus the newly tracked wake IRQ attached to an
unregistered net_device.

Removal then calls unregister_netdev() on that device:

net/core/dev.c:unregister_netdevice_many_notify() {
		if (dev->reg_state == NETREG_UNINITIALIZED) {
			...
			WARN_ON(1);
			list_del(&dev->unreg_list);
			continue;
		}
		dev->dismantle = true;
		BUG_ON(dev->reg_state != NETREG_REGISTERED);

In the common case register_netdevice() has rolled back to
NETREG_UNINITIALIZED and this is the WARN_ON(1).  If registration instead
failed via the NETDEV_REGISTER notifier path, which internally
unregisters the device and clears needs_free_netdev, reg_state is no
longer NETREG_UNINITIALIZED and the later unregister_netdev() reaches the
BUG_ON(dev->reg_state != NETREG_REGISTERED).

Would "if (ret) goto out;" be the intended control flow here?

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but a few
lines above this hunk dm9000_probe() folds -EPROBE_DEFER into "the
optional interrupt is absent":

	db->irq_wake = platform_get_irq_optional(pdev, 1);
	if (db->irq_wake >= 0) {

platform_get_irq_optional() forwards to platform_get_irq_affinity(),
which explicitly propagates the deferral:

drivers/base/platform.c:platform_get_irq_affinity() {
		ret = of_irq_get(to_of_node(fwnode), num);
		if (ret > 0 || ret == -EPROBE_DEFER)
			goto out;

So when the second interrupt is described in DT but its irqchip has not
probed yet, probe silently continues and returns success, wake_supported
stays 0, and ethtool set_wol returns -EOPNOTSUPP for the lifetime of the
binding with no retry.  Should -EPROBE_DEFER be propagated out of
dm9000_probe() instead?

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