diff mbox

PCI hotplug: shpchp: don't blindly claim non-AMD 0x7450 device IDs

Message ID 20110823161643.27268.89203.stgit@bhelgaas.mtv.corp.google.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bjorn Helgaas Aug. 23, 2011, 4:16 p.m. UTC
Previously we claimed device ID 0x7450, regardless of the vendor, which is
clearly wrong.  Now we'll claim that device ID only for AMD.

I suspect this was just a typo in the original code, but it's possible this
change will break shpchp on non-7450 AMD bridges.  If so, we'll have to fix
them as we find them.

Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
Reported-by: Ralf Jung <ralfjung-e@gmx.de>
Cc: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/shpchp_core.c |    4 ++--
 drivers/pci/hotplug/shpchp_hpc.c  |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)


--
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

Comments

Greg KH Aug. 23, 2011, 4:28 p.m. UTC | #1
On Tue, Aug 23, 2011 at 10:16:43AM -0600, Bjorn Helgaas wrote:
> Previously we claimed device ID 0x7450, regardless of the vendor, which is
> clearly wrong.  Now we'll claim that device ID only for AMD.
> 
> I suspect this was just a typo in the original code, but it's possible this
> change will break shpchp on non-7450 AMD bridges.  If so, we'll have to fix
> them as we find them.
> 
> Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
> Reported-by: Ralf Jung <ralfjung-e@gmx.de>
> Cc: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Cc: stable <stable@kernel.org> as well?

thanks,

greg k-h
--
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
Ralf Jung Aug. 23, 2011, 4:40 p.m. UTC | #2
Hi,

thanks for the quick reaction, this is awesome!

Kind regards,
Ralf

On Tuesday 23 August 2011 18:16:43 Bjorn Helgaas wrote:
> Previously we claimed device ID 0x7450, regardless of the vendor, which is
> clearly wrong.  Now we'll claim that device ID only for AMD.
> 
> I suspect this was just a typo in the original code, but it's possible this
> change will break shpchp on non-7450 AMD bridges.  If so, we'll have to fix
> them as we find them.
> 
> Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
> Reported-by: Ralf Jung <ralfjung-e@gmx.de>
> Cc: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/shpchp_core.c |    4 ++--
>  drivers/pci/hotplug/shpchp_hpc.c  |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/shpchp_core.c
> b/drivers/pci/hotplug/shpchp_core.c index aca972b..dd7e0c5 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -278,8 +278,8 @@ static int get_adapter_status (struct hotplug_slot
> *hotplug_slot, u8 *value)
> 
>  static int is_shpc_capable(struct pci_dev *dev)
>  {
> -	if ((dev->vendor == PCI_VENDOR_ID_AMD) || (dev->device ==
> -						PCI_DEVICE_ID_AMD_GOLAM_7450))
> +	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;
> diff --git a/drivers/pci/hotplug/shpchp_hpc.c
> b/drivers/pci/hotplug/shpchp_hpc.c index 36547f0..75ba231 100644
> --- a/drivers/pci/hotplug/shpchp_hpc.c
> +++ b/drivers/pci/hotplug/shpchp_hpc.c
> @@ -944,8 +944,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev
> *pdev) ctrl->pci_dev = pdev;  /* pci_dev of the P2P bridge */
>  	ctrl_dbg(ctrl, "Hotplug Controller:\n");
> 
> -	if ((pdev->vendor == PCI_VENDOR_ID_AMD) || (pdev->device ==
> -				PCI_DEVICE_ID_AMD_GOLAM_7450)) {
> +	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> +	    pdev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) {
>  		/* amd shpc driver doesn't use Base Offset; assume 0 */
>  		ctrl->mmio_base = pci_resource_start(pdev, 0);
>  		ctrl->mmio_size = pci_resource_len(pdev, 0);
--
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
Jesse Barnes Aug. 26, 2011, 3:02 p.m. UTC | #3
On Tue, 23 Aug 2011 10:16:43 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> Previously we claimed device ID 0x7450, regardless of the vendor, which is
> clearly wrong.  Now we'll claim that device ID only for AMD.
> 
> I suspect this was just a typo in the original code, but it's possible this
> change will break shpchp on non-7450 AMD bridges.  If so, we'll have to fix
> them as we find them.
> 
> Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
> Reported-by: Ralf Jung <ralfjung-e@gmx.de>
> Cc: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/shpchp_core.c |    4 ++--
>  drivers/pci/hotplug/shpchp_hpc.c  |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index aca972b..dd7e0c5 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -278,8 +278,8 @@ static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)
>  
>  static int is_shpc_capable(struct pci_dev *dev)
>  {
> -	if ((dev->vendor == PCI_VENDOR_ID_AMD) || (dev->device ==
> -						PCI_DEVICE_ID_AMD_GOLAM_7450))
> +	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;
> diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
> index 36547f0..75ba231 100644
> --- a/drivers/pci/hotplug/shpchp_hpc.c
> +++ b/drivers/pci/hotplug/shpchp_hpc.c
> @@ -944,8 +944,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
>  	ctrl->pci_dev = pdev;  /* pci_dev of the P2P bridge */
>  	ctrl_dbg(ctrl, "Hotplug Controller:\n");
>  
> -	if ((pdev->vendor == PCI_VENDOR_ID_AMD) || (pdev->device ==
> -				PCI_DEVICE_ID_AMD_GOLAM_7450)) {
> +	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> +	    pdev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) {
>  		/* amd shpc driver doesn't use Base Offset; assume 0 */
>  		ctrl->mmio_base = pci_resource_start(pdev, 0);
>  		ctrl->mmio_size = pci_resource_len(pdev, 0);
> 
> 

Great, looks good.  I'll queue it up for Linus (assume a cc: stable is
ok too?).

Ralf, can I get a tested-by?

Thanks,
Ralf Jung Aug. 26, 2011, 3:11 p.m. UTC | #4
Hi,

> Great, looks good.  I'll queue it up for Linus (assume a cc: stable is
> ok too?).
> 
> Ralf, can I get a tested-by?
Sure, if you tell me how to do that... it's the first time ever I deal with 
kernel patches.

