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

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

(2011/09/16 4:03), Bjorn Helgaas wrote:
> On Tue, Sep 13, 2011 at 10:55 PM, Amos Kong<akong@redhat.com>  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<akong@redhat.com>
>> ---
>>   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);
>>                 }
>>         }
>>
>> --
>> 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
>>
>
>