Re: [PATCH] PCI: Init driver override spinlock in new_id_store()

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Apr 27, 2026 at 10:36 PM CEST, Samiullah Khawaja wrote:
> On Mon, Apr 27, 2026 at 09:46:20PM +0200, Danilo Krummrich wrote:
>>On Mon Apr 27, 2026 at 9:31 PM CEST, Samiullah Khawaja wrote:
>>> Fixes: cb3d1049f4ea ("driver core: generalize driver_override in struct device")
>>
>>I don't think anything is wrong with this commit, and it seems unrelated.
>
> I will remove this one.
>>
>>> Fixes: 10a4206a2401 ("PCI: use generic driver_override infrastructure")
>>
>>I'm also not sure that this one contains the root cause, despite revealing the
>>actual issue, more below.
>
> I see your point. I understand the actual issue is that the temporary
> pci_dev struct was not init and that was added in 2014. I added this
> since this is where the regression happened.

Yeah, I did not mean to say that this tag is wrong -- it is where the regression
happened. But it may be worth adding both, this one and the one that did omit
device_initialize() in the first place.

More in general, this can matter because it influences how (far) it is
backported in stable trees.

While rather unlikely in this case, there could be a subsequent fix that is
incompatible with device_initialize() not being called that may be backported
event further.

This would not regress in recent trees as it would have been fixed there
already, but might regress in an ancient stable tree, that did not get this fix.

>>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>>> ---
>>>  drivers/pci/pci-driver.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>>> index d10ece0889f0..5f453213b8c5 100644
>>> --- a/drivers/pci/pci-driver.c
>>> +++ b/drivers/pci/pci-driver.c
>>> @@ -215,6 +215,11 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>>>  		pdev->subsystem_device = subdevice;
>>>  		pdev->class = class;
>>>
>>> +		/*
>>> +		 * Initialize the embedded struct device driver_override lock to
>>> +		 * avoid the lockdep errors.
>>> +		 */
>>> +		spin_lock_init(&pdev->dev.driver_override.lock);
>>
>>Can't we just call device_initialize() and set pdev->dev.release to a new
>>function that just calls kfree()?
>>
>>This way nothing of that kind can ever happen again; it is hard to predict that
>>a device is used without ever being initialized.
>
> Agreed.
>
> yes we can set the release function and call device_initialize().
>
> Should I send v2 with something like following:
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index d10ece0889f0..634b4311bf5d 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -179,6 +179,11 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>          return NULL;
>   }
>
> +static void _release_temp_device(struct device *dev)
> +{
> +       kfree(to_pci_dev(dev));
> +}
> +
>   /**
>    * new_id_store - sysfs frontend to pci_add_dynid()
>    * @driver: target device driver
> @@ -214,11 +219,14 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>                  pdev->subsystem_vendor = subvendor;
>                  pdev->subsystem_device = subdevice;
>                  pdev->class = class;
> +               pdev->dev.release = _release_temp_device;
>
> +               /* Initialize the embedded struct device. */
> +               device_initialize(&pdev->dev);
>                  if (pci_match_device(pdrv, pdev))
>                          retval = -EEXIST;
>
> -               kfree(pdev);
> +               put_device(&pdev->dev);

Overall Looks good. Not sure about the _release_temp_device() name and I think
the comment above device_initialize() may be unnecessary, but I'm pretty sure
the PCI maintainers will have an opinion on that. :)