Message ID | 20240828130403.14145-2-przemyslaw.kitszel@intel.com |
---|---|
State | Superseded |
Headers | show |
Series | [iwl-net] ice: stop calling pci_disable_device() as we use pcim | expand |
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. 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); -} - -static struct pci_devres *get_pci_dr(struct pci_dev *pdev) -{ - struct pci_devres *dr, *new_dr; - - dr = devres_find(&pdev->dev, pcim_release, NULL, NULL); - if (dr) - return dr; + struct pci_dev *pdev = pdev_raw; - new_dr = devres_alloc(pcim_release, sizeof(*new_dr), GFP_KERNEL); - if (!new_dr) - return NULL; - return devres_get(&pdev->dev, new_dr, NULL, NULL); + if (!pdev->pinned) + pci_disable_device(pdev); } 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. > (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 <pstanner@redhat.com> > Fixes: f748a07a0b64 ("PCI: Remove legacy pcim_release()") > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Reviewed-by: Philipp Stanner <pstanner@redhat.com> with or without the above suggestion for the commit message. Thanks for solving this! Regards, P. > --- > drivers/net/ethernet/intel/ice/ice_main.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c > b/drivers/net/ethernet/intel/ice/ice_main.c > index 6f97ed471fe9..18e4950316f1 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -5350,7 +5350,6 @@ ice_probe(struct pci_dev *pdev, const struct > pci_device_id __always_unused *ent) > ice_deinit(pf); > err_init: > ice_adapter_put(pdev); > - pci_disable_device(pdev); > return err; > } > > @@ -5457,7 +5456,6 @@ static void ice_remove(struct pci_dev *pdev) > ice_set_wake(pf); > > ice_adapter_put(pdev); > - pci_disable_device(pdev); > } > > /** > > base-commit: 4186c8d9e6af57bab0687b299df10ebd47534a0a
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 <pstanner@redhat.com> >> Fixes: f748a07a0b64 ("PCI: Remove legacy pcim_release()") >> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> >> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > Reviewed-by: Philipp Stanner <pstanner@redhat.com> > > with or without the above suggestion for the commit message. > > > Thanks for solving this! > > Regards, > P.
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 6f97ed471fe9..18e4950316f1 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -5350,7 +5350,6 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent) ice_deinit(pf); err_init: ice_adapter_put(pdev); - pci_disable_device(pdev); return err; } @@ -5457,7 +5456,6 @@ static void ice_remove(struct pci_dev *pdev) ice_set_wake(pf); ice_adapter_put(pdev); - pci_disable_device(pdev); } /**