* [PATCH] pcie: Release references of virtual functions
@ 2023-04-11 9:04 Akihiko Odaki
2023-04-11 9:07 ` Cédric Le Goater
2023-04-11 10:11 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 7+ messages in thread
From: Akihiko Odaki @ 2023-04-11 9:04 UTC (permalink / raw)
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Akihiko Odaki
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;
--
2.40.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pcie: Release references of virtual functions
2023-04-11 9:04 [PATCH] pcie: Release references of virtual functions Akihiko Odaki
@ 2023-04-11 9:07 ` Cédric Le Goater
2023-04-11 10:11 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2023-04-11 9:07 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum
On 4/11/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>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> 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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcie: Release references of virtual functions
2023-04-11 9:04 [PATCH] pcie: Release references of virtual functions Akihiko Odaki
2023-04-11 9:07 ` Cédric Le Goater
@ 2023-04-11 10:11 ` Philippe Mathieu-Daudé
2023-04-11 11:05 ` Akihiko Odaki
2023-04-21 8:09 ` Michael S. Tsirkin
1 sibling, 2 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-04-11 10:11 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Cédric Le Goater
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;
---
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pcie: Release references of virtual functions
2023-04-11 10:11 ` Philippe Mathieu-Daudé
@ 2023-04-11 11:05 ` Akihiko Odaki
2023-04-21 8:09 ` Michael S. Tsirkin
1 sibling, 0 replies; 7+ messages in thread
From: Akihiko Odaki @ 2023-04-11 11:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Cédric Le Goater
On 2023/04/11 19:11, 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.
I added object_unref() to unregister_vfs() because dev->exp.sriov_pf.vf
actually holds the reference. Practically there should be no difference
as the parent bus also keeps the reference until unregister_vfs() calls
object_unparent(), but I think it is semantically more correct to
object_unref() in unregister_vfs().
>
> [/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;
> ---
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcie: Release references of virtual functions
2023-04-11 10:11 ` Philippe Mathieu-Daudé
2023-04-11 11:05 ` Akihiko Odaki
@ 2023-04-21 8:09 ` Michael S. Tsirkin
2023-07-01 6:19 ` Akihiko Odaki
1 sibling, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2023-04-21 8:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Akihiko Odaki, qemu-devel, Marcel Apfelbaum,
Cédric Le Goater
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?
> ---
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcie: Release references of virtual functions
2023-04-21 8:09 ` Michael S. Tsirkin
@ 2023-07-01 6:19 ` Akihiko Odaki
2023-07-02 11:21 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Akihiko Odaki @ 2023-07-01 6:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Michael S. Tsirkin, qemu-devel, Marcel Apfelbaum,
Cédric Le Goater
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?
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcie: Release references of virtual functions
2023-07-01 6:19 ` Akihiko Odaki
@ 2023-07-02 11:21 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-07-02 11:21 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, qemu-devel, Marcel Apfelbaum,
Cédric Le Goater, Markus Armbruster
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-07-02 11:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-11 9:04 [PATCH] pcie: Release references of virtual functions Akihiko Odaki
2023-04-11 9:07 ` Cédric Le Goater
2023-04-11 10:11 ` Philippe Mathieu-Daudé
2023-04-11 11:05 ` Akihiko Odaki
2023-04-21 8:09 ` Michael S. Tsirkin
2023-07-01 6:19 ` Akihiko Odaki
2023-07-02 11:21 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).