Re: [PATCH] net: dsa: vitesse-vsc73xx: Fix SPI device reference leak in vsc73xx_spi_probe()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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 newly added spi_dev_put() calls do not cover
  the shutdown()->remove() ordering, so the reference leak the commit…

--

net: dsa: vitesse-vsc73xx: Fix SPI device reference leak in vsc73xx_spi_probe()

This adds spi_dev_put() calls to balance the spi_dev_get() done in
vsc73xx_spi_probe(), covering the two probe error paths and
vsc73xx_spi_remove().

> diff --git a/drivers/net/dsa/vitesse-vsc73xx-spi.c b/drivers/net/dsa/vitesse-vsc73xx-spi.c
> index 85b9a0f51dd89..7b784320c41e7 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-spi.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-spi.c
> @@ -153,10 +153,15 @@ static int vsc73xx_spi_probe(struct spi_device *spi)
>  	ret = spi_setup(spi);
>  	if (ret < 0) {
>  		dev_err(dev, "spi setup failed.\n");
> +		spi_dev_put(vsc_spi->spi);
>  		return ret;
>  	}
>  
> -	return vsc73xx_probe(&vsc_spi->vsc);
> +	ret = vsc73xx_probe(&vsc_spi->vsc);
> +	if (ret)
> +		spi_dev_put(vsc_spi->spi);
> +
> +	return ret;
>  }
>  
>  static void vsc73xx_spi_remove(struct spi_device *spi)
> @@ -167,6 +172,8 @@ static void vsc73xx_spi_remove(struct spi_device *spi)
>  		return;
>  
>  	vsc73xx_remove(&vsc_spi->vsc);
> +
> +	spi_dev_put(vsc_spi->spi);
>  }
>  
>  static void vsc73xx_spi_shutdown(struct spi_device *spi)

[Severity: High]
Does this still leak the spi_device reference when shutdown() runs before
remove()?

The new spi_dev_put() sits after the NULL drvdata guard in
vsc73xx_spi_remove():

	struct vsc73xx_spi *vsc_spi = spi_get_drvdata(spi);

	if (!vsc_spi)
		return;

	vsc73xx_remove(&vsc_spi->vsc);

	spi_dev_put(vsc_spi->spi);

and vsc73xx_spi_shutdown() deliberately clears drvdata without dropping
the reference:

	vsc73xx_shutdown(&vsc_spi->vsc);

	spi_set_drvdata(spi, NULL);

Documentation/networking/dsa/dsa.rst describes this ordering as permitted:

  to be called even if ``shutdown()`` was already called. Therefore, drivers are
  expected to implement a mutual exclusion method between ``remove()`` and
  ``shutdown()`` by setting their drvdata to NULL after any of these has run

So on reboot followed by SPI controller teardown, for example
dspi_shutdown() -> dspi_remove() -> spi_unregister_controller() ->
spi_unregister_device() -> device_del() -> vsc73xx_spi_remove(), the
remove callback returns early at the !vsc_spi check and the extra
reference on the spi_device (and its per-CPU spi_statistics) is never
dropped. The commit message does not mention the shutdown path.

Would it be simpler to drop the reference entirely instead of adding
matching puts, i.e. in vsc73xx_spi_probe():

	vsc_spi->spi = spi;

vsc_spi is devm_kzalloc()'d on &spi->dev, and vsc_spi->spi is only
dereferenced from vsc73xx_spi_read()/vsc73xx_spi_write() while the driver
is bound, a window in which the driver core already keeps the spi_device
alive. This file is also the only user of spi_dev_get()/spi_dev_put()
outside drivers/spi and drivers/platform/x86; peer DSA SPI drivers just
store the pointer, see b53_spi_probe() in drivers/net/dsa/b53/b53_spi.c
and ksz_spi_probe() in drivers/net/dsa/microchip/ksz_spi.c. That would
make every path, including shutdown, symmetric without adding three call
sites to keep in sync.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917104920.2147568-1-vulab%40iscas.ac.cn