On 8/29/24 09:11, Philipp Stanner wrote:
Hi,
On Wed, 2024-08-28 at 15:03 +0200, Przemek Kitszel wrote:
Our driver uses devres to manage resources, in particular we call
pcim_enable_device(), which recently has registered
pcim_disable_device()
as device remove action
That's not the exact cause, actually.
Thanks for review, you are correct, I will reword the above paragraph.
The ultimate call to pci_disable_device() (not pcim_) through callbacks
set up by pcim_enable_device() has always been there. It's not me
adding that which caused the warning. What caused it is that I removed
the enabled-check from pcim_disable_device():
f748a07a0b64:
-static void pcim_release(struct device *gendev, void *res)
+static void pcim_disable_device(void *pdev_raw)
{
- struct pci_dev *dev = to_pci_dev(gendev);
-
- if (pci_is_enabled(dev) && !dev->pinned)
- pci_disable_device(dev);
[...]
Theoretically, we could add
if (pci_is_enabled(...
back, but I think the far cleaner solution is to clean up the drivers
as you do here if that warning occurs. Faults should not be caused by
this, just warnings, if I read the code correctly. Please correct me if
not.
I agree that your approach is the correct way, and it's fine to
introduce warnings in misconfigured drivers. Fixes tag is to only
allow backporting the change (as noone wants to see unrelated-to-their-
work splats ;)).
(see cited "Fixes" commit). Since that, unloading
the driver yields following warn+splat:
[70633.628490] ice 0000:af:00.7: disabling already-disabled device
[70633.628512] WARNING: CPU: 52 PID: 33890 at drivers/pci/pci.c:2250
pci_disable_device+0xf4/0x100
...
[70633.628744] ? pci_disable_device+0xf4/0x100
[70633.628752] release_nodes+0x4a/0x70
[70633.628759] devres_release_all+0x8b/0xc0
[70633.628768] device_unbind_cleanup+0xe/0x70
[70633.628774] device_release_driver_internal+0x208/0x250
[70633.628781] driver_detach+0x47/0x90
[70633.628786] bus_remove_driver+0x80/0x100
[70633.628791] pci_unregister_driver+0x2a/0xb0
[70633.628799] ice_module_exit+0x11/0x3a [ice]
Note that this is the only Intel ethernet driver that needs such fix.
CC: Philipp Stanner <[email protected]>
Fixes: f748a07a0b64 ("PCI: Remove legacy pcim_release()")
Reviewed-by: Larysa Zaremba <[email protected]>
Signed-off-by: Przemek Kitszel <[email protected]>
Reviewed-by: Philipp Stanner <[email protected]>
with or without the above suggestion for the commit message.
Thanks for solving this!
Regards,
P.
