| Message ID | 1473844466-17458-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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; > } > } >
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
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; } }
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(+)