| Message ID | 1358525267-14268-8-git-send-email-jiang.liu@huawei.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote: > If user specifies "pci=nopciehp" on kernel boot command line, OSPM > won't claim PCIe native hotplug service from firmware and no PCIe > port devices will be created for PCIe native hotplug service. Why do we need this option? If I understand correctly, there are machines where it *looks* like we should use pciehp, but pciehp doesn't work because we don't get the interrupts we expect. On those machines, we have to use acpiphp instead. It seems like many Dell XPS laptops have this issue with ExpressCard slots, e.g., https://bugzilla.kernel.org/show_bug.cgi?id=40802 . If you want "pci=nopciehp" as a way for users to deal with this problem by forcing the use of acpiphp, I object. Windows manages to make these slots work without having users do anything special, so we should be able to do it, too. > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > --- > Documentation/kernel-parameters.txt | 2 ++ > drivers/acpi/pci_root.c | 3 ++- > drivers/pci/pci.c | 2 ++ > drivers/pci/pcie/portdrv_core.c | 5 +++-- > drivers/pci/pcie/portdrv_pci.c | 3 +++ > include/linux/pci.h | 9 +++++++++ > 6 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt > index 9776f06..28dd0ad 100644 > --- a/Documentation/kernel-parameters.txt > +++ b/Documentation/kernel-parameters.txt > @@ -2106,6 +2106,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted. > noaer [PCIE] If the PCIEAER kernel config parameter is > enabled, this kernel boot option can be used to > disable the use of PCIE advanced error reporting. > + nopciehp [PCIE] this kernel boot option can be used to > + disable PCIe native hotplug. > nodomains [PCI] Disable support for multiple PCI > root domains (aka PCI segments, in ACPI-speak). > nommconf [X86] Disable use of MMCONFIG for PCI > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index c6200ff..c37eedb 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -551,8 +551,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device) > if (!pcie_ports_disabled > && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) { > flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL > - | OSC_PCI_EXPRESS_NATIVE_HP_CONTROL > | OSC_PCI_EXPRESS_PME_CONTROL; > + if (!pcie_native_hotplug_disabled) > + flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL; > > if (pci_aer_available()) { > if (aer_acpi_firmware_first()) > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 2f8f4c6..34b2c83 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3869,6 +3869,8 @@ static int __init pci_setup(char *str) > pci_no_msi(); > } else if (!strcmp(str, "noaer")) { > pci_no_aer(); > + } else if (!strcmp(str, "nopciehp")) { > + pcie_no_native_hotplug(); > } else if (!strncmp(str, "realloc=", 8)) { > pci_realloc_get_opt(str + 8); > } else if (!strncmp(str, "realloc", 7)) { > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index ed129b4..e7e1679 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -263,8 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev) > > err = pcie_port_platform_notify(dev, &cap_mask); > if (!pcie_ports_auto) { > - cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP > - | PCIE_PORT_SERVICE_VC; > + cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC; > + if (!pcie_native_hotplug_disabled) > + cap_mask |= PCIE_PORT_SERVICE_HP; > if (pci_aer_available()) > cap_mask |= PCIE_PORT_SERVICE_AER; > } else if (err) { > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c > index 0761d90..018cee0 100644 > --- a/drivers/pci/pcie/portdrv_pci.c > +++ b/drivers/pci/pcie/portdrv_pci.c > @@ -40,6 +40,9 @@ bool pcie_ports_disabled; > */ > bool pcie_ports_auto = true; > > +/* If set, the PCIe native hotplug will not be used. */ > +bool pcie_native_hotplug_disabled; > + > static int __init pcie_port_setup(char *str) > { > if (!strncmp(str, "compat", 6)) { > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 12e5447..715e17b 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1138,9 +1138,18 @@ extern int pci_msi_enabled(void); > #ifdef CONFIG_PCIEPORTBUS > extern bool pcie_ports_disabled; > extern bool pcie_ports_auto; > +extern bool pcie_native_hotplug_disabled; > + > +static inline void pcie_no_native_hotplug(void) > +{ > + pcie_native_hotplug_disabled = true; > +} > #else > #define pcie_ports_disabled true > #define pcie_ports_auto false > +#define pcie_native_hotplug_disabled true > + > +static inline void pcie_no_native_hotplug(void) { } > #endif > > #ifndef CONFIG_PCIEASPM > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 18, 2013 at 9:35 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > If you want "pci=nopciehp" as a way for users to deal with this > problem by forcing the use of acpiphp, I object. Windows manages to > make these slots work without having users do anything special, so we > should be able to do it, too. Agreed. We need to think that more. I think that we should fix acpiphp, and should follow first come and first serve for acpiphp and pciehp. Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, January 18, 2013 10:35:46 AM Bjorn Helgaas wrote: > On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote: > > If user specifies "pci=nopciehp" on kernel boot command line, OSPM > > won't claim PCIe native hotplug service from firmware and no PCIe > > port devices will be created for PCIe native hotplug service. > > Why do we need this option? > > If I understand correctly, there are machines where it *looks* like we > should use pciehp, but pciehp doesn't work because we don't get the > interrupts we expect. On those machines, we have to use acpiphp > instead. It seems like many Dell XPS laptops have this issue with > ExpressCard slots, e.g., > https://bugzilla.kernel.org/show_bug.cgi?id=40802 . > > If you want "pci=nopciehp" as a way for users to deal with this > problem by forcing the use of acpiphp, I object. Windows manages to > make these slots work without having users do anything special, so we > should be able to do it, too. And one can use pcie_ports=compat to work around these problems anyway. Thanks, Rafael
On Friday, January 18, 2013 09:50:59 AM Yinghai Lu wrote: > On Fri, Jan 18, 2013 at 9:35 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > If you want "pci=nopciehp" as a way for users to deal with this > > problem by forcing the use of acpiphp, I object. Windows manages to > > make these slots work without having users do anything special, so we > > should be able to do it, too. > > Agreed. > > We need to think that more. > > I think that we should fix acpiphp, and should follow first come and > first serve for acpiphp and pciehp. That would introduce regressions for some users, though. I actually think we should merge acpiphp with pciehp and make one driver that can use both types of signalling. There would be a problem with getting notifications for the same event from both sources, but that doesn't seem to be unsolvable. And I don't think that that one combined driver should be modular. Thanks, Rafael
? 2013-01-19 1:35, Bjorn Helgaas ??: > On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote: >> If user specifies "pci=nopciehp" on kernel boot command line, OSPM >> won't claim PCIe native hotplug service from firmware and no PCIe >> port devices will be created for PCIe native hotplug service. > > Why do we need this option? > > If I understand correctly, there are machines where it *looks* like we > should use pciehp, but pciehp doesn't work because we don't get the > interrupts we expect. On those machines, we have to use acpiphp > instead. It seems like many Dell XPS laptops have this issue with > ExpressCard slots, e.g., > https://bugzilla.kernel.org/show_bug.cgi?id=40802 . What about use modprobe pciehp pciehp_poll_mode=1? If just cannot receive the interrupt, maybe this module parameter will fix it. > > If you want "pci=nopciehp" as a way for users to deal with this > problem by forcing the use of acpiphp, I object. Windows manages to > make these slots work without having users do anything special, so we > should be able to do it, too. In fact, pcie native hotplug may not be implemented perfectly, for example, I found some x86 machines pcie native hotplug slots that have problems with latch register. After inserted a pci card, the latch still report latch open state. So we cannot use pciehp to hotplug, But this problem cannot detect while system bootup or load pciehp module. And now kernel force to use pciehp if pci slotcap has hotplug capable. So I agree as Yinghai said users should have choice to choose hotplug module to use. > >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> --- >> Documentation/kernel-parameters.txt | 2 ++ >> drivers/acpi/pci_root.c | 3 ++- >> drivers/pci/pci.c | 2 ++ >> drivers/pci/pcie/portdrv_core.c | 5 +++-- >> drivers/pci/pcie/portdrv_pci.c | 3 +++ >> include/linux/pci.h | 9 +++++++++ >> 6 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt >> index 9776f06..28dd0ad 100644 >> --- a/Documentation/kernel-parameters.txt >> +++ b/Documentation/kernel-parameters.txt >> @@ -2106,6 +2106,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted. >> noaer [PCIE] If the PCIEAER kernel config parameter is >> enabled, this kernel boot option can be used to >> disable the use of PCIE advanced error reporting. >> + nopciehp [PCIE] this kernel boot option can be used to >> + disable PCIe native hotplug. >> nodomains [PCI] Disable support for multiple PCI >> root domains (aka PCI segments, in ACPI-speak). >> nommconf [X86] Disable use of MMCONFIG for PCI >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index c6200ff..c37eedb 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -551,8 +551,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device) >> if (!pcie_ports_disabled >> && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) { >> flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL >> - | OSC_PCI_EXPRESS_NATIVE_HP_CONTROL >> | OSC_PCI_EXPRESS_PME_CONTROL; >> + if (!pcie_native_hotplug_disabled) >> + flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL; >> >> if (pci_aer_available()) { >> if (aer_acpi_firmware_first()) >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 2f8f4c6..34b2c83 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3869,6 +3869,8 @@ static int __init pci_setup(char *str) >> pci_no_msi(); >> } else if (!strcmp(str, "noaer")) { >> pci_no_aer(); >> + } else if (!strcmp(str, "nopciehp")) { >> + pcie_no_native_hotplug(); >> } else if (!strncmp(str, "realloc=", 8)) { >> pci_realloc_get_opt(str + 8); >> } else if (!strncmp(str, "realloc", 7)) { >> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c >> index ed129b4..e7e1679 100644 >> --- a/drivers/pci/pcie/portdrv_core.c >> +++ b/drivers/pci/pcie/portdrv_core.c >> @@ -263,8 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev) >> >> err = pcie_port_platform_notify(dev, &cap_mask); >> if (!pcie_ports_auto) { >> - cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP >> - | PCIE_PORT_SERVICE_VC; >> + cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC; >> + if (!pcie_native_hotplug_disabled) >> + cap_mask |= PCIE_PORT_SERVICE_HP; >> if (pci_aer_available()) >> cap_mask |= PCIE_PORT_SERVICE_AER; >> } else if (err) { >> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c >> index 0761d90..018cee0 100644 >> --- a/drivers/pci/pcie/portdrv_pci.c >> +++ b/drivers/pci/pcie/portdrv_pci.c >> @@ -40,6 +40,9 @@ bool pcie_ports_disabled; >> */ >> bool pcie_ports_auto = true; >> >> +/* If set, the PCIe native hotplug will not be used. */ >> +bool pcie_native_hotplug_disabled; >> + >> static int __init pcie_port_setup(char *str) >> { >> if (!strncmp(str, "compat", 6)) { >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 12e5447..715e17b 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -1138,9 +1138,18 @@ extern int pci_msi_enabled(void); >> #ifdef CONFIG_PCIEPORTBUS >> extern bool pcie_ports_disabled; >> extern bool pcie_ports_auto; >> +extern bool pcie_native_hotplug_disabled; >> + >> +static inline void pcie_no_native_hotplug(void) >> +{ >> + pcie_native_hotplug_disabled = true; >> +} >> #else >> #define pcie_ports_disabled true >> #define pcie_ports_auto false >> +#define pcie_native_hotplug_disabled true >> + >> +static inline void pcie_no_native_hotplug(void) { } >> #endif >> >> #ifndef CONFIG_PCIEASPM >> -- >> 1.7.9.5 >> > -- > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 19, 2013 at 09:56:24AM +0800, Yijing Wang wrote: > ? 2013-01-19 1:35, Bjorn Helgaas ??: > > On Fri, Jan 18, 2013 at 9:07 AM, Jiang Liu <liuj97@gmail.com> wrote: > >> If user specifies "pci=nopciehp" on kernel boot command line, OSPM > >> won't claim PCIe native hotplug service from firmware and no PCIe > >> port devices will be created for PCIe native hotplug service. > > > > Why do we need this option? > > > > If I understand correctly, there are machines where it *looks* like we > > should use pciehp, but pciehp doesn't work because we don't get the > > interrupts we expect. On those machines, we have to use acpiphp > > instead. It seems like many Dell XPS laptops have this issue with > > ExpressCard slots, e.g., > > https://bugzilla.kernel.org/show_bug.cgi?id=40802 . > > What about use modprobe pciehp pciehp_poll_mode=1? > If just cannot receive the interrupt, maybe this module parameter will fix it. You can't add a new option that you now force hardware that was working with a different module to now define and use. It needs to be "automatic". > > If you want "pci=nopciehp" as a way for users to deal with this > > problem by forcing the use of acpiphp, I object. Windows manages to > > make these slots work without having users do anything special, so we > > should be able to do it, too. > > In fact, pcie native hotplug may not be implemented perfectly, Oh, we know that is true, it's the problem here. Fixing BIOSes would be nice, but we can't do that :( greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Yinghai and Rafael, Thanks for your comments, seems you all don't like the proposed solution, Then how about this solution: 1) keep acpiphp and pciehp as modules 2) introduce a new interface pci_for_each_bus() to walk all PCI buses. 3) use pci_for_each_bus() to replace ACPI PCI subdriver for acpiphp. 4) use PCI bus notifier chain to support PCI device and host bridge hotplug. 5) add some quirks to handle PCIe bridges having issues with native PCIe hotplug. Regards! Gerry On 01/19/2013 06:08 AM, Rafael J. Wysocki wrote: > On Friday, January 18, 2013 09:50:59 AM Yinghai Lu wrote: >> On Fri, Jan 18, 2013 at 9:35 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> If you want "pci=nopciehp" as a way for users to deal with this >>> problem by forcing the use of acpiphp, I object. Windows manages to >>> make these slots work without having users do anything special, so we >>> should be able to do it, too. >> >> Agreed. >> >> We need to think that more. >> >> I think that we should fix acpiphp, and should follow first come and >> first serve for acpiphp and pciehp. > > That would introduce regressions for some users, though. > > I actually think we should merge acpiphp with pciehp and make one driver that > can use both types of signalling. There would be a problem with getting > notifications for the same event from both sources, but that doesn't seem to > be unsolvable. > > And I don't think that that one combined driver should be modular. > > Thanks, > Rafael > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 9776f06..28dd0ad 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -2106,6 +2106,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted. noaer [PCIE] If the PCIEAER kernel config parameter is enabled, this kernel boot option can be used to disable the use of PCIE advanced error reporting. + nopciehp [PCIE] this kernel boot option can be used to + disable PCIe native hotplug. nodomains [PCI] Disable support for multiple PCI root domains (aka PCI segments, in ACPI-speak). nommconf [X86] Disable use of MMCONFIG for PCI diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index c6200ff..c37eedb 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -551,8 +551,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device) if (!pcie_ports_disabled && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) { flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL - | OSC_PCI_EXPRESS_NATIVE_HP_CONTROL | OSC_PCI_EXPRESS_PME_CONTROL; + if (!pcie_native_hotplug_disabled) + flags |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL; if (pci_aer_available()) { if (aer_acpi_firmware_first()) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 2f8f4c6..34b2c83 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3869,6 +3869,8 @@ static int __init pci_setup(char *str) pci_no_msi(); } else if (!strcmp(str, "noaer")) { pci_no_aer(); + } else if (!strcmp(str, "nopciehp")) { + pcie_no_native_hotplug(); } else if (!strncmp(str, "realloc=", 8)) { pci_realloc_get_opt(str + 8); } else if (!strncmp(str, "realloc", 7)) { diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index ed129b4..e7e1679 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -263,8 +263,9 @@ static int get_port_device_capability(struct pci_dev *dev) err = pcie_port_platform_notify(dev, &cap_mask); if (!pcie_ports_auto) { - cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP - | PCIE_PORT_SERVICE_VC; + cap_mask = PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_VC; + if (!pcie_native_hotplug_disabled) + cap_mask |= PCIE_PORT_SERVICE_HP; if (pci_aer_available()) cap_mask |= PCIE_PORT_SERVICE_AER; } else if (err) { diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c index 0761d90..018cee0 100644 --- a/drivers/pci/pcie/portdrv_pci.c +++ b/drivers/pci/pcie/portdrv_pci.c @@ -40,6 +40,9 @@ bool pcie_ports_disabled; */ bool pcie_ports_auto = true; +/* If set, the PCIe native hotplug will not be used. */ +bool pcie_native_hotplug_disabled; + static int __init pcie_port_setup(char *str) { if (!strncmp(str, "compat", 6)) { diff --git a/include/linux/pci.h b/include/linux/pci.h index 12e5447..715e17b 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1138,9 +1138,18 @@ extern int pci_msi_enabled(void); #ifdef CONFIG_PCIEPORTBUS extern bool pcie_ports_disabled; extern bool pcie_ports_auto; +extern bool pcie_native_hotplug_disabled; + +static inline void pcie_no_native_hotplug(void) +{ + pcie_native_hotplug_disabled = true; +} #else #define pcie_ports_disabled true #define pcie_ports_auto false +#define pcie_native_hotplug_disabled true + +static inline void pcie_no_native_hotplug(void) { } #endif #ifndef CONFIG_PCIEASPM
If user specifies "pci=nopciehp" on kernel boot command line, OSPM won't claim PCIe native hotplug service from firmware and no PCIe port devices will be created for PCIe native hotplug service. Signed-off-by: Jiang Liu <jiang.liu@huawei.com> --- Documentation/kernel-parameters.txt | 2 ++ drivers/acpi/pci_root.c | 3 ++- drivers/pci/pci.c | 2 ++ drivers/pci/pcie/portdrv_core.c | 5 +++-- drivers/pci/pcie/portdrv_pci.c | 3 +++ include/linux/pci.h | 9 +++++++++ 6 files changed, 21 insertions(+), 3 deletions(-)