From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52669) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blrsi-0004oj-CZ for qemu-devel@nongnu.org; Mon, 19 Sep 2016 02:16:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1blrsd-0002lN-Uq for qemu-devel@nongnu.org; Mon, 19 Sep 2016 02:16:43 -0400 Received: from [59.151.112.132] (port=10602 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blrsd-0002i2-FJ for qemu-devel@nongnu.org; Mon, 19 Sep 2016 02:16:39 -0400 References: <1473844466-17458-1-git-send-email-caoj.fnst@cn.fujitsu.com> From: Cao jin Message-ID: <57DF82D6.4040902@cn.fujitsu.com> Date: Mon, 19 Sep 2016 14:16:54 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC] pci: call PCIDeviceClass->exit on its .realize failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org 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 >> CC: Marcel Apfelbaum >> Signed-off-by: Cao jin >> --- >> >> 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