Re: [RFC Patch V2 15/16] pci, ioapic: kill ioapic PCI driver

[Jiang Liu]

On 2014/6/18 0:49, Bjorn Helgaas wrote:
> Your subject lines aren't even consistent in the same patch series.  I suggest:
> 
>   PCI: Remove PCI ioapic driver
> 
> On Mon, Jun 16, 2014 at 11:29 PM, Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx> wrote:
>> Originally the ioapic PCI driver is designed to support IOAPIC hotplug,
>> but it never works because no architecture has implemented required
>> interface to support it.
> 
> Please be specific about what these "required interfaces" are that no
> architecture has implemented.  Otherwise it's hard for me to be
> convinced that it's safe to remove this.
> 
>> We plan to reimplement it as an ACPI driver
>> instead of PCI driver due to:
>> 1) To support IOAPIC hotplug, an IOAPIC unit must have an companion
>>    ACPI device, but it may or may not be presented in PCI domain.
> 
> Please include the reason why there must be a companion ACPI device.
> 
> What about hotplugging IOAPICs on non-ACPI systems?  How would that
> work?  Will that require totally separate IOAPIC drivers?  I guess
> pci/ioapic.c isn't very big and contains mostly ACPI stuff, so maybe
> there's nothing worth sharing.
> 
> If there must be a companion ACPI device, I guess that implies that we
> can't support an IOAPIC on a plug-in card, because there won't be
> anything in the ACPI namespace about the card.  Is it possible to have
> an IOAPIC on a plug-in card?  I guess I don't know what we could do
> with it, since I don't know how we would learn about what is connected
> to the IOAPIC input pins.  There's no standard for describing that for
> plug-in IOAPICs, is there?
Hi Bjorn,
	As I know, ACPI is the only specification defining interfaces
to support IOAPIC hotplug on x86 and IA64, and seems IOAPIC is only used
on x86 and IA64 too.
	I suspect that there's a real product with IOAPIC on plug-in
card. Correct me if I'm wrong. If there are products with IOAPIC on
plug-in card, it could only be  supported by ACPIPHP because SHPC and
PCIe native hotplug have no  chance to update ACPI data structures.

Thanks!
Gerry

