Re: [Intel-wired-lan] [PATCH iwl-net] ice: stop calling pci_disable_device() as we use pcim

[Philipp Stanner <pstanner@redhat.com>]

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