Re: [PATCH net-next v2] net: davicom: Propagate wakeup IRQ errors

[netdev-bot+sashiko]

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

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

- [High] dm9000_probe() now aborts the whole probe when the *optional*
  wakeup-IRQ setup fails.
- [High] The newly added 'goto out' after a failing
  irq_set_irq_wake(db->irq_wake, 1) is taken while…

Pre-existing issues:
- [High] The wakeup IRQ requested in dm9000_probe() is never released on
  any path other than the newly added one either: the pre-existing later…
- [High] In dm9000_probe() the shared wakeup IRQ is requested (line 1521,
  IRQF_SHARED, handler dm9000_wol_interrupt, dev_id = ndev) before…
- [Low] Documentation/devicetree/bindings/net/davicom,dm9000.yaml
  restricts 'interrupts' to 'maxItems: 1' (and defines no interrupt-…

--

net: davicom: Propagate wakeup IRQ errors

This changes dm9000_probe() so that an error from
platform_get_irq_optional(pdev, 1) is returned to the caller unless it is
-ENXIO, and so that failures of request_irq() and irq_set_irq_wake() for
the optional wakeup IRQ abort the probe instead of only being logged.

A few questions about the new error paths below.

> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index b87eaf0c250c..904b859f0a63 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
> @@ -1511,13 +1511,18 @@ dm9000_probe(struct platform_device *pdev)
>  	}
>  
>  	db->irq_wake = platform_get_irq_optional(pdev, 1);
> -	if (db->irq_wake >= 0) {
> +	if (db->irq_wake < 0 && db->irq_wake != -ENXIO) {
> +		ret = db->irq_wake;
> +		goto out;
> +	}
> +	if (db->irq_wake > 0) {

[Severity: Low]
This is a pre-existing inconsistency and not something introduced here, but
is the wakeup IRQ index reachable at all from device tree?

Documentation/devicetree/bindings/net/davicom,dm9000.yaml declares:

  interrupts:
    maxItems: 1

and defines no interrupt-names.  For a node that validates against that
schema, index 1 does not exist, so of_irq_get() fails and
platform_get_irq_affinity() falls back to platform_get_resource(), which
returns NULL and produces -ENXIO.  That is exactly the value the new check
tolerates, so on schema-compliant device trees the whole wakeup block that
this patch hardens is skipped.

Should the binding gain a second interrupt entry (with interrupt-names) so
the driver and the documented contract agree on how many interrupts a
davicom,dm9000 node may declare?

>  		dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
>  
>  		ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
>  				  IRQF_SHARED, dev_name(db->dev), ndev);
>  		if (ret) {
>  			dev_err(db->dev, "cannot get wakeup irq (%d)\n", ret);
> +			goto out;

[Severity: High]
Is it intended that a failure to set up an optional feature now takes down
the whole netdev?

request_irq() here uses IRQF_SHARED, so it can legitimately return -EBUSY
when a non-shared handler already owns that line.  Before this change the
failure was logged, db->wake_supported stayed 0, and the interface
registered normally; dm9000_set_wol() then simply refused WoL:

	if (!dm->wake_supported)
		return -EOPNOTSUPP;

With the new goto out, register_netdev() is never reached and such a board
loses Ethernet entirely.  Would it be better to keep the graceful
degradation for the two setup failures and only propagate the
platform_get_irq_optional() error (which is where -EPROBE_DEFER matters)?

[Severity: High]
This ordering isn't introduced by this patch, but can the shared wake
handler run before the register window exists?

request_irq() installs dm9000_wol_interrupt() here, while db->io_addr and
db->io_data are only filled in later in dm9000_probe():

	db->io_addr = ioremap(db->addr_res->start, iosize);
	...
	db->io_data = ioremap(db->data_res->start, iosize);

db comes from alloc_etherdev()/netdev_priv() and is zeroed, so both
pointers are NULL in that window.  The handler starts with MMIO
immediately:

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

and ior() has no guard:

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

Since the line is shared, a co-tenant device (or an already-asserted
level-triggered line at request time) can invoke this and dereference NULL
in hard-IRQ context.  Would moving the wake-IRQ request to after the
ioremap()/dm9000_set_io() sequence close this, and at the same time make
the new error paths easier to unwind?

>  		} else {
>  
>  			/* test to see if irq is really wakeup capable */
> @@ -1525,6 +1530,7 @@ dm9000_probe(struct platform_device *pdev)
>  			if (ret) {
>  				dev_err(db->dev, "irq %d cannot set wakeup (%d)\n",
>  					db->irq_wake, ret);
> +				goto out;

[Severity: High]
The comment right above this call describes the operation as a capability
test:

	/* test to see if irq is really wakeup capable */
	ret = irq_set_irq_wake(db->irq_wake, 1);

Is a non-zero return really an error condition here?  set_irq_wake_real()
in kernel/irq/manage.c starts with:

	int ret = -ENXIO;

	if (irq_desc_get_chip(desc)->flags &  IRQCHIP_SKIP_SET_WAKE)
		return 0;

	if (desc->irq_data.chip->irq_set_wake)
		ret = desc->irq_data.chip->irq_set_wake(&desc->irq_data, on);

so -ENXIO is the normal result for any irqchip that implements no
.irq_set_wake and is not flagged IRQCHIP_SKIP_SET_WAKE, which covers many
simple platform, SoC and GPIO controllers.  On such a board the probe now
fails where it previously succeeded with WoL disabled.

[Severity: High]
Can this goto out leave the wake handler registered on a freed net_device?

At this point request_irq(db->irq_wake, dm9000_wol_interrupt, IRQF_SHARED,
dev_name(db->dev), ndev) has already succeeded, but the unwind path is:

out:
	dev_err(db->dev, "not found (%d).\n", ret);

	dm9000_release_board(pdev, db);
	free_netdev(ndev);

and dm9000_release_board() does no IRQ teardown at all:

	iounmap(db->io_addr);
	iounmap(db->io_data);

	if (db->data_req)
		release_resource(db->data_req);
	kfree(db->data_req);
	...

The only free_irq() in the driver is free_irq(ndev->irq, ndev) in
dm9000_stop(), so nothing releases db->irq_wake.  After free_netdev(ndev)
the shared handler is still installed with dev_id pointing at the freed
net_device, and any assertion of that line runs:

	struct net_device *dev = dev_id;
	struct board_info *db = netdev_priv(dev);
	...
	spin_lock_irqsave(&db->lock, flags);

	nsr = ior(db, DM9000_NSR);

which touches freed memory and does MMIO through the already-iounmapped
bases in hard-IRQ context.  Should this path get its own label that calls
free_irq(db->irq_wake, ndev) before dm9000_release_board()?

[Severity: High]
This isn't a bug introduced by this patch, but the same missing free_irq()
applies to the paths that already existed.  The later goto out failures in
dm9000_probe() (request_mem_region() or ioremap() failure, unknown chip ID,
register_netdev() failure), for example:

	if (db->addr_req == NULL) {
		dev_err(db->dev, "cannot claim address reg area\n");
		ret = -EIO;
		goto out;
	}

and dm9000_drv_remove():

	unregister_netdev(ndev);
	dm9000_release_board(pdev, dm);
	if (dm->power_supply)
		regulator_disable(dm->power_supply);

	free_netdev(ndev);		/* free device structure */

all reach free_netdev(ndev) while dm9000_wol_interrupt() is still
registered with dev_id = ndev, so an unbind or rmmod of a device that has a
wake IRQ leaves a dangling handler on a shared line.  Would it make sense
to add the wake-IRQ release in a helper shared between the probe error
paths and remove()?

>  			} else {
>  				irq_set_irq_wake(db->irq_wake, 0);
>  				db->wake_supported = 1;

[ ... ]

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