> 
>> 2) All other PCI devices have dependency on IOAPIC, so we must hot-add
>>    it before all other PCI devices and hot-remove it after all other
>>    PCI devices. So we need to explicitly control the order to add/remove
>>    IOAPIC devices.
>>
>> So kill the ioapic PCI driver and will reimplement it as an ACPI driver.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/pci/Kconfig  |    7 ---
>>  drivers/pci/Makefile |    2 -
>>  drivers/pci/ioapic.c |  121 --------------------------------------------------
>>  3 files changed, 130 deletions(-)
>>  delete mode 100644 drivers/pci/ioapic.c
>>
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index 893503fa1782..39866d18004e 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -104,13 +104,6 @@ config PCI_PASID
>>
>>           If unsure, say N.
>>
>> -config PCI_IOAPIC
>> -       bool "PCI IO-APIC hotplug support" if X86
>> -       depends on PCI
>> -       depends on ACPI
>> -       depends on X86_IO_APIC
>> -       default !X86
>> -
>>  config PCI_LABEL
>>         def_bool y if (DMI || ACPI)
>>         select NLS
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index e04fe2d9df3b..73e4af400a5a 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -13,8 +13,6 @@ obj-$(CONFIG_PCI_QUIRKS) += quirks.o
>>  # Build PCI Express stuff if needed
>>  obj-$(CONFIG_PCIEPORTBUS) += pcie/
>>
>> -obj-$(CONFIG_PCI_IOAPIC) += ioapic.o
>> -
>>  # Build the PCI Hotplug drivers if we were asked to
>>  obj-$(CONFIG_HOTPLUG_PCI) += hotplug/
>>  ifdef CONFIG_HOTPLUG_PCI
>> diff --git a/drivers/pci/ioapic.c b/drivers/pci/ioapic.c
>> deleted file mode 100644
>> index 6b2b7dddbbdb..000000000000
>> --- a/drivers/pci/ioapic.c
>> +++ /dev/null
>> @@ -1,121 +0,0 @@
>> -/*
>> - * IOAPIC/IOxAPIC/IOSAPIC driver
>> - *
>> - * Copyright (C) 2009 Fujitsu Limited.
>> - * (c) Copyright 2009 Hewlett-Packard Development Company, L.P.
>> - *
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License version 2 as
>> - * published by the Free Software Foundation.
>> - */
>> -
>> -/*
>> - * This driver manages PCI I/O APICs added by hotplug after boot.  We try to
>> - * claim all I/O APIC PCI devices, but those present at boot were registered
>> - * when we parsed the ACPI MADT, so we'll fail when we try to re-register
>> - * them.
>> - */
>> -
>> -#include <linux/pci.h>
>> -#include <linux/module.h>
>> -#include <linux/acpi.h>
>> -#include <linux/slab.h>
>> -
>> -struct ioapic {
>> -       acpi_handle     handle;
>> -       u32             gsi_base;
>> -};
>> -
>> -static int ioapic_probe(struct pci_dev *dev, const struct pci_device_id *ent)
>> -{
>> -       acpi_handle handle;
>> -       acpi_status status;
>> -       unsigned long long gsb;
>> -       struct ioapic *ioapic;
>> -       int ret;
>> -       char *type;
>> -       struct resource *res;
>> -
>> -       handle = ACPI_HANDLE(&dev->dev);
>> -       if (!handle)
>> -               return -EINVAL;
>> -
>> -       status = acpi_evaluate_integer(handle, "_GSB", NULL, &gsb);
>> -       if (ACPI_FAILURE(status))
>> -               return -EINVAL;
>> -
>> -       /*
>> -        * The previous code in acpiphp evaluated _MAT if _GSB failed, but
>> -        * ACPI spec 4.0 sec 6.2.2 requires _GSB for hot-pluggable I/O APICs.
>> -        */
>> -
>> -       ioapic = kzalloc(sizeof(*ioapic), GFP_KERNEL);
>> -       if (!ioapic)
>> -               return -ENOMEM;
>> -
>> -       ioapic->handle = handle;
>> -       ioapic->gsi_base = (u32) gsb;
>> -
>> -       if (dev->class == PCI_CLASS_SYSTEM_PIC_IOAPIC)
>> -               type = "IOAPIC";
>> -       else
>> -               type = "IOxAPIC";
>> -
>> -       ret = pci_enable_device(dev);
>> -       if (ret < 0)
>> -               goto exit_free;
>> -
>> -       pci_set_master(dev);
>> -
>> -       if (pci_request_region(dev, 0, type))
>> -               goto exit_disable;
>> -
>> -       res = &dev->resource[0];
>> -       if (acpi_register_ioapic(ioapic->handle, res->start, ioapic->gsi_base))
>> -               goto exit_release;
>> -
>> -       pci_set_drvdata(dev, ioapic);
>> -       dev_info(&dev->dev, "%s at %pR, GSI %u\n", type, res, ioapic->gsi_base);
>> -       return 0;
>> -
>> -exit_release:
>> -       pci_release_region(dev, 0);
>> -exit_disable:
>> -       pci_disable_device(dev);
>> -exit_free:
>> -       kfree(ioapic);
>> -       return -ENODEV;
>> -}
>> -
>> -static void ioapic_remove(struct pci_dev *dev)
>> -{
>> -       struct ioapic *ioapic = pci_get_drvdata(dev);
>> -
>> -       acpi_unregister_ioapic(ioapic->handle, ioapic->gsi_base);
>> -       pci_release_region(dev, 0);
>> -       pci_disable_device(dev);
>> -       kfree(ioapic);
>> -}
>> -
>> -
>> -static DEFINE_PCI_DEVICE_TABLE(ioapic_devices) = {
>> -       { PCI_DEVICE_CLASS(PCI_CLASS_SYSTEM_PIC_IOAPIC, ~0) },
>> -       { PCI_DEVICE_CLASS(PCI_CLASS_SYSTEM_PIC_IOXAPIC, ~0) },
>> -       { }
>> -};
>> -MODULE_DEVICE_TABLE(pci, ioapic_devices);
>> -
>> -static struct pci_driver ioapic_driver = {
>> -       .name           = "ioapic",
>> -       .id_table       = ioapic_devices,
>> -       .probe          = ioapic_probe,
>> -       .remove         = ioapic_remove,
>> -};
>> -
>> -static int __init ioapic_init(void)
>> -{
>> -       return pci_register_driver(&ioapic_driver);
>> -}
>> -module_init(ioapic_init);
>> -
>> -MODULE_LICENSE("GPL");
>> --
>> 1.7.10.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/