Re: [SeaBIOS] [PATCH] pci: clean all funcs when hot-removing multifunc device

[Bjorn Helgaas]

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.

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.

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<num; 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);
>
> }
>        }