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

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

[Samiullah Khawaja]

When setting new_id of a PCI device driver using sysfs a lockdep splat
occurs. This is because the new_id function new_id_store() checks for
driver_override by creating temporary pci_dev structs.

Since the newly added driver_override spinlock is not init for temporary
pci_dev structs, the lockdep splat complains about it.

Init the driver_override spinlock to fix this.

[    4.464296] INFO: trying to register non-static key.
[    4.466207] The code is fine but needs lockdep annotation, or maybe
[    4.468487] you didn't initialize this object before use?
[    4.470486] turning off the locking correctness validator.
[    4.472494] CPU: 2 UID: 0 PID: 177 Comm: liveupdate-iomm Not tainted 7.0.0+ #9 PREEMPT(full)
[    4.472497] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[    4.472498] Call Trace:
[    4.472501]  <TASK>
[    4.472503]  dump_stack_lvl+0x5d/0x80
[    4.472508]  register_lock_class+0x77e/0x790
[    4.472512]  ? check_prev_add+0xf4/0xd30
[    4.472513]  ? check_prev_add+0xf4/0xd30
[    4.472515]  __lock_acquire+0x3b4/0x1b80
[    4.472518]  ? raw_irqentry_exit_cond_resched+0x20/0x50
[    4.472556]  lock_acquire+0xbf/0x2e0
[    4.472558]  ? pci_match_device+0x24/0x180
[    4.472563]  _raw_spin_lock+0x30/0x40
[    4.472567]  ? pci_match_device+0x24/0x180
[    4.472569]  pci_match_device+0x24/0x180
[    4.472571]  new_id_store+0x189/0x1d0
[    4.472574]  kernfs_fop_write_iter+0x14f/0x210
[    4.472578]  vfs_write+0x263/0x5e0
[    4.472583]  ksys_write+0x79/0xf0
[    4.472585]  do_syscall_64+0x117/0xf80
[    4.472587]  ? clear_bhb_loop+0x40/0x90
[    4.472590]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[    4.472592] RIP: 0033:0x7fe7bc0beeb2
[    4.472595] Code: 18 41 8b 93 08 03 00 00 59 5e 48 83 f8 fc 75 1a 83 e2 39 83 fa 08 75 12 e8 2b ff ff ff 0f 1f 00 49 89 ca 48 8b 44 24 20 0f 05 <48> 83 c4 18 c3 66 0f 1f 84 00 00 00 00 00 48 83 ec 10 ff 74 24 18
[    4.472596] RSP: 002b:00007ffcdbb1ed80 EFLAGS: 00000202 ORIG_RAX: 0000000000000001
[    4.472598] RAX: ffffffffffffffda RBX: 00007fe7bc2415c0 RCX: 00007fe7bc0beeb2
[    4.472599] RDX: 000000000000000a RSI: 00005580955433c0 RDI: 0000000000000001
[    4.472600] RBP: 000000000000000a R08: 0000000000000000 R09: 0000000000000000
[    4.472601] R10: 0000000000000000 R11: 0000000000000202 R12: 000000000000000a
[    4.472602] R13: 00005580955433c0 R14: 000055805932493e R15: 0000558095545490
[    4.472605]  </TASK>

Fixes: cb3d1049f4ea ("driver core: generalize driver_override in struct device")
Fixes: 10a4206a2401 ("PCI: use generic driver_override infrastructure")
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);
 		if (pci_match_device(pdrv, pdev))
 			retval = -EEXIST;
 

base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.54.0.545.g6539524ca2-goog

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

[Danilo Krummrich]

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.

> 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.

> 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.

>  		if (pci_match_device(pdrv, pdev))
>  			retval = -EEXIST;
>  
>
> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> -- 
> 2.54.0.545.g6539524ca2-goog

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

[Samiullah Khawaja]

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.
>
>> 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);

                 if (retval)
                         return retval;
>
>>  		if (pci_match_device(pdrv, pdev))
>>  			retval = -EEXIST;
>>
>>
>> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
>> --
>> 2.54.0.545.g6539524ca2-goog

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

[Danilo Krummrich]

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. :)