Kind regards,
Ralf
--
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
Jesse Barnes Aug. 26, 2011, 3:12 p.m. UTC | #5
On Fri, 26 Aug 2011 17:11:06 +0200
Ralf Jung <ralfjung-e@gmx.de> wrote:

> Hi,
> 
> > Great, looks good.  I'll queue it up for Linus (assume a cc: stable is
> > ok too?).
> > 
> > Ralf, can I get a tested-by?
> Sure, if you tell me how to do that... it's the first time ever I deal with 
> kernel patches.

If you've tested and it works, you can just reply to the patch with the
text:

Tested-by: Ralf Jung <ralfjung-e@gmx.de>

and I'll include it in the patch when I commit it.

Thanks,
Ralf Jung Aug. 26, 2011, 3:23 p.m. UTC | #6
Seems to work fine here, thanks!

Tested-by: Ralf Jung <ralfjung-e@gmx.de>

On Tuesday 23 August 2011 18:16:43 Bjorn Helgaas wrote:
> Previously we claimed device ID 0x7450, regardless of the vendor, which is
> clearly wrong.  Now we'll claim that device ID only for AMD.
> 
> I suspect this was just a typo in the original code, but it's possible this
> change will break shpchp on non-7450 AMD bridges.  If so, we'll have to fix
> them as we find them.
> 
> Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
> Reported-by: Ralf Jung <ralfjung-e@gmx.de>
> Cc: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/shpchp_core.c |    4 ++--
>  drivers/pci/hotplug/shpchp_hpc.c  |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/shpchp_core.c
> b/drivers/pci/hotplug/shpchp_core.c index aca972b..dd7e0c5 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -278,8 +278,8 @@ static int get_adapter_status (struct hotplug_slot
> *hotplug_slot, u8 *value)
> 
>  static int is_shpc_capable(struct pci_dev *dev)
>  {
> -	if ((dev->vendor == PCI_VENDOR_ID_AMD) || (dev->device ==
> -						PCI_DEVICE_ID_AMD_GOLAM_7450))
> +	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;
> diff --git a/drivers/pci/hotplug/shpchp_hpc.c
> b/drivers/pci/hotplug/shpchp_hpc.c index 36547f0..75ba231 100644
> --- a/drivers/pci/hotplug/shpchp_hpc.c
> +++ b/drivers/pci/hotplug/shpchp_hpc.c
> @@ -944,8 +944,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev
> *pdev) ctrl->pci_dev = pdev;  /* pci_dev of the P2P bridge */
>  	ctrl_dbg(ctrl, "Hotplug Controller:\n");
> 
> -	if ((pdev->vendor == PCI_VENDOR_ID_AMD) || (pdev->device ==
> -				PCI_DEVICE_ID_AMD_GOLAM_7450)) {
> +	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> +	    pdev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) {
>  		/* amd shpc driver doesn't use Base Offset; assume 0 */
>  		ctrl->mmio_base = pci_resource_start(pdev, 0);
>  		ctrl->mmio_size = pci_resource_len(pdev, 0);
Bjorn Helgaas Aug. 26, 2011, 3:48 p.m. UTC | #7
On Fri, Aug 26, 2011 at 8:51 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Bjorn Helgaas wrote:
>
>> [Subject: [PATCH] PCI hotplug: shpchp: don't blindly claim non-AMD 0x7450 device IDs]
>
> To users, the more noticeable impact will be that this doesn't claim
> all pci-pci bridges with AMD vendor ID any more either, right?

The driver normally claims PCI bridges with the SHPC (Standard
Hot-plug Controller) capability.  That works for all bridges that
conform to the SHPC spec.

In addition, we have this special case for AMD bridges.  Previously,
we used the special case for all bridges with device ID 0x7450 and all
AMD bridges.  The special case ignores the SHPC capability.  I suspect
the intent of this "quirk" was to deal with an early AMD bridge that
didn't conform to the SHPC spec.

With my patch, the special case will apply only to the AMD 0x7450
bridge.  So we won't treat non-AMD 0x7450 bridges (if any) specially
any more.  And, as you say, we won't treat AMD non-0x7450 bridges
specially either.  It's possible that will break SHPC on some AMD
bridges, but it's also possible that it will *fix* newer AMD bridges
that conform to the spec and supply the SHPC capability.  We used to
ignore that SHPC capability, and now we'll pay attention to it.

So SHPC users with AMD bridges *will* likely notice a difference.
Hopefully it's usually that an error message goes away.  This seems to
be quite common: googling for "shpchp 0000:00:01.0: Cannot reserve
MMIO region" found many reports.  It could also be that SHPC breaks on
AMD non-0x7450 bridges that don't conform to the spec.  Or, if we're
lucky, SHPC could start working on AMD bridges that *do* conform to
the spec.

Bjorn
--
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
Jesse Barnes Aug. 26, 2011, 9:24 p.m. UTC | #8
On Tue, 23 Aug 2011 10:16:43 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> Previously we claimed device ID 0x7450, regardless of the vendor, which is
> clearly wrong.  Now we'll claim that device ID only for AMD.
> 
> I suspect this was just a typo in the original code, but it's possible this
> change will break shpchp on non-7450 AMD bridges.  If so, we'll have to fix
> them as we find them.

Applied to my for-linus branch, thanks.
diff mbox

Patch

diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index aca972b..dd7e0c5 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -278,8 +278,8 @@  static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)
 
 static int is_shpc_capable(struct pci_dev *dev)
 {
-	if ((dev->vendor == PCI_VENDOR_ID_AMD) || (dev->device ==
-						PCI_DEVICE_ID_AMD_GOLAM_7450))
+	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;
diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
index 36547f0..75ba231 100644
--- a/drivers/pci/hotplug/shpchp_hpc.c
+++ b/drivers/pci/hotplug/shpchp_hpc.c
@@ -944,8 +944,8 @@  int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
 	ctrl->pci_dev = pdev;  /* pci_dev of the P2P bridge */
 	ctrl_dbg(ctrl, "Hotplug Controller:\n");
 
-	if ((pdev->vendor == PCI_VENDOR_ID_AMD) || (pdev->device ==
-				PCI_DEVICE_ID_AMD_GOLAM_7450)) {
+	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+	    pdev->device == PCI_DEVICE_ID_AMD_GOLAM_7450) {
 		/* amd shpc driver doesn't use Base Offset; assume 0 */
 		ctrl->mmio_base = pci_resource_start(pdev, 0);
 		ctrl->mmio_size = pci_resource_len(pdev, 0);