| Message ID | 20180528124756.78512-3-mika.westerberg@linux.intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug | expand |
On Monday, May 28, 2018 2:47:51 PM CEST Mika Westerberg wrote: > In the same way we do for pciehp, introduce a new function > shpchp_is_native() that returns true whether the bridge should be > handled by the native SHCP driver. Then convert the driver to use this > function. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/hotplug/acpi_pcihp.c | 4 ++-- > drivers/pci/hotplug/shpchp_core.c | 2 -- > drivers/pci/pci-acpi.c | 21 +++++++++++++++++++++ > include/linux/pci_hotplug.h | 2 ++ > 4 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > index 597d22aeefc1..3979f89b250a 100644 > --- a/drivers/pci/hotplug/acpi_pcihp.c > +++ b/drivers/pci/hotplug/acpi_pcihp.c > @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev) > * OSHP within the scope of the hotplug controller and its parents, > * up to the host bridge under which this controller exists. > */ > - host = pci_find_host_bridge(pdev->bus); > - if (host->native_shpc_hotplug) > + if (shpchp_is_native(pdev)) > return 0; > > /* If _OSC exists, we should not evaluate OSHP */ > + host = pci_find_host_bridge(pdev->bus); > root = acpi_pci_find_root(ACPI_HANDLE(&host->dev)); > if (root->osc_support_set) > goto no_control; > diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c > index 47decc9b3bb3..0f3711404c2b 100644 > --- a/drivers/pci/hotplug/shpchp_core.c > +++ b/drivers/pci/hotplug/shpchp_core.c > @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev) > if (dev->vendor == PCI_VENDOR_ID_AMD && > dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) > return 1; > - if (!pci_find_capability(dev, PCI_CAP_ID_SHPC)) > - return 0; > if (acpi_get_hp_hw_control_from_firmware(dev)) > return 0; > return 1; > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 52b8434d4d6e..bb83be0d0e5b 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge) > return host->native_pcie_hotplug; > } > > +/** > + * shpchp_is_native - Check whether a hotplug port is handled by the OS > + * @bridge: Hotplug port to check > + * > + * Returns true if the given @bridge is handled by the native SHPC hotplug > + * driver. > + */ > +bool shpchp_is_native(struct pci_dev *bridge) > +{ > + const struct pci_host_bridge *host; > + > + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > + return false; > + > + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) > + return false; > + > + host = pci_find_host_bridge(bridge->bus); > + return host->native_shpc_hotplug; > +} > + > /** > * pci_acpi_wake_bus - Root bus wakeup notification fork function. > * @context: Device wakeup context. > diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h > index 1f5c935eb0de..4c378368215c 100644 > --- a/include/linux/pci_hotplug.h > +++ b/include/linux/pci_hotplug.h > @@ -164,6 +164,7 @@ struct hotplug_params { > int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp); > bool pciehp_is_native(struct pci_dev *bridge); > int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge); > +bool shpchp_is_native(struct pci_dev *bridge); > int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle); > int acpi_pci_detect_ejectable(acpi_handle handle); > #else > @@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge) > return 0; > } > static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; } > +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; } > #endif > #endif >
On Mon, 2018-05-28 at 15:47 +0300, Mika Westerberg wrote: > In the same way we do for pciehp, introduce a new function > shpchp_is_native() that returns true whether the bridge should be > handled by the native SHCP driver. Then convert the driver to use this > function. > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/pci/hotplug/acpi_pcihp.c | 4 ++-- > drivers/pci/hotplug/shpchp_core.c | 2 -- > drivers/pci/pci-acpi.c | 21 +++++++++++++++++++++ > include/linux/pci_hotplug.h | 2 ++ > 4 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c > b/drivers/pci/hotplug/acpi_pcihp.c > index 597d22aeefc1..3979f89b250a 100644 > --- a/drivers/pci/hotplug/acpi_pcihp.c > +++ b/drivers/pci/hotplug/acpi_pcihp.c > @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct > pci_dev *pdev) > * OSHP within the scope of the hotplug controller and its > parents, > * up to the host bridge under which this controller exists. > */ > - host = pci_find_host_bridge(pdev->bus); > - if (host->native_shpc_hotplug) > + if (shpchp_is_native(pdev)) > return 0; > > /* If _OSC exists, we should not evaluate OSHP */ > + host = pci_find_host_bridge(pdev->bus); > root = acpi_pci_find_root(ACPI_HANDLE(&host->dev)); > if (root->osc_support_set) > goto no_control; > diff --git a/drivers/pci/hotplug/shpchp_core.c > b/drivers/pci/hotplug/shpchp_core.c > index 47decc9b3bb3..0f3711404c2b 100644 > --- a/drivers/pci/hotplug/shpchp_core.c > +++ b/drivers/pci/hotplug/shpchp_core.c > @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev) > if (dev->vendor == PCI_VENDOR_ID_AMD && > dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) > return 1; > - if (!pci_find_capability(dev, PCI_CAP_ID_SHPC)) > - return 0; > if (acpi_get_hp_hw_control_from_firmware(dev)) > return 0; > return 1; > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 52b8434d4d6e..bb83be0d0e5b 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge) > return host->native_pcie_hotplug; > } > > +/** > + * shpchp_is_native - Check whether a hotplug port is handled by the > OS > + * @bridge: Hotplug port to check > + * > + * Returns true if the given @bridge is handled by the native SHPC > hotplug > + * driver. > + */ > +bool shpchp_is_native(struct pci_dev *bridge) > +{ > + const struct pci_host_bridge *host; > + > + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > + return false; > + > + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) > + return false; > + > + host = pci_find_host_bridge(bridge->bus); > + return host->native_shpc_hotplug; > +} > + > /** > * pci_acpi_wake_bus - Root bus wakeup notification fork function. > * @context: Device wakeup context. > diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h > index 1f5c935eb0de..4c378368215c 100644 > --- a/include/linux/pci_hotplug.h > +++ b/include/linux/pci_hotplug.h > @@ -164,6 +164,7 @@ struct hotplug_params { > int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params > *hpp); > bool pciehp_is_native(struct pci_dev *bridge); > int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge); > +bool shpchp_is_native(struct pci_dev *bridge); > int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle > handle); > int acpi_pci_detect_ejectable(acpi_handle handle); > #else > @@ -178,5 +179,6 @@ static inline int > acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge) > return 0; > } > static inline bool pciehp_is_native(struct pci_dev *bridge) { return > true; } > +static inline bool shpchp_is_native(struct pci_dev *bridge) { return > true; } > #endif > #endif
[+cc David] On Mon, May 28, 2018 at 03:47:51PM +0300, Mika Westerberg wrote: > In the same way we do for pciehp, introduce a new function > shpchp_is_native() that returns true whether the bridge should be > handled by the native SHCP driver. Then convert the driver to use this > function. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/pci/hotplug/acpi_pcihp.c | 4 ++-- > drivers/pci/hotplug/shpchp_core.c | 2 -- > drivers/pci/pci-acpi.c | 21 +++++++++++++++++++++ > include/linux/pci_hotplug.h | 2 ++ > 4 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > index 597d22aeefc1..3979f89b250a 100644 > --- a/drivers/pci/hotplug/acpi_pcihp.c > +++ b/drivers/pci/hotplug/acpi_pcihp.c > @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev) > * OSHP within the scope of the hotplug controller and its parents, > * up to the host bridge under which this controller exists. > */ > - host = pci_find_host_bridge(pdev->bus); > - if (host->native_shpc_hotplug) > + if (shpchp_is_native(pdev)) > return 0; > > /* If _OSC exists, we should not evaluate OSHP */ > + host = pci_find_host_bridge(pdev->bus); > root = acpi_pci_find_root(ACPI_HANDLE(&host->dev)); > if (root->osc_support_set) > goto no_control; > diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c > index 47decc9b3bb3..0f3711404c2b 100644 > --- a/drivers/pci/hotplug/shpchp_core.c > +++ b/drivers/pci/hotplug/shpchp_core.c > @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev) > if (dev->vendor == PCI_VENDOR_ID_AMD && > dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) > return 1; > - if (!pci_find_capability(dev, PCI_CAP_ID_SHPC)) > - return 0; > if (acpi_get_hp_hw_control_from_firmware(dev)) > return 0; > return 1; > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 52b8434d4d6e..bb83be0d0e5b 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge) > return host->native_pcie_hotplug; > } > > +/** > + * shpchp_is_native - Check whether a hotplug port is handled by the OS > + * @bridge: Hotplug port to check > + * > + * Returns true if the given @bridge is handled by the native SHPC hotplug > + * driver. > + */ > +bool shpchp_is_native(struct pci_dev *bridge) > +{ > + const struct pci_host_bridge *host; > + > + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > + return false; > + > + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) > + return false; > + > + host = pci_find_host_bridge(bridge->bus); > + return host->native_shpc_hotplug; > +} This is sort-of-but-not-exactly the same as is_shpc_capable(). For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true and shpchp will claim the device, but shpchp_is_native() will return false because there's no SHPC capability. At least that's my interpretation of the AMD GOLAM stuff. I don't have a spec for it, but if GOLAM did have an SHPC capability, I assume is_shpc_capable() would look for it *before* testing for GOLAM. So I think these two things need to be reconciled somehow. I mentioned this before, but it was buried at the bottom of a long email, sorry about that. I wish we had a spec or details about the erratum. It looks like it's been there "forever": https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/hotplug/shpchp.h?id=c16b4b14d9806e639f4afefa2d651a857a212afe But neither GOLAM (0x7450) nor POGO (0x7458) is in the PCI database at https://pci-ids.ucw.cz/read/PC/1002, so who knows if those chips ever even saw the light of day. I'll cc the author of 53044f357448 ("[PATCH] PCI Hotplug: shpchp: AMD POGO errata fix") But that was 12 years ago, so the email address may not even work any more. > /** > * pci_acpi_wake_bus - Root bus wakeup notification fork function. > * @context: Device wakeup context. > diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h > index 1f5c935eb0de..4c378368215c 100644 > --- a/include/linux/pci_hotplug.h > +++ b/include/linux/pci_hotplug.h > @@ -164,6 +164,7 @@ struct hotplug_params { > int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp); > bool pciehp_is_native(struct pci_dev *bridge); > int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge); > +bool shpchp_is_native(struct pci_dev *bridge); > int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle); > int acpi_pci_detect_ejectable(acpi_handle handle); > #else > @@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge) > return 0; > } > static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; } > +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; } > #endif > #endif > -- > 2.17.0 >
[-cc David, his email bounced] On Wed, May 30, 2018 at 03:23:43PM -0500, Bjorn Helgaas wrote: > [+cc David] > > On Mon, May 28, 2018 at 03:47:51PM +0300, Mika Westerberg wrote: > > In the same way we do for pciehp, introduce a new function > > shpchp_is_native() that returns true whether the bridge should be > > handled by the native SHCP driver. Then convert the driver to use this > > function. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > drivers/pci/hotplug/acpi_pcihp.c | 4 ++-- > > drivers/pci/hotplug/shpchp_core.c | 2 -- > > drivers/pci/pci-acpi.c | 21 +++++++++++++++++++++ > > include/linux/pci_hotplug.h | 2 ++ > > 4 files changed, 25 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > > index 597d22aeefc1..3979f89b250a 100644 > > --- a/drivers/pci/hotplug/acpi_pcihp.c > > +++ b/drivers/pci/hotplug/acpi_pcihp.c > > @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev) > > * OSHP within the scope of the hotplug controller and its parents, > > * up to the host bridge under which this controller exists. > > */ > > - host = pci_find_host_bridge(pdev->bus); > > - if (host->native_shpc_hotplug) > > + if (shpchp_is_native(pdev)) > > return 0; > > > > /* If _OSC exists, we should not evaluate OSHP */ > > + host = pci_find_host_bridge(pdev->bus); > > root = acpi_pci_find_root(ACPI_HANDLE(&host->dev)); > > if (root->osc_support_set) > > goto no_control; > > diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c > > index 47decc9b3bb3..0f3711404c2b 100644 > > --- a/drivers/pci/hotplug/shpchp_core.c > > +++ b/drivers/pci/hotplug/shpchp_core.c > > @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev) > > if (dev->vendor == PCI_VENDOR_ID_AMD && > > dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) > > return 1; > > - if (!pci_find_capability(dev, PCI_CAP_ID_SHPC)) > > - return 0; > > if (acpi_get_hp_hw_control_from_firmware(dev)) > > return 0; > > return 1; > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index 52b8434d4d6e..bb83be0d0e5b 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge) > > return host->native_pcie_hotplug; > > } > > > > +/** > > + * shpchp_is_native - Check whether a hotplug port is handled by the OS > > + * @bridge: Hotplug port to check > > + * > > + * Returns true if the given @bridge is handled by the native SHPC hotplug > > + * driver. > > + */ > > +bool shpchp_is_native(struct pci_dev *bridge) > > +{ > > + const struct pci_host_bridge *host; > > + > > + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > > + return false; > > + > > + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) > > + return false; > > + > > + host = pci_find_host_bridge(bridge->bus); > > + return host->native_shpc_hotplug; > > +} > > This is sort-of-but-not-exactly the same as is_shpc_capable(). > > For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true > and shpchp will claim the device, but shpchp_is_native() will return > false because there's no SHPC capability. > > At least that's my interpretation of the AMD GOLAM stuff. I don't > have a spec for it, but if GOLAM did have an SHPC capability, I assume > is_shpc_capable() would look for it *before* testing for GOLAM. > > So I think these two things need to be reconciled somehow. I > mentioned this before, but it was buried at the bottom of a long > email, sorry about that. > > I wish we had a spec or details about the erratum. It looks like > it's been there "forever": https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/hotplug/shpchp.h?id=c16b4b14d9806e639f4afefa2d651a857a212afe > > But neither GOLAM (0x7450) nor POGO (0x7458) is in the PCI database at > https://pci-ids.ucw.cz/read/PC/1002, so who knows if those chips ever > even saw the light of day. I'll cc the author of > > 53044f357448 ("[PATCH] PCI Hotplug: shpchp: AMD POGO errata fix") > > But that was 12 years ago, so the email address may not even work any > more. > > > /** > > * pci_acpi_wake_bus - Root bus wakeup notification fork function. > > * @context: Device wakeup context. > > diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h > > index 1f5c935eb0de..4c378368215c 100644 > > --- a/include/linux/pci_hotplug.h > > +++ b/include/linux/pci_hotplug.h > > @@ -164,6 +164,7 @@ struct hotplug_params { > > int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp); > > bool pciehp_is_native(struct pci_dev *bridge); > > int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge); > > +bool shpchp_is_native(struct pci_dev *bridge); > > int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle); > > int acpi_pci_detect_ejectable(acpi_handle handle); > > #else > > @@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge) > > return 0; > > } > > static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; } > > +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; } > > #endif > > #endif > > -- > > 2.17.0 > > > -- > 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 Wed, May 30, 2018 at 03:23:43PM -0500, Bjorn Helgaas wrote: > > +{ > > + const struct pci_host_bridge *host; > > + > > + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > > + return false; > > + > > + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) > > + return false; > > + > > + host = pci_find_host_bridge(bridge->bus); > > + return host->native_shpc_hotplug; > > +} > > This is sort-of-but-not-exactly the same as is_shpc_capable(). > > For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true > and shpchp will claim the device, but shpchp_is_native() will return > false because there's no SHPC capability. > > At least that's my interpretation of the AMD GOLAM stuff. I don't > have a spec for it, but if GOLAM did have an SHPC capability, I assume > is_shpc_capable() would look for it *before* testing for GOLAM. Most probably it did not have SHPC capability because I find it hard to explain the check otherwise. > So I think these two things need to be reconciled somehow. I > mentioned this before, but it was buried at the bottom of a long > email, sorry about that. No it wasn't. I read it and did exactly what you wanted. Now there is no duplication in these two functions. is_shpc_capable() calls acpi_get_hp_hw_control_from_firmware() which calls shpchp_is_native(). In fact I don't think is_shpc_capable() warrants to exist at all and it should be removed (but in another separate cleanup patch). > I wish we had a spec or details about the erratum. It looks like > it's been there "forever": https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/hotplug/shpchp.h?id=c16b4b14d9806e639f4afefa2d651a857a212afe > > But neither GOLAM (0x7450) nor POGO (0x7458) is in the PCI database at > https://pci-ids.ucw.cz/read/PC/1002, so who knows if those chips ever > even saw the light of day. I'll cc the author of > > 53044f357448 ("[PATCH] PCI Hotplug: shpchp: AMD POGO errata fix") > > But that was 12 years ago, so the email address may not even work any > more. Ín any case even if somehow the original patch from 2006 was wrong, I don't have absolutely any idea why it needs to be fixed now in this patch series? Given that there are two real fixes in this series that fix real issues on real contemporary hardware, I don't really understand why you are stalling them. Yes, it is good to do cleanups because it makes the code easier to understand and thus more maintainable but that's something typically not priorized as high as fixes for real problems. These fixes have been out there since february virtually unchanged, so you would think they have had enough review already. If not please point out what exactly I need to fix and I'll do that. Otherwise please consider taking the series for v4.18.
On Thu, May 31, 2018 at 09:58:52AM +0300, Mika Westerberg wrote: > On Wed, May 30, 2018 at 03:23:43PM -0500, Bjorn Helgaas wrote: > > > +{ > > > + const struct pci_host_bridge *host; > > > + > > > + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > > > + return false; > > > + > > > + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) > > > + return false; > > > + > > > + host = pci_find_host_bridge(bridge->bus); > > > + return host->native_shpc_hotplug; > > > +} > > > > This is sort-of-but-not-exactly the same as is_shpc_capable(). > > > > For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true > > and shpchp will claim the device, but shpchp_is_native() will return > > false because there's no SHPC capability. > > > > At least that's my interpretation of the AMD GOLAM stuff. I don't > > have a spec for it, but if GOLAM did have an SHPC capability, I assume > > is_shpc_capable() would look for it *before* testing for GOLAM. > > Most probably it did not have SHPC capability because I find it hard to > explain the check otherwise. > > > So I think these two things need to be reconciled somehow. I > > mentioned this before, but it was buried at the bottom of a long > > email, sorry about that. > > No it wasn't. I read it and did exactly what you wanted. Now there is no > duplication in these two functions. is_shpc_capable() calls > acpi_get_hp_hw_control_from_firmware() which calls shpchp_is_native(). > In fact I don't think is_shpc_capable() warrants to exist at all and it > should be removed (but in another separate cleanup patch). Maybe I'm reading your patches wrong. It looks to me like shpchp will claim GOLAM_7450, which means shpchp will register slots, program the SHPC, handle hotplug interrupts, etc. But since shpchp_is_native() returns false, acpiphp thinks *it* should handle hotplug. For example, I think that given some ACPI prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register(): shpc_probe is_shpc_capable # true for GOLAM_7450 init_slots pci_hp_register acpi_pci_add_slots acpiphp_enumerate_slots acpi_walk_namespace(..., acpiphp_add_context) acpiphp_add_context hotplug_is_native # false for GOLAM_7450 acpiphp_register_hotplug_slot pci_hp_register It is true that the same situation occurred before your patches, since acpiphp_add_context() only checked pciehp_is_native(). In fact, with the existing code, shpchp and acpiphp could both try to manage *any* SHPC, not just GOLAM_7450. I think the current series fixes 99% of that problem and it seems like we should try to do that last 1% at the same time so the SHPC code makes more sense. Bjorn
On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote: > Maybe I'm reading your patches wrong. It looks to me like shpchp will > claim GOLAM_7450, which means shpchp will register slots, program the > SHPC, handle hotplug interrupts, etc. > > But since shpchp_is_native() returns false, acpiphp thinks *it* should > handle hotplug. For example, I think that given some ACPI > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register(): > > shpc_probe > is_shpc_capable # true for GOLAM_7450 > init_slots > pci_hp_register > > acpi_pci_add_slots > acpiphp_enumerate_slots > acpi_walk_namespace(..., acpiphp_add_context) > acpiphp_add_context > hotplug_is_native # false for GOLAM_7450 > acpiphp_register_hotplug_slot > pci_hp_register > > It is true that the same situation occurred before your patches, since > acpiphp_add_context() only checked pciehp_is_native(). In fact, with > the existing code, shpchp and acpiphp could both try to manage *any* > SHPC, not just GOLAM_7450. > > I think the current series fixes 99% of that problem and it seems like > we should try to do that last 1% at the same time so the SHPC code > makes more sense. Would the following fix the last 1% for you? Applies on top of this patch. diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h index 9675ab757323..516e4835019c 100644 --- a/drivers/pci/hotplug/shpchp.h +++ b/drivers/pci/hotplug/shpchp.h @@ -105,7 +105,6 @@ struct controller { }; /* Define AMD SHPC ID */ -#define PCI_DEVICE_ID_AMD_GOLAM_7450 0x7450 #define PCI_DEVICE_ID_AMD_POGO_7458 0x7458 /* AMD PCI-X bridge registers */ diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c index 0f3711404c2b..e91be287f292 100644 --- a/drivers/pci/hotplug/shpchp_core.c +++ b/drivers/pci/hotplug/shpchp_core.c @@ -270,22 +270,12 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) return 0; } -static int is_shpc_capable(struct pci_dev *dev) -{ - if (dev->vendor == PCI_VENDOR_ID_AMD && - dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) - return 1; - if (acpi_get_hp_hw_control_from_firmware(dev)) - return 0; - return 1; -} - static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { int rc; struct controller *ctrl; - if (!is_shpc_capable(pdev)) + if (acpi_get_hp_hw_control_from_firmware(pdev)) return -ENODEV; ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index bbc4ea70505a..fd1c0ee33805 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -408,6 +408,14 @@ bool shpchp_is_native(struct pci_dev *bridge) if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) return false; + /* + * It is assumed that AMD GOLAM chips support SHPC but they do not + * have SHPC capability. + */ + if (bridge->vendor == PCI_VENDOR_ID_AMD && + bridge->device == PCI_DEVICE_ID_AMD_GOLAM_7450) + return true; + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) return false; diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 883cb7bf78aa..5aace6cca0d7 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -561,6 +561,7 @@ #define PCI_DEVICE_ID_AMD_OPUS_7443 0x7443 #define PCI_DEVICE_ID_AMD_VIPER_7443 0x7443 #define PCI_DEVICE_ID_AMD_OPUS_7445 0x7445 +#define PCI_DEVICE_ID_AMD_GOLAM_7450 0x7450 #define PCI_DEVICE_ID_AMD_8111_PCI 0x7460 #define PCI_DEVICE_ID_AMD_8111_LPC 0x7468 #define PCI_DEVICE_ID_AMD_8111_IDE 0x7469
On Thu, May 31, 2018 at 04:51:17PM +0300, Mika Westerberg wrote: > On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote: > > Maybe I'm reading your patches wrong. It looks to me like shpchp will > > claim GOLAM_7450, which means shpchp will register slots, program the > > SHPC, handle hotplug interrupts, etc. > > > > But since shpchp_is_native() returns false, acpiphp thinks *it* should > > handle hotplug. For example, I think that given some ACPI > > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register(): > > > > shpc_probe > > is_shpc_capable # true for GOLAM_7450 > > init_slots > > pci_hp_register > > > > acpi_pci_add_slots > > acpiphp_enumerate_slots > > acpi_walk_namespace(..., acpiphp_add_context) > > acpiphp_add_context > > hotplug_is_native # false for GOLAM_7450 > > acpiphp_register_hotplug_slot > > pci_hp_register > > > > It is true that the same situation occurred before your patches, since > > acpiphp_add_context() only checked pciehp_is_native(). In fact, with > > the existing code, shpchp and acpiphp could both try to manage *any* > > SHPC, not just GOLAM_7450. > > > > I think the current series fixes 99% of that problem and it seems like > > we should try to do that last 1% at the same time so the SHPC code > > makes more sense. > > Would the following fix the last 1% for you? Applies on top of this > patch. Yes, that's exactly what I was looking for! Thanks! > diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h > index 9675ab757323..516e4835019c 100644 > --- a/drivers/pci/hotplug/shpchp.h > +++ b/drivers/pci/hotplug/shpchp.h > @@ -105,7 +105,6 @@ struct controller { > }; > > /* Define AMD SHPC ID */ > -#define PCI_DEVICE_ID_AMD_GOLAM_7450 0x7450 > #define PCI_DEVICE_ID_AMD_POGO_7458 0x7458 > > /* AMD PCI-X bridge registers */ > diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c > index 0f3711404c2b..e91be287f292 100644 > --- a/drivers/pci/hotplug/shpchp_core.c > +++ b/drivers/pci/hotplug/shpchp_core.c > @@ -270,22 +270,12 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) > return 0; > } > > -static int is_shpc_capable(struct pci_dev *dev) > -{ > - if (dev->vendor == PCI_VENDOR_ID_AMD && > - dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) > - return 1; > - if (acpi_get_hp_hw_control_from_firmware(dev)) > - return 0; > - return 1; > -} > - > static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > { > int rc; > struct controller *ctrl; > > - if (!is_shpc_capable(pdev)) > + if (acpi_get_hp_hw_control_from_firmware(pdev)) > return -ENODEV; > > ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index bbc4ea70505a..fd1c0ee33805 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -408,6 +408,14 @@ bool shpchp_is_native(struct pci_dev *bridge) > if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > return false; > > + /* > + * It is assumed that AMD GOLAM chips support SHPC but they do not > + * have SHPC capability. > + */ > + if (bridge->vendor == PCI_VENDOR_ID_AMD && > + bridge->device == PCI_DEVICE_ID_AMD_GOLAM_7450) > + return true; > + > if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) > return false; > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index 883cb7bf78aa..5aace6cca0d7 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -561,6 +561,7 @@ > #define PCI_DEVICE_ID_AMD_OPUS_7443 0x7443 > #define PCI_DEVICE_ID_AMD_VIPER_7443 0x7443 > #define PCI_DEVICE_ID_AMD_OPUS_7445 0x7445 > +#define PCI_DEVICE_ID_AMD_GOLAM_7450 0x7450 > #define PCI_DEVICE_ID_AMD_8111_PCI 0x7460 > #define PCI_DEVICE_ID_AMD_8111_LPC 0x7468 > #define PCI_DEVICE_ID_AMD_8111_IDE 0x7469
On Thu, May 31, 2018 at 11:55:56AM -0500, Bjorn Helgaas wrote: > On Thu, May 31, 2018 at 04:51:17PM +0300, Mika Westerberg wrote: > > On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote: > > > Maybe I'm reading your patches wrong. It looks to me like shpchp will > > > claim GOLAM_7450, which means shpchp will register slots, program the > > > SHPC, handle hotplug interrupts, etc. > > > > > > But since shpchp_is_native() returns false, acpiphp thinks *it* should > > > handle hotplug. For example, I think that given some ACPI > > > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register(): > > > > > > shpc_probe > > > is_shpc_capable # true for GOLAM_7450 > > > init_slots > > > pci_hp_register > > > > > > acpi_pci_add_slots > > > acpiphp_enumerate_slots > > > acpi_walk_namespace(..., acpiphp_add_context) > > > acpiphp_add_context > > > hotplug_is_native # false for GOLAM_7450 > > > acpiphp_register_hotplug_slot > > > pci_hp_register > > > > > > It is true that the same situation occurred before your patches, since > > > acpiphp_add_context() only checked pciehp_is_native(). In fact, with > > > the existing code, shpchp and acpiphp could both try to manage *any* > > > SHPC, not just GOLAM_7450. > > > > > > I think the current series fixes 99% of that problem and it seems like > > > we should try to do that last 1% at the same time so the SHPC code > > > makes more sense. > > > > Would the following fix the last 1% for you? Applies on top of this > > patch. > > Yes, that's exactly what I was looking for! Thanks! Great. Do you want me to update this patch accordingly or will you do that yourself?
On Fri, Jun 01, 2018 at 12:27:10PM +0300, Mika Westerberg wrote: > On Thu, May 31, 2018 at 11:55:56AM -0500, Bjorn Helgaas wrote: > > On Thu, May 31, 2018 at 04:51:17PM +0300, Mika Westerberg wrote: > > > On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote: > > > > Maybe I'm reading your patches wrong. It looks to me like shpchp will > > > > claim GOLAM_7450, which means shpchp will register slots, program the > > > > SHPC, handle hotplug interrupts, etc. > > > > > > > > But since shpchp_is_native() returns false, acpiphp thinks *it* should > > > > handle hotplug. For example, I think that given some ACPI > > > > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register(): > > > > > > > > shpc_probe > > > > is_shpc_capable # true for GOLAM_7450 > > > > init_slots > > > > pci_hp_register > > > > > > > > acpi_pci_add_slots > > > > acpiphp_enumerate_slots > > > > acpi_walk_namespace(..., acpiphp_add_context) > > > > acpiphp_add_context > > > > hotplug_is_native # false for GOLAM_7450 > > > > acpiphp_register_hotplug_slot > > > > pci_hp_register > > > > > > > > It is true that the same situation occurred before your patches, since > > > > acpiphp_add_context() only checked pciehp_is_native(). In fact, with > > > > the existing code, shpchp and acpiphp could both try to manage *any* > > > > SHPC, not just GOLAM_7450. > > > > > > > > I think the current series fixes 99% of that problem and it seems like > > > > we should try to do that last 1% at the same time so the SHPC code > > > > makes more sense. > > > > > > Would the following fix the last 1% for you? Applies on top of this > > > patch. > > > > Yes, that's exactly what I was looking for! Thanks! > > Great. Do you want me to update this patch accordingly or will you do > that yourself? No need, I squashed it in already.
diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c index 597d22aeefc1..3979f89b250a 100644 --- a/drivers/pci/hotplug/acpi_pcihp.c +++ b/drivers/pci/hotplug/acpi_pcihp.c @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev) * OSHP within the scope of the hotplug controller and its parents, * up to the host bridge under which this controller exists. */ - host = pci_find_host_bridge(pdev->bus); - if (host->native_shpc_hotplug) + if (shpchp_is_native(pdev)) return 0; /* If _OSC exists, we should not evaluate OSHP */ + host = pci_find_host_bridge(pdev->bus); root = acpi_pci_find_root(ACPI_HANDLE(&host->dev)); if (root->osc_support_set) goto no_control; diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c index 47decc9b3bb3..0f3711404c2b 100644 --- a/drivers/pci/hotplug/shpchp_core.c +++ b/drivers/pci/hotplug/shpchp_core.c @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev) if (dev->vendor == PCI_VENDOR_ID_AMD && dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) return 1; - if (!pci_find_capability(dev, PCI_CAP_ID_SHPC)) - return 0; if (acpi_get_hp_hw_control_from_firmware(dev)) return 0; return 1; diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 52b8434d4d6e..bb83be0d0e5b 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge) return host->native_pcie_hotplug; } +/** + * shpchp_is_native - Check whether a hotplug port is handled by the OS + * @bridge: Hotplug port to check + * + * Returns true if the given @bridge is handled by the native SHPC hotplug + * driver. + */ +bool shpchp_is_native(struct pci_dev *bridge) +{ + const struct pci_host_bridge *host; + + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) + return false; + + if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC)) + return false; + + host = pci_find_host_bridge(bridge->bus); + return host->native_shpc_hotplug; +} + /** * pci_acpi_wake_bus - Root bus wakeup notification fork function. * @context: Device wakeup context. diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h index 1f5c935eb0de..4c378368215c 100644 --- a/include/linux/pci_hotplug.h +++ b/include/linux/pci_hotplug.h @@ -164,6 +164,7 @@ struct hotplug_params { int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp); bool pciehp_is_native(struct pci_dev *bridge); int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge); +bool shpchp_is_native(struct pci_dev *bridge); int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle); int acpi_pci_detect_ejectable(acpi_handle handle); #else @@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge) return 0; } static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; } +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; } #endif #endif
In the same way we do for pciehp, introduce a new function shpchp_is_native() that returns true whether the bridge should be handled by the native SHCP driver. Then convert the driver to use this function. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pci/hotplug/acpi_pcihp.c | 4 ++-- drivers/pci/hotplug/shpchp_core.c | 2 -- drivers/pci/pci-acpi.c | 21 +++++++++++++++++++++ include/linux/pci_hotplug.h | 2 ++ 4 files changed, 25 insertions(+), 4 deletions(-)