* [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).