Re: [PATCH] pcie: Release references of virtual functions

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 1/7/23 08:19, Akihiko Odaki wrote:
> On 2023/04/21 17:09, Michael S. Tsirkin wrote:
>> On Tue, Apr 11, 2023 at 12:11:30PM +0200, Philippe Mathieu-Daudé wrote:
>>> On 11/4/23 11:04, Akihiko Odaki wrote:
>>>> pci_new() automatically retains a reference to a virtual function when
>>>> registering it so we need to release the reference when unregistering.
>>>>
>>>> Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O 
>>>> Virtualization (SR/IOV)")
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    hw/pci/pcie_sriov.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>>>> index aa5a757b11..76a3b6917e 100644
>>>> --- a/hw/pci/pcie_sriov.c
>>>> +++ b/hw/pci/pcie_sriov.c
>>>> @@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
>>>>                error_free(local_err);
>>>>            }
>>>>            object_unparent(OBJECT(vf));
>>>> +        object_unref(OBJECT(vf));
>>>>        }
>>>>        g_free(dev->exp.sriov_pf.vf);
>>>>        dev->exp.sriov_pf.vf = NULL;
>>>
>>> It feels the issue is at the device creation.
>>>
>>> [/me looking at the code]
>>>
>>> What about:
>>>
>>> -- >8 --
>>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>>> index aa5a757b11..fca3bf6e72 100644
>>> --- a/hw/pci/pcie_sriov.c
>>> +++ b/hw/pci/pcie_sriov.c
>>> @@ -150,7 +150,7 @@ static PCIDevice *register_vf(PCIDevice *pf, int 
>>> devfn,
>>> const char *name,
>>>       PCIBus *bus = pci_get_bus(pf);
>>>       Error *local_err = NULL;
>>>
>>> -    qdev_realize(&dev->qdev, &bus->qbus, &local_err);
>>> +    pci_realize_and_unref(dev, bus, &local_err);
>>>       if (local_err) {
>>>           error_report_err(local_err);
>>>           return NULL;
>>
>> ok you want to repost this or Akihiko convinced you?
>>
>>
>>> ---
>>
> 
> Can you reply to this?

Sorry I didn't noticed MST question.

PCIDevice should use their class realize() handler, not the qdev
parent. Code smell somewhere, but can be cleaned later.
I'm fine with Akihiko's patch.

Regards,

Phil.