Patchwork [v2,25/26] PCI: Disable mem in the ioapic removing path

login
register
mail settings
Submitter Yinghai Lu
Date Feb. 8, 2013, 7:28 p.m.
Message ID <1360351703-20571-26-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/219266/
State Not Applicable
Headers show

Comments

Yinghai Lu - Feb. 8, 2013, 7:28 p.m.
For physical hot plug should be ok, but for remove/rescan path will
need us to disable that.

otherwise rescan mmio resource for pci ioapic device will not be
sized and allocated, aka skiped.
For ioapic_probe:pci_enable_device will not enable the device
correctly, and will bail out early.

So we can just disable mmio for all removing case, and that will not hurt
real hotplug path.

Signed-off-by: <yinghai@kernel.org>
---
 drivers/pci/ioapic.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)
Bjorn Helgaas - Feb. 8, 2013, 9:14 p.m.
On Fri, Feb 8, 2013 at 12:28 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> For physical hot plug should be ok, but for remove/rescan path will
> need us to disable that.
>
> otherwise rescan mmio resource for pci ioapic device will not be
> sized and allocated, aka skiped.

When we scan other PCI devices, we can size memory BARs even if
PCI_COMMAND_MEMORY is already set.  So there must be something
different about IOAPICs?  Or maybe it's something different about
rescan vs. the initial scan?

> For ioapic_probe:pci_enable_device will not enable the device
> correctly, and will bail out early.

Exactly where and why do we bail out early?  The only early bail out I
see is where __pci_enable_device_flags() returns if "dev->enable_cnt >
1".  Is that what you mean?

> So we can just disable mmio for all removing case, and that will not hurt
> real hotplug path.

> Signed-off-by: <yinghai@kernel.org>
> ---
>  drivers/pci/ioapic.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/pci/ioapic.c b/drivers/pci/ioapic.c
> index 3c6bbdd..8dacfd0 100644
> --- a/drivers/pci/ioapic.c
> +++ b/drivers/pci/ioapic.c
> @@ -88,6 +88,17 @@ exit_free:
>         return -ENODEV;
>  }
>
> +static void pci_disable_device_mem(struct pci_dev *dev)
> +{
> +       u16 pci_command;
> +
> +       pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> +       if (pci_command & PCI_COMMAND_MEMORY) {
> +               pci_command &= ~PCI_COMMAND_MEMORY;
> +               pci_write_config_word(dev, PCI_COMMAND, pci_command);
> +       }
> +}
> +
>  static void ioapic_remove(struct pci_dev *dev)
>  {
>         struct ioapic *ioapic = pci_get_drvdata(dev);
> @@ -95,6 +106,8 @@ static void ioapic_remove(struct pci_dev *dev)
>         acpi_unregister_ioapic(ioapic->handle, ioapic->gsi_base);
>         pci_release_region(dev, 0);
>         pci_disable_device(dev);
> +       /* need to disable it, otherwise remove/rescan will not work */
> +       pci_disable_device_mem(dev);
>         kfree(ioapic);
>  }
>
> --
> 1.7.10.4
>
--
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
Yinghai Lu - Feb. 8, 2013, 10:33 p.m.
On Fri, Feb 8, 2013 at 1:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Feb 8, 2013 at 12:28 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> For physical hot plug should be ok, but for remove/rescan path will
>> need us to disable that.
>>
>> otherwise rescan mmio resource for pci ioapic device will not be
>> sized and allocated, aka skiped.
>
> When we scan other PCI devices, we can size memory BARs even if
> PCI_COMMAND_MEMORY is already set.  So there must be something
> different about IOAPICs?  Or maybe it's something different about
> rescan vs. the initial scan?

it is in drivers/pci/setup-bus.c::__dev_sort_resources()
it will skip the reallocation for ioapic controller.

