Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/8/5 16:10, Jan Beulich wrote:
> On 05.08.2025 05:49, Jiqian Chen wrote:
>> When MSI-X initialization fails vPCI will hide the capability, but
>> remove of handlers and data won't be performed until the device is
>> deassigned.  Introduce a MSI-X cleanup hook that will be called when
>> initialization fails to cleanup MSI-X related hooks and free it's
>> associated data.
>>
>> As all supported capabilities have been switched to use the cleanup
>> hooks call those from vpci_deassign_device() instead of open-code the
>> capability specific cleanup in there.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>> v9->v10 changes:
>> * Call all cleanup hook in vpci_deassign_device() instead of cleanup_msix().
> 
> Isn't this rather an omission in an earlier change, and hence may want to
> come separately and with a Fixes: tag?
This is not really an omission, after all, all the cleanup hooks were 
implemented at the end of this series.
And judging from the commit message(which was written by Roger in v8), Roger 
also agreed to add them in this patch.

> 
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -321,6 +321,27 @@ void vpci_deassign_device(struct pci_dev *pdev)
>>                      &pdev->domain->vpci_dev_assigned_map);
>>  #endif
>>  
>> +    for ( i = 0; i < NUM_VPCI_INIT; i++ )
>> +    {
>> +        const vpci_capability_t *capability = &__start_vpci_array[i];
>> +        const unsigned int cap = capability->id;
>> +        unsigned int pos = 0;
>> +
>> +        if ( !capability->is_ext )
>> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
>> +        else if ( is_hardware_domain(pdev->domain) )
>> +            pos = pci_find_ext_capability(pdev->sbdf, cap);
> 
> What's the point of doing this when ...
> 
>> +        if ( pos && capability->cleanup )
> 
> ... ->cleanup is NULL? Don't you want to have
> 
>         if ( !capability->cleanup )
>             continue;
> 
> earlier on?
Will change in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.