| Message ID | 1360351703-20571-26-git-send-email-yinghai@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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); }
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(+)