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

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

[Przemek Kitszel]

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 (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>
---
 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
-- 
2.45.1

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

[Philipp Stanner]

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

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

[Przemek Kitszel]

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.