Re: [PATCH v3] x86/msi: fix locking for SR-IOV devices

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 8/15/24 04:36, Jan Beulich wrote:
> On 15.08.2024 03:28, Stewart Hildebrand wrote:
>> On 8/13/24 10:01, Jan Beulich wrote:
>>> On 12.08.2024 22:39, Stewart Hildebrand wrote:
>>>> @@ -446,7 +448,27 @@ static void free_pdev(struct pci_seg *pseg, struct 
>>>> pci_dev *pdev)
>>>>  
>>>>      list_del(&pdev->alldevs_list);
>>>>      pdev_msi_deinit(pdev);
>>>> -    xfree(pdev);
>>>> +
>>>> +    if ( pdev->info.is_virtfn )
>>>> +    {
>>>> +        struct pci_dev *pf_pdev = pdev->virtfn.pf_pdev;
>>>> +
>>>> +        if ( pf_pdev )
>>>> +        {
>>>> +            list_del(&pdev->virtfn.next);
>>>> +            if ( pf_pdev->physfn.dealloc_pending &&
>>>> +                 list_empty(&pf_pdev->physfn.vf_list) )
>>>> +                xfree(pf_pdev);
>>>> +        }
>>>> +        xfree(pdev);
>>>> +    }
>>>> +    else
>>>> +    {
>>>> +        if ( list_empty(&pdev->physfn.vf_list) )
>>>> +            xfree(pdev);
>>>> +        else
>>>> +            pdev->physfn.dealloc_pending = true;
>>>> +    }
>>>
>>> Could I talk you into un-indenting the last if/else by a level, by making
>>> the earlier else and "else if()"?
>>>
>>> One aspect I didn't properly consider when making the suggestion: What if,
>>> without all VFs having gone away, the PF is re-added? In that case we
>>> would better recycle the existing structure. That's getting a little
>>> complicated, so maybe a better approach is to refuse the request (in
>>> pci_remove_device()) when the list isn't empty?
>>
>> Hm. Right. The growing complexity of maintaining these PF<->VF links
>> (introduced on a hunch that they may be useful in the future) is making
>> me favor the previous approach from v2 of simply copying the vf_len
>> array. So unless there are any objections I'll go back to that approach
>> for v4.
> 
> I continue to consider the earlier approach at least undesirable. The
> vf_len[] array shouldn't be copied around, risking for it to be / go
> stale. There should be one central place for every bit of information,
> unless there are very good reasons to duplicate something.

OK, I'll continue to refine the PF<->VF link approach for v4.