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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

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.