From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kenji Kaneshige Subject: Re: [PATCH] pci: clean all funcs when hot-removing multifunc device Date: Fri, 16 Sep 2011 08:56:32 +0900 Message-ID: <4E7290B0.1040801@jp.fujitsu.com> References: <1315976141-6684-1-git-send-email-akong@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: linux-pci@vger.kernel.org, jbarnes@virtuousgeek.org, mst@redhat.com, alex.williamson@redhat.com, mtosatti@redhat.com, kvm@vger.kernel.org, Kevin O'Connor , seabios@seabios.org To: Bjorn Helgaas , Amos Kong Return-path: In-Reply-To: Sender: linux-pci-owner@vger.kernel.org List-Id: kvm.vger.kernel.org (2011/09/16 4:03), Bjorn Helgaas wrote: > On Tue, Sep 13, 2011 at 10:55 PM, Amos Kong wrote: >> 'slot->funcs' is initialized in acpiphp_glue.c:register_slot() before >> hotpluging device, and only one entry(func 0) is added to it, >> no new entry will be added to the list when hotpluging devices to the slot. >> When we release the whole device, there is only one entry in the list, >> this causes func1~7 could not be released. >> I try to add entries for all hotpluged device in enable_device(), but >> it doesn't work, because 'slot->funcs' is used in many place which we only >> need to process func 0. This patch just try to clean all funcs in >> disable_device(). > ... >> Hotpluging multifunc of WinXp is fine. > > I'm going to ignore this patch for now. Please consider these > questions, then repost it if you still want it: > > I assume you mean that Linux and WinXP are both running on top of the > same SeaBIOS, and hot-remove of a multifunction device works in WinXP > and fails in Linux. That sounds like Linux is broken, and we should > fix it. We might want to make a SeaBIOS change for other reasons, but > it'd still be good to fix Linux in case there are other similar > BIOSes. No objection about fixing Linux. > > Why do we need pci_scan_single_device()? The device should have been > scanned already when it was added, and I would think that should have > set pdev->multifunction. It should be pci_get_slot() instead. Note that it needs corresponding pci_dev_put(). Regards, Kenji Kaneshige > > Your patch needs spaces around the operators in the "for" loop. > > In the changelog, it would be nice to have the URL of a bugzilla where > the dmesg and DSDT are attached. > > Bjorn > >> Signed-off-by: Amos Kong >> --- >> drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++++--------- >> 1 files changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c >> index a70fa89..3b86d1a 100644 >> --- a/drivers/pci/hotplug/acpiphp_glue.c >> +++ b/drivers/pci/hotplug/acpiphp_glue.c >> @@ -880,6 +880,8 @@ static int disable_device(struct acpiphp_slot *slot) >> { >> struct acpiphp_func *func; >> struct pci_dev *pdev; >> + struct pci_bus *bus = slot->bridge->pci_bus; >> + int i, num = 1; >> >> /* is this slot already disabled? */ >> if (!(slot->flags& SLOT_ENABLED)) >> @@ -893,16 +895,23 @@ static int disable_device(struct acpiphp_slot *slot) >> func->bridge = NULL; >> } >> >> - pdev = pci_get_slot(slot->bridge->pci_bus, >> - PCI_DEVFN(slot->device, func->function)); >> - if (pdev) { >> - pci_stop_bus_device(pdev); >> - if (pdev->subordinate) { >> - disable_bridges(pdev->subordinate); >> - pci_disable_device(pdev); >> + pdev = pci_scan_single_device(bus, >> + PCI_DEVFN(slot->device, 0)); >> + if (!pdev) >> + goto err_exit; >> + if (pdev->multifunction == 1) >> + num = 8; >> + for (i=0; i> + pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, i)); >> + if (pdev) { >> + pci_stop_bus_device(pdev); >> + if (pdev->subordinate) { >> + disable_bridges(pdev->subordinate); >> + pci_disable_device(pdev); >> + } >> + pci_remove_bus_device(pdev); >> + pci_dev_put(pdev); >> } >> - pci_remove_bus_device(pdev); >> - pci_dev_put(pdev); >> } >> } >> >> -- >> 1.7.6.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > >