* [Qemu-devel] [PATCH RFC] pci: call PCIDeviceClass->exit on its .realize failure
@ 2016-09-14 9:14 Cao jin
2016-09-14 11:59 ` Marcel Apfelbaum
0 siblings, 1 reply; 3+ messages in thread
From: Cao jin @ 2016-09-14 9:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum
It cannot guarantee all pci devices will free the allocated resource in
its .realize function on realize failure.
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
I found not all the devices will free the allocated resources on .realize
failure, and .exit function is the one who take responsibility to free all
the resource. In theory, I think it should be PCIDeviceClass->exit who does
the cleanup on realize failure, with appropriate check whether certain
resources is allocated.
It passed make check, but maybe need more confirmation, so, RFC.
hw/pci/pci.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 24fae16..4b63a79 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1967,6 +1967,9 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
if (local_err) {
error_propagate(errp, local_err);
do_pci_unregister_device(pci_dev);
+ if (pc->exit) {
+ pc->exit(pci_dev);
+ }
return;
}
}
--
2.1.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] pci: call PCIDeviceClass->exit on its .realize failure
2016-09-14 9:14 [Qemu-devel] [PATCH RFC] pci: call PCIDeviceClass->exit on its .realize failure Cao jin
@ 2016-09-14 11:59 ` Marcel Apfelbaum
2016-09-19 6:16 ` Cao jin
0 siblings, 1 reply; 3+ messages in thread
From: Marcel Apfelbaum @ 2016-09-14 11:59 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: Michael S. Tsirkin
On 09/14/2016 12:14 PM, Cao jin wrote:
> It cannot guarantee all pci devices will free the allocated resource in
> its .realize function on realize failure.
>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>
> I found not all the devices will free the allocated resources on .realize
> failure, and .exit function is the one who take responsibility to free all
> the resource. In theory, I think it should be PCIDeviceClass->exit who does
> the cleanup on realize failure, with appropriate check whether certain
> resources is allocated.
> It passed make check, but maybe need more confirmation, so, RFC.
>
> hw/pci/pci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 24fae16..4b63a79 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1967,6 +1967,9 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> if (local_err) {
> error_propagate(errp, local_err);
> do_pci_unregister_device(pci_dev);
> + if (pc->exit) {
> + pc->exit(pci_dev);
> + }
Hi,
I think the call to pc->exit is not necessary here.
The call to pc->realize failed, so it is the callee (pc)
responsibility to clean the resources.
Please see in the same method:
pci_add_option_rom(pci_dev, is_default_rom, &local_err);
if (local_err) {
error_propagate(errp, local_err);
pci_qdev_unrealize(DEVICE(pci_dev), NULL);
^^^^^^^^^^^^^^^^^^^
This time we call pci_qdev_unrealize (that will call pc->exit)
because pc->realize succeeded and it is now the caller responsibility
to clean the resources.
Have you found a specific scenario that causes problems?
Thanks,
Marcel
> return;
> }
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] pci: call PCIDeviceClass->exit on its .realize failure
2016-09-14 11:59 ` Marcel Apfelbaum
@ 2016-09-19 6:16 ` Cao jin
0 siblings, 0 replies; 3+ messages in thread
From: Cao jin @ 2016-09-19 6:16 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: Michael S. Tsirkin
Hi,
sorry for replying late(was in vacation).
On 09/14/2016 07:59 PM, Marcel Apfelbaum wrote:
> On 09/14/2016 12:14 PM, Cao jin wrote:
>> It cannot guarantee all pci devices will free the allocated resource in
>> its .realize function on realize failure.
>>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>
>> I found not all the devices will free the allocated resources on .realize
>> failure, and .exit function is the one who take responsibility to free
>> all
>> the resource. In theory, I think it should be PCIDeviceClass->exit who
>> does
>> the cleanup on realize failure, with appropriate check whether certain
>> resources is allocated.
>> It passed make check, but maybe need more confirmation, so, RFC.
>>
>
> Hi,
>
> I think the call to pc->exit is not necessary here.
>
> The call to pc->realize failed, so it is the callee (pc)
> responsibility to clean the resources.
>
> Please see in the same method:
>
> pci_add_option_rom(pci_dev, is_default_rom, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>
> ^^^^^^^^^^^^^^^^^^^
>
> This time we call pci_qdev_unrealize (that will call pc->exit)
> because pc->realize succeeded and it is now the caller responsibility
> to clean the resources.
>
>
> Have you found a specific scenario that causes problems?
No, it came to my mind when doing the patch "convert to error". Look
usb_xhci_realize(), it will call usb_xhci_init to do some resource
allocation, but won't free the resource once return because of
msi_init's failure(that may be my fault), so the following patch[*] move
it down.
I agree your view above. We can forget this patch:)
[*]http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg03063.html
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-09-19 6:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-14 9:14 [Qemu-devel] [PATCH RFC] pci: call PCIDeviceClass->exit on its .realize failure Cao jin
2016-09-14 11:59 ` Marcel Apfelbaum
2016-09-19 6:16 ` Cao jin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.