Impact: get correct pci_cfg_size for host_bridge
more host bridges support 4k cfg, so check them directly
instead of quirks.
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/pci/fixup.c | 20 --------------------
drivers/pci/probe.c | 9 ++++++++-
2 files changed, 8 insertions(+), 21 deletions(-)
Index: linux-2.6/arch/x86/pci/fixup.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/fixup.c
+++ linux-2.6/arch/x86/pci/fixup.c
@@ -495,26 +495,6 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S
pci_siemens_interrupt_controller);
/*
- * Regular PCI devices have 256 bytes, but AMD Family 10h/11h CPUs have
- * 4096 bytes configuration space for each function of their processor
- * configuration space.
- */
-static void amd_cpu_pci_cfg_space_size(struct pci_dev *dev)
-{
- dev->cfg_size = pci_cfg_space_size_ext(dev);
-}
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1200, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1201, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1202, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1203, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1204, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1300, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1301, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1302, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1303, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1304, amd_cpu_pci_cfg_space_size);
-
-/*
* SB600: Disable BAR1 on device 14.0 to avoid HPET resources from
* confusing the PCI engine:
*/
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -847,6 +847,11 @@ int pci_cfg_space_size(struct pci_dev *d
{
int pos;
u32 status;
+ u16 class;
+
+ class = dev->class >> 8;
+ if (class == PCI_CLASS_BRIDGE_HOST)
+ return pci_cfg_space_size_ext(dev);
pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
if (!pos) {
@@ -936,7 +941,6 @@ static struct pci_dev *pci_scan_device(s
dev->multifunction = !!(hdr_type & 0x80);
dev->vendor = l & 0xffff;
dev->device = (l >> 16) & 0xffff;
- dev->cfg_size = pci_cfg_space_size(dev);
dev->error_state = pci_channel_io_normal;
set_pcie_port_type(dev);
@@ -952,6 +956,9 @@ static struct pci_dev *pci_scan_device(s
return NULL;
}
+ /* need to have dev->class ready */
+ dev->cfg_size = pci_cfg_space_size(dev);
+
return dev;
}
Yinghai Lu wrote:
> Impact: get correct pci_cfg_size for host_bridge
>
> more host bridges support 4k cfg, so check them directly
> instead of quirks.
>
> Signed-off-by: Yinghai Lu <[email protected]>
I'm utterly confused by this. This is basically saying we should try to
probe for an extended device space for every host bridge. Logically
speaking, this is valid: if there is a valid path by which we can probe
for byte 256 then it should succeed.
HOWEVER, the same argument applies for *every single device*. So if
this does indeed work, why should we limit it to host bridges?
-hpa
H. Peter Anvin wrote:
> Yinghai Lu wrote:
>> Impact: get correct pci_cfg_size for host_bridge
>>
>> more host bridges support 4k cfg, so check them directly
>> instead of quirks.
>>
>> Signed-off-by: Yinghai Lu <[email protected]>
>
> I'm utterly confused by this. This is basically saying we should try to
> probe for an extended device space for every host bridge. Logically
> speaking, this is valid: if there is a valid path by which we can probe
> for byte 256 then it should succeed.
>
> HOWEVER, the same argument applies for *every single device*. So if
> this does indeed work, why should we limit it to host bridges?
currently only found amd and intel host bridge (on cpu or io hub) need this trick.
other device we could still use quirks.
YH
H. Peter Anvin wrote:
> Yinghai Lu wrote:
>> Impact: get correct pci_cfg_size for host_bridge
>>
>> more host bridges support 4k cfg, so check them directly
>> instead of quirks.
>>
>> Signed-off-by: Yinghai Lu <[email protected]>
>
> I'm utterly confused by this. This is basically saying we should try to
> probe for an extended device space for every host bridge. Logically
> speaking, this is valid: if there is a valid path by which we can probe
> for byte 256 then it should succeed.
>
> HOWEVER, the same argument applies for *every single device*. So if
> this does indeed work, why should we limit it to host bridges?
>
Looking at the code (as opposed to just the patch) made it a bit
clearer. The argument you're making here is that only host bridges are
known to have extended address space without also having a PCI-X
extension header, right?
-hpa
H. Peter Anvin wrote:
> H. Peter Anvin wrote:
>> Yinghai Lu wrote:
>>> Impact: get correct pci_cfg_size for host_bridge
>>>
>>> more host bridges support 4k cfg, so check them directly
>>> instead of quirks.
>>>
>>> Signed-off-by: Yinghai Lu <[email protected]>
>>
>> I'm utterly confused by this. This is basically saying we should try
>> to probe for an extended device space for every host bridge.
>> Logically speaking, this is valid: if there is a valid path by which
>> we can probe for byte 256 then it should succeed.
>>
>> HOWEVER, the same argument applies for *every single device*. So if
>> this does indeed work, why should we limit it to host bridges?
>>
>
> Looking at the code (as opposed to just the patch) made it a bit
> clearer. The argument you're making here is that only host bridges are
> known to have extended address space without also having a PCI-X
> extension header, right?
Yes.
YH
Yinghai Lu wrote:
>
> currently only found amd and intel host bridge (on cpu or io hub) need this trick.
> other device we could still use quirks.
>
I'd suspect that we might just want to do this for all devices if we
ever find any, but yes, let's wait until we have a concrete specimen
before even trying something like that.
However, it'd be good to have this in the patch description (which
really is a PCI patch more than an x86 patch, so I'd be happier if it
went through Jesse than me.)
-hpa
H. Peter Anvin wrote:
> Yinghai Lu wrote:
>>
>> currently only found amd and intel host bridge (on cpu or io hub) need
>> this trick.
>> other device we could still use quirks.
>>
>
> I'd suspect that we might just want to do this for all devices if we
> ever find any, but yes, let's wait until we have a concrete specimen
> before even trying something like that.
>
> However, it'd be good to have this in the patch description (which
> really is a PCI patch more than an x86 patch, so I'd be happier if it
> went through Jesse than me.)
sure. it could affect IA64 guys with io hubs too.
YH
* Yinghai Lu <[email protected]> wrote:
> H. Peter Anvin wrote:
> > H. Peter Anvin wrote:
> >> Yinghai Lu wrote:
> >>> Impact: get correct pci_cfg_size for host_bridge
> >>>
> >>> more host bridges support 4k cfg, so check them directly
> >>> instead of quirks.
> >>>
> >>> Signed-off-by: Yinghai Lu <[email protected]>
> >>
> >> I'm utterly confused by this. This is basically saying we should try
> >> to probe for an extended device space for every host bridge.
> >> Logically speaking, this is valid: if there is a valid path by which
> >> we can probe for byte 256 then it should succeed.
> >>
> >> HOWEVER, the same argument applies for *every single device*. So if
> >> this does indeed work, why should we limit it to host bridges?
> >>
> >
> > Looking at the code (as opposed to just the patch) made it a bit
> > clearer. The argument you're making here is that only host bridges are
> > known to have extended address space without also having a PCI-X
> > extension header, right?
>
>
> Yes.
This too should go into the v2 patch description.
I'd also suggest to flip around the subject line from x86/pci to
pci/x86 - it's more of a PCI patch than a pure x86 patch. The
same problem could affect other platforms too i suspect.
Ingo
On Sun, Mar 08, 2009 at 12:04:37PM +0100, Ingo Molnar wrote:
> I'd also suggest to flip around the subject line from x86/pci to
> pci/x86 - it's more of a PCI patch than a pure x86 patch. The
> same problem could affect other platforms too i suspect.
Not all platforms expose the root bridge in PCI configuration space.
Intel ia64 platforms do have a materialised root bridge; HP's don't.
(I don't know whether Intel ship any PCIe ia64 machines or not). I
don't remember details for other platforms, but I think Intel is the odd
one out in this regard.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Sun, Mar 08, 2009 at 12:04:37PM +0100, Ingo Molnar wrote:
>> I'd also suggest to flip around the subject line from x86/pci to
>> pci/x86 - it's more of a PCI patch than a pure x86 patch. The
>> same problem could affect other platforms too i suspect.
>
> Not all platforms expose the root bridge in PCI configuration space.
> Intel ia64 platforms do have a materialised root bridge; HP's don't.
> (I don't know whether Intel ship any PCIe ia64 machines or not). I
> don't remember details for other platforms, but I think Intel is the odd
> one out in this regard.
>
It's still generic PCI code, however. If there is no host bridge
exposed in PCI space, the patch is a noop.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
H. Peter Anvin wrote:
> Matthew Wilcox wrote:
>> On Sun, Mar 08, 2009 at 12:04:37PM +0100, Ingo Molnar wrote:
>>> I'd also suggest to flip around the subject line from x86/pci to
>>> pci/x86 - it's more of a PCI patch than a pure x86 patch. The
>>> same problem could affect other platforms too i suspect.
>> Not all platforms expose the root bridge in PCI configuration space.
>> Intel ia64 platforms do have a materialised root bridge; HP's don't.
>> (I don't know whether Intel ship any PCIe ia64 machines or not). I
>> don't remember details for other platforms, but I think Intel is the odd
>> one out in this regard.
>>
>
> It's still generic PCI code, however. If there is no host bridge
> exposed in PCI space, the patch is a noop.
ok, will split that patch to two
1. one touch pci.c
2. and one touch x86/pci.
YH
Yinghai Lu wrote:
>>>
>> It's still generic PCI code, however. If there is no host bridge
>> exposed in PCI space, the patch is a noop.
>
> ok, will split that patch to two
> 1. one touch pci.c
> 2. and one touch x86/pci.
>
Speaking for myself, I don't see a reason to do that. Just push the
combined patch to Jesse.
Acked-by: H. Peter Anvin <[email protected]>
... if you need it.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
H. Peter Anvin wrote:
> Yinghai Lu wrote:
>>> It's still generic PCI code, however. If there is no host bridge
>>> exposed in PCI space, the patch is a noop.
>> ok, will split that patch to two
>> 1. one touch pci.c
>> 2. and one touch x86/pci.
>>
>
> Speaking for myself, I don't see a reason to do that. Just push the
> combined patch to Jesse.
>
> Acked-by: H. Peter Anvin <[email protected]>
>
> ... if you need it.
thanks.
Jesse or Matthew, can you pick this patch?
YH
* Yinghai Lu <[email protected]> wrote:
> H. Peter Anvin wrote:
> > Yinghai Lu wrote:
> >>> It's still generic PCI code, however. If there is no host bridge
> >>> exposed in PCI space, the patch is a noop.
> >> ok, will split that patch to two
> >> 1. one touch pci.c
> >> 2. and one touch x86/pci.
> >>
> >
> > Speaking for myself, I don't see a reason to do that. Just push the
> > combined patch to Jesse.
> >
> > Acked-by: H. Peter Anvin <[email protected]>
> >
> > ... if you need it.
>
> thanks.
>
> Jesse or Matthew, can you pick this patch?
You might want to resubmit it with the added changelog items
pointed out in the discussion.
Ingo
On Sun, Mar 08, 2009 at 02:21:40PM -0700, Yinghai Lu wrote:
> Jesse or Matthew, can you pick this patch?
Having reviewed it in light of HPA's comment, I don't have a problem
with it:
Reviewed-by: Matthew Wilcox <[email protected]>
However, I don't think it's my place to take this patch while Jesse is
away; it doesn't feel like it's needed to be submitted between -rc7 and
2.6.29. Do you have a reason that it needs to be merged more urgently
than 2.6.30-rc1?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Sun, Mar 08, 2009 at 02:21:40PM -0700, Yinghai Lu wrote:
>> Jesse or Matthew, can you pick this patch?
>
> Having reviewed it in light of HPA's comment, I don't have a problem
> with it:
>
> Reviewed-by: Matthew Wilcox <[email protected]>
>
> However, I don't think it's my place to take this patch while Jesse is
> away; it doesn't feel like it's needed to be submitted between -rc7 and
> 2.6.29. Do you have a reason that it needs to be merged more urgently
> than 2.6.30-rc1?
>
I didn't see one... I was assuming it was a submission to be pushed
upstream during the merge window.
It doesn't fix a regression, so it doesn't seem to me to be a case for a
late -rc merge. It's hardware enablement, so it *might* qualify for
2.6.29-stable, as far as I understand Greg and Chris' policies.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
* H. Peter Anvin <[email protected]> wrote:
> Matthew Wilcox wrote:
> > On Sun, Mar 08, 2009 at 02:21:40PM -0700, Yinghai Lu wrote:
> >> Jesse or Matthew, can you pick this patch?
> >
> > Having reviewed it in light of HPA's comment, I don't have a problem
> > with it:
> >
> > Reviewed-by: Matthew Wilcox <[email protected]>
> >
> > However, I don't think it's my place to take this patch while Jesse is
> > away; it doesn't feel like it's needed to be submitted between -rc7 and
> > 2.6.29. Do you have a reason that it needs to be merged more urgently
> > than 2.6.30-rc1?
> >
>
> I didn't see one... I was assuming it was a submission to be
> pushed upstream during the merge window.
>
> It doesn't fix a regression, so it doesn't seem to me to be a
> case for a late -rc merge. It's hardware enablement, so it
> *might* qualify for 2.6.29-stable, as far as I understand Greg
> and Chris' policies.
Yeah. The patch is replacing a slowly-but-surely-bitrotting and
always-behind quirk table with a more generic
approach/workaround.
So there's no regression technically - but non-fully-working
cards are obviously quite annoying on new systems. So i'd
suggest a .30 merge with a Cc: <[email protected]> tag.
Ingo
please check...
Impact: get correct pci_cfg_size for host_bridge
more host bridges support 4k cfg, so check them directy instead of quirks.
only need to do this extra check for host_bridge at this point, because only
host bridges are known to have extended address space without also having a
PCI-X/PCI-E caps.
other devices we could still do quirks for them if there is any.
also remove the quirks for AMD host bridges with family 10h and 11h that is not
needed any more
with this patch, we can get correct pci cfg size of new Intel CPUs/IOHs with
host bridges
v2: updated commit log.
Signed-off-by: Yinghai Lu <[email protected]>
Acked-by: H. Peter Anvin <[email protected]>
Reviewed-by: Matthew Wilcox <[email protected]>
Cc: <[email protected]>
---
arch/x86/pci/fixup.c | 20 --------------------
drivers/pci/probe.c | 9 ++++++++-
2 files changed, 8 insertions(+), 21 deletions(-)
Index: linux-2.6/arch/x86/pci/fixup.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/fixup.c
+++ linux-2.6/arch/x86/pci/fixup.c
@@ -495,26 +495,6 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S
pci_siemens_interrupt_controller);
/*
- * Regular PCI devices have 256 bytes, but AMD Family 10h/11h CPUs have
- * 4096 bytes configuration space for each function of their processor
- * configuration space.
- */
-static void amd_cpu_pci_cfg_space_size(struct pci_dev *dev)
-{
- dev->cfg_size = pci_cfg_space_size_ext(dev);
-}
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1200, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1201, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1202, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1203, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1204, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1300, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1301, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1302, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1303, amd_cpu_pci_cfg_space_size);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, 0x1304, amd_cpu_pci_cfg_space_size);
-
-/*
* SB600: Disable BAR1 on device 14.0 to avoid HPET resources from
* confusing the PCI engine:
*/
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -847,6 +847,11 @@ int pci_cfg_space_size(struct pci_dev *d
{
int pos;
u32 status;
+ u16 class;
+
+ class = dev->class >> 8;
+ if (class == PCI_CLASS_BRIDGE_HOST)
+ return pci_cfg_space_size_ext(dev);
pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
if (!pos) {
@@ -936,7 +941,6 @@ static struct pci_dev *pci_scan_device(s
dev->multifunction = !!(hdr_type & 0x80);
dev->vendor = l & 0xffff;
dev->device = (l >> 16) & 0xffff;
- dev->cfg_size = pci_cfg_space_size(dev);
dev->error_state = pci_channel_io_normal;
set_pcie_port_type(dev);
@@ -952,6 +956,9 @@ static struct pci_dev *pci_scan_device(s
return NULL;
}
+ /* need to have dev->class ready */
+ dev->cfg_size = pci_cfg_space_size(dev);
+
return dev;
}
On Sun, 08 Mar 2009 21:35:37 -0700
Yinghai Lu <[email protected]> wrote:
>
> Impact: get correct pci_cfg_size for host_bridge
>
> more host bridges support 4k cfg, so check them directy instead of
> quirks.
>
> only need to do this extra check for host_bridge at this point,
> because only host bridges are known to have extended address space
> without also having a PCI-X/PCI-E caps.
> other devices we could still do quirks for them if there is any.
>
> also remove the quirks for AMD host bridges with family 10h and 11h
> that is not needed any more
>
> with this patch, we can get correct pci cfg size of new Intel
> CPUs/IOHs with host bridges
>
> v2: updated commit log.
Applied to my linux-next branch, thanks.
--
Jesse Barnes, Intel Open Source Technology Center