| Message ID | 20110823161643.27268.89203.stgit@bhelgaas.mtv.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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
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,
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
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,
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);
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
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 --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);
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