Re: [PATCH 1/2] drm/xe/pf: Disable VFs on remove

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 27.06.2024 10:24, Jani Nikula wrote:
> On Wed, 26 Jun 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
>> We shouldn't leave VFs enabled when unloading the PF driver.
>> Otherwise we will get a message like:
>>
>>  [ ] xe 0000:4d:00.0: driver left SR-IOV enabled after remove
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Piotr Piórkowski <piotr.piorkowski@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_pci.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index ebff5ea79b1d..4b91a9878f41 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -749,6 +749,11 @@ static void xe_pci_remove(struct pci_dev *pdev)
>>  	if (!xe) /* driver load aborted, nothing to cleanup */
>>  		return;
>>  
>> +#ifdef CONFIG_PCI_IOV
>> +	if (IS_SRIOV_PF(xe))
>> +		xe_pci_sriov_configure(pdev, 0);
>> +#endif
>> +
> 
> Please don't add inline #ifdefs.
> 
> IS_SRIOV_PF() works for CONFIG_PCI_IOV=n. xe_pci_sriov_configure()
> should also have a stub for CONFIG_PCI_IOV=n.
> 
> Alternatively, if you also added a dedicated function for disabling VF,
> you could have a stub for that, and
> 
> #define xe_pci_sriov_configure NULL
> 
> for CONFIG_PCI_IOV=n, so you wouldn't have to have inline #ifdefs in
> xe_pci_driver initialization either.

I was somehow biased that pci_driver.sriov_config hook is only available
under CONFIG_PCI_IOV (which is not true) and that's why followed the
pattern as it was done for the .driver.pm

See [1] where both #ifdef CONFIG_PCI_IOV were removed

[1] https://patchwork.freedesktop.org/series/135472/

Thanks for spotting this,
Michal

> 
> 
> BR,
> Jani.
> 
> 
>>  	xe_device_remove(xe);
>>  	xe_pm_runtime_fini(xe);
>>  	pci_set_drvdata(pdev, NULL);
>