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

[Kenji Kaneshige]

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