...
        /* Don't touch ioapic devices already enabled by firmware */
        if (class == PCI_CLASS_SYSTEM_PIC) {
                u16 command;
                pci_read_config_word(dev, PCI_COMMAND, &command);
                if (command & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
                        return;
        }

>
>> For ioapic_probe:pci_enable_device will not enable the device
>> correctly, and will bail out early.
>
> Exactly where and why do we bail out early?  The only early bail out I
> see is where __pci_enable_device_flags() returns if "dev->enable_cnt >
> 1".  Is that what you mean?

pci_enable_device==>pci_enable_device_flags
==>do_pci_enable_device==>pcibios_enable_device==>pci_enable_resources
...
                if (!r->parent) {
                        dev_err(&dev->dev, "device not available "
                                "(can't reserve %pR)\n", r);
                        return -EINVAL;
                }
...

only reassign one will have get it's parent.

Thanks

Yinghai
--
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
Bjorn Helgaas - Feb. 8, 2013, 11:18 p.m.
On Fri, Feb 8, 2013 at 3:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Feb 8, 2013 at 1:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Fri, Feb 8, 2013 at 12:28 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> For physical hot plug should be ok, but for remove/rescan path will
>>> need us to disable that.
>>>
>>> otherwise rescan mmio resource for pci ioapic device will not be
>>> sized and allocated, aka skiped.
>>
>> When we scan other PCI devices, we can size memory BARs even if
>> PCI_COMMAND_MEMORY is already set.  So there must be something
>> different about IOAPICs?  Or maybe it's something different about
>> rescan vs. the initial scan?
>
> it is in drivers/pci/setup-bus.c::__dev_sort_resources()
> it will skip the reallocation for ioapic controller.
>
> ...
>         /* Don't touch ioapic devices already enabled by firmware */
>         if (class == PCI_CLASS_SYSTEM_PIC) {
>                 u16 command;
>                 pci_read_config_word(dev, PCI_COMMAND, &command);
>                 if (command & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
>                         return;
>         }
>
>>
>>> For ioapic_probe:pci_enable_device will not enable the device
>>> correctly, and will bail out early.
>>
>> Exactly where and why do we bail out early?  The only early bail out I
>> see is where __pci_enable_device_flags() returns if "dev->enable_cnt >
>> 1".  Is that what you mean?
>
> pci_enable_device==>pci_enable_device_flags
> ==>do_pci_enable_device==>pcibios_enable_device==>pci_enable_resources
> ...
>                 if (!r->parent) {
>                         dev_err(&dev->dev, "device not available "
>                                 "(can't reserve %pR)\n", r);
>                         return -EINVAL;
>                 }
> ...
>
> only reassign one will have get it's parent.

Hmmm, OK.  So basically this patch is a hack to work around previous
hacks elsewhere.

We are like flies in a spider's web, bound tighter and tighter by
every loop of silk.
--
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

Patch

diff --git a/drivers/pci/ioapic.c b/drivers/pci/ioapic.c
index 3c6bbdd..8dacfd0 100644
--- a/drivers/pci/ioapic.c
+++ b/drivers/pci/ioapic.c
@@ -88,6 +88,17 @@  exit_free:
 	return -ENODEV;
 }
 
+static void pci_disable_device_mem(struct pci_dev *dev)
+{
+	u16 pci_command;
+
+	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
+	if (pci_command & PCI_COMMAND_MEMORY) {
+		pci_command &= ~PCI_COMMAND_MEMORY;
+		pci_write_config_word(dev, PCI_COMMAND, pci_command);
+	}
+}
+
 static void ioapic_remove(struct pci_dev *dev)
 {
 	struct ioapic *ioapic = pci_get_drvdata(dev);
@@ -95,6 +106,8 @@  static void ioapic_remove(struct pci_dev *dev)
 	acpi_unregister_ioapic(ioapic->handle, ioapic->gsi_base);
 	pci_release_region(dev, 0);
 	pci_disable_device(dev);
+	/* need to disable it, otherwise remove/rescan will not work */
+	pci_disable_device_mem(dev);
 	kfree(ioapic);
 }