Re: [PATCH] PCI: Handle dev_set_name() failure in pci_setup_device()

[Krzysztof Wilczyński]

Hello,

[...]
> Propagate the error instead.  pci_setup_device() already returns int and
> both callers, pci_scan_device() and pci_iov_scan_device(), release the
> device on failure, so no caller changes are needed.  Release the OF node
> first, matching the existing error path for an unknown header type.

The kernel-doc for pci_setup_device() will probably need to be updated to
reflect changes in what the function returns on failure, since now you have
the -EIO and also -ENOMEM, potentially.

> Fixes: eebfcfb52ce7 ("PCI: handle pci_name() being const")

Probably:

  Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")

The commit you have references a state of the code, a much older code base,
where the implementation was fundamentally different, per:

  va_start(vargs, fmt);
  vsnprintf(dev->bus_id, sizeof(dev->bus_id), fmt, vargs);
  va_end(vargs);
  return 0;

The kvasprintf(), the kobject_set_name_vargs() uses internally, has been
replaced with kvasprintf_const() since commit f773f32d71a4 ("lib/kobject.c:
use kvasprintf_const for formatting ->name"), so the commit log is not
strictly correct.

[...]
> Two things I noticed while working on this and deliberately left alone,
> since they look like separate changes:
> 
> - pci_scan_device() releases the pci_dev with kfree() rather than
>   put_device(), so on the existing "unknown header type" -EIO path the
>   name allocated by dev_set_name() is leaked.  That was reported in 2022
>   [1] but never applied.

The fix there is valid and would be nice to also pick it up.  Might use
a little...

  dev->dev.kobj.name = NULL;

After the kfree_const().

Feel free to pick it up, and include here as a second patch, so a small
series.  Don't forget to credit Yang Yingliang, if you do decide to
follow-up.

> - pci_device_add() ignores device_add() failure entirely; the WARN_ON() is
>   all there is and the half-added device stays on bus->devices.  I built a
>   kernel with [2] applied (it stops device_add() from freeing dev->p) and
>   the NULL dereference above does go away, but the WARNING and the stale
>   bus->devices entry remain.  Happy to follow up on that separately if you
>   think it is worth doing.

This one I am not sure.  The NULL-assignment move looks awkward there.

[...]
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd0abbc63e18..474c6cb327be 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2052,9 +2052,13 @@ int pci_setup_device(struct pci_dev *dev)
>  	 */
>  	dev->msi_addr_mask = DMA_BIT_MASK(64);
>  
> -	dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
> -		     dev->bus->number, PCI_SLOT(dev->devfn),
> -		     PCI_FUNC(dev->devfn));
> +	err = dev_set_name(&dev->dev, "%04x:%02x:%02x.%d",
> +			   pci_domain_nr(dev->bus), dev->bus->number,
> +			   PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> +	if (err) {
> +		pci_release_of_node(dev);
> +		return err;
> +	}
>  
>  	class = pci_class(dev);

Looks good!

  Reviewed-by: Krzysztof Wilczyński <kwilczynski@xxxxxxxxxx>

Thank you for the fix!

	Krzysztof