x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bin Gao]

x86/pci/mrst: force all pci config access toward 0:0:0, 0:2:0 and 0:3:0 to type 1

For real pci devices 0:0:0, 0:2:0 and 0:3:0, there is either no pci shim, or
no guarantee of data correctness of offset 256-4k. So for whatever reason,
Linux kernel should not do MMCFG pci config access to those devices. Instead,
always use type 1 for those devices.

Signed-off-by: Bin Gao <bin.gao@linux.intel.com>
---
 arch/x86/pci/mrst.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/pci/mrst.c b/arch/x86/pci/mrst.c
index 6eb18c4..0e0fabf 100644
--- a/arch/x86/pci/mrst.c
+++ b/arch/x86/pci/mrst.c
@@ -141,6 +141,11 @@ static int pci_device_update_fixed(struct pci_bus *bus, unsigned int devfn,
  */
 static bool type1_access_ok(unsigned int bus, unsigned int devfn, int reg)
 {
+	if (bus == 0 && (devfn == PCI_DEVFN(2, 0)
+				|| devfn == PCI_DEVFN(0, 0)
+				|| devfn == PCI_DEVFN(3, 0)))
+		return 1;
+
 	/* This is a workaround for A0 LNC bug where PCI status register does
 	 * not have new CAP bit set. can not be written by SW either.
 	 *
@@ -150,10 +155,7 @@ static bool type1_access_ok(unsigned int bus, unsigned int devfn, int reg)
 	 */
 	if (reg >= 0x100 || reg == PCI_STATUS || reg == PCI_HEADER_TYPE)
 		return 0;
-	if (bus == 0 && (devfn == PCI_DEVFN(2, 0)
-				|| devfn == PCI_DEVFN(0, 0)
-				|| devfn == PCI_DEVFN(3, 0)))
-		return 1;
+
 	return 0; /* langwell on others */
 }
 
-- 
1.7.4.4

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bjorn Helgaas]

On Tue, Apr 30, 2013 at 1:21 AM, Bin Gao <bin.gao@linux.intel.com> wrote:
> x86/pci/mrst: force all pci config access toward 0:0:0, 0:2:0 and 0:3:0 to type 1
>
> For real pci devices 0:0:0, 0:2:0 and 0:3:0, there is either no pci shim, or
> no guarantee of data correctness of offset 256-4k. So for whatever reason,
> Linux kernel should not do MMCFG pci config access to those devices. Instead,
> always use type 1 for those devices.
>
> Signed-off-by: Bin Gao <bin.gao@linux.intel.com>

I applied this to my for-linus branch and will try to put this in v3.10.

> ---
>  arch/x86/pci/mrst.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/pci/mrst.c b/arch/x86/pci/mrst.c
> index 6eb18c4..0e0fabf 100644
> --- a/arch/x86/pci/mrst.c
> +++ b/arch/x86/pci/mrst.c
> @@ -141,6 +141,11 @@ static int pci_device_update_fixed(struct pci_bus *bus, unsigned int devfn,
>   */
>  static bool type1_access_ok(unsigned int bus, unsigned int devfn, int reg)
>  {
> +       if (bus == 0 && (devfn == PCI_DEVFN(2, 0)
> +                               || devfn == PCI_DEVFN(0, 0)
> +                               || devfn == PCI_DEVFN(3, 0)))
> +               return 1;
> +
>         /* This is a workaround for A0 LNC bug where PCI status register does
>          * not have new CAP bit set. can not be written by SW either.
>          *
> @@ -150,10 +155,7 @@ static bool type1_access_ok(unsigned int bus, unsigned int devfn, int reg)
>          */
>         if (reg >= 0x100 || reg == PCI_STATUS || reg == PCI_HEADER_TYPE)
>                 return 0;
> -       if (bus == 0 && (devfn == PCI_DEVFN(2, 0)
> -                               || devfn == PCI_DEVFN(0, 0)
> -                               || devfn == PCI_DEVFN(3, 0)))
> -               return 1;
> +
>         return 0; /* langwell on others */
>  }
>
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bjorn Helgaas]

On Tue, Apr 30, 2013 at 1:21 AM, Bin Gao <bin.gao@linux.intel.com> wrote:
> x86/pci/mrst: force all pci config access toward 0:0:0, 0:2:0 and 0:3:0 to type 1
>
> For real pci devices 0:0:0, 0:2:0 and 0:3:0, there is either no pci shim, or
> no guarantee of data correctness of offset 256-4k. So for whatever reason,
> Linux kernel should not do MMCFG pci config access to those devices. Instead,
> always use type 1 for those devices.
>
> Signed-off-by: Bin Gao <bin.gao@linux.intel.com>
> ---
>  arch/x86/pci/mrst.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/pci/mrst.c b/arch/x86/pci/mrst.c
> index 6eb18c4..0e0fabf 100644
> --- a/arch/x86/pci/mrst.c
> +++ b/arch/x86/pci/mrst.c
> @@ -141,6 +141,11 @@ static int pci_device_update_fixed(struct pci_bus *bus, unsigned int devfn,
>   */
>  static bool type1_access_ok(unsigned int bus, unsigned int devfn, int reg)
>  {
> +       if (bus == 0 && (devfn == PCI_DEVFN(2, 0)
> +                               || devfn == PCI_DEVFN(0, 0)
> +                               || devfn == PCI_DEVFN(3, 0)))
> +               return 1;

Is there any possibility of multi-function devices at bus 0, device 0, 2, or 3?

What about bridges -- can any of these be a bridge?

If either of those could happen, these checks could be too specific.

Is there a doc that identifies these cases where config mechanism #1
should be used instead of MMCONFIG?

Bjorn

> +
>         /* This is a workaround for A0 LNC bug where PCI status register does
>          * not have new CAP bit set. can not be written by SW either.
>          *
> @@ -150,10 +155,7 @@ static bool type1_access_ok(unsigned int bus, unsigned int devfn, int reg)
>          */
>         if (reg >= 0x100 || reg == PCI_STATUS || reg == PCI_HEADER_TYPE)
>                 return 0;
> -       if (bus == 0 && (devfn == PCI_DEVFN(2, 0)
> -                               || devfn == PCI_DEVFN(0, 0)
> -                               || devfn == PCI_DEVFN(3, 0)))
> -               return 1;
> +
>         return 0; /* langwell on others */
>  }
>
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bin Gao]

On Wed, May 01, 2013 at 11:17:10AM -0600, Bjorn Helgaas wrote:
> Is there any possibility of multi-function devices at bus 0, device 0, 2, or 3?
> 
> What about bridges -- can any of these be a bridge?
> 
> If either of those could happen, these checks could be too specific.
0:2:0 and 0:3:0 are the only 2 devices behind the bridge 0:0:0.
These devices don't implement pcie capability list in legacy config space
so no offset above 255 would be triggered.
The fixed bar pcie capability located at 0x100 is for pci-shimed device
only, not for 0, 2 and 3. But current implementation applies it to 0,
2 and 3 as well. This is what the patch is going to address.

> 
> Is there a doc that identifies these cases where config mechanism #1
> should be used instead of MMCONFIG?
Unfortunately no doc identifies this.
But since FW doesn't provide pci shim for device 0, 2 and 3,
and these 3 real pci devices have only 256 bytes legacy pci
config space so they can only be accessed by type 1.

> 
> Bjorn
> 

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bjorn Helgaas]

On Wed, May 1, 2013 at 11:53 PM, Bin Gao <bin.gao@linux.intel.com> wrote:
> On Wed, May 01, 2013 at 11:17:10AM -0600, Bjorn Helgaas wrote:
>> Is there any possibility of multi-function devices at bus 0, device 0, 2, or 3?
>>
>> What about bridges -- can any of these be a bridge?
>>
>> If either of those could happen, these checks could be too specific.
> 0:2:0 and 0:3:0 are the only 2 devices behind the bridge 0:0:0.

Please use the conventional Linux PCI address formatting
(DDDD:BB:dd.f, where DDDD = domain (optional, often omitted if DDDD ==
0), BB = bus, dd = device, f = function), because this is quite
confusing.

You say "0:2:0 and 0:3:0" are behind the bridge "0:0:0", but the patch
you sent clearly applies only to devices on bus 0.  The patch applies
to devices 00:00.0, 00:02.0, and 00:03.0.  These are all on the same
bus, so none of them can be behind a bridge.

If the 00:00.0 device is in fact a bridge, its secondary bus will be
something other than 0, so any devices behind the bridge will be on a
non-zero bus number.  And I assume you would want to use config
mechanism #1 to reach those devices, too.  Your current patch doesn't
do that -- it only applies to devices on bus 0.

I dropped this patch until this gets straightened out.

> These devices don't implement pcie capability list in legacy config space
> so no offset above 255 would be triggered.
> The fixed bar pcie capability located at 0x100 is for pci-shimed device
> only, not for 0, 2 and 3. But current implementation applies it to 0,
> 2 and 3 as well. This is what the patch is going to address.
>
>>
>> Is there a doc that identifies these cases where config mechanism #1
>> should be used instead of MMCONFIG?
> Unfortunately no doc identifies this.
> But since FW doesn't provide pci shim for device 0, 2 and 3,
> and these 3 real pci devices have only 256 bytes legacy pci
> config space so they can only be accessed by type 1.
>
>>
>> Bjorn
>>

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bjorn Helgaas]

On Mon, May 6, 2013 at 10:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, May 1, 2013 at 11:53 PM, Bin Gao <bin.gao@linux.intel.com> wrote:
>> On Wed, May 01, 2013 at 11:17:10AM -0600, Bjorn Helgaas wrote:
>>> Is there any possibility of multi-function devices at bus 0, device 0, 2, or 3?

I hate it when I go to the trouble of reviewing something and asking
relevant questions, and then people ignore the questions :(

I'd still like to know whether multi-function devices are possible at
bus 0, device 0, 2, or 3.

Are these devices built-in, so you know exactly what they are and they
can never be anything else?  Or is there a slot or other way to
connect a user-chosen device?

>>> What about bridges -- can any of these be a bridge?
>>>
>>> If either of those could happen, these checks could be too specific.
>> 0:2:0 and 0:3:0 are the only 2 devices behind the bridge 0:0:0.

If you included "lspci -vv" output, that would make this much easier
to talk about.

> Please use the conventional Linux PCI address formatting
> (DDDD:BB:dd.f, where DDDD = domain (optional, often omitted if DDDD ==
> 0), BB = bus, dd = device, f = function), because this is quite
> confusing.
>
> You say "0:2:0 and 0:3:0" are behind the bridge "0:0:0", but the patch
> you sent clearly applies only to devices on bus 0.  The patch applies
> to devices 00:00.0, 00:02.0, and 00:03.0.  These are all on the same
> bus, so none of them can be behind a bridge.
>
> If the 00:00.0 device is in fact a bridge, its secondary bus will be
> something other than 0, so any devices behind the bridge will be on a
> non-zero bus number.  And I assume you would want to use config
> mechanism #1 to reach those devices, too.  Your current patch doesn't
> do that -- it only applies to devices on bus 0.
>
> I dropped this patch until this gets straightened out.
>
>> These devices don't implement pcie capability list in legacy config space
>> so no offset above 255 would be triggered.
>> The fixed bar pcie capability located at 0x100 is for pci-shimed device
>> only, not for 0, 2 and 3. But current implementation applies it to 0,
>> 2 and 3 as well. This is what the patch is going to address.
>>
>>>
>>> Is there a doc that identifies these cases where config mechanism #1
>>> should be used instead of MMCONFIG?
>> Unfortunately no doc identifies this.
>> But since FW doesn't provide pci shim for device 0, 2 and 3,
>> and these 3 real pci devices have only 256 bytes legacy pci
>> config space so they can only be accessed by type 1.
>>
>>>
>>> Bjorn
>>>

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bin Gao]

On Mon, May 06, 2013 at 11:56:38AM -0700, Bjorn Helgaas wrote:
> I hate it when I go to the trouble of reviewing something and asking
> relevant questions, and then people ignore the questions :(
Sorry for that.
 
> I'd still like to know whether multi-function devices are possible at
> bus 0, device 0, 2, or 3.
0000:00:00.0 is host bridge.
0000:00:02.0 and 0000:00:03.0 are single-function devices.

> Are these devices built-in, so you know exactly what they are and they
> can never be anything else? 
Yes, they are built-in devices and I know exactly what they are.

> Or is there a slot or other way to connect a user-chosen device?
No, not that way.

> If you included "lspci -vv" output, that would make this much easier
> to talk about.
I'm sorry the busybox shrinks lspci options so it doesn't have
meaningful ouput excpet for DID:VID and class code.

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bjorn Helgaas]

[+cc linux-pci]

On Mon, May 6, 2013 at 3:42 PM, Bin Gao <bin.gao@linux.intel.com> wrote:
> On Mon, May 06, 2013 at 11:56:38AM -0700, Bjorn Helgaas wrote:
>> I hate it when I go to the trouble of reviewing something and asking
>> relevant questions, and then people ignore the questions :(
> Sorry for that.
>
>> I'd still like to know whether multi-function devices are possible at
>> bus 0, device 0, 2, or 3.
> 0000:00:00.0 is host bridge.
> 0000:00:02.0 and 0000:00:03.0 are single-function devices.
>
>> Are these devices built-in, so you know exactly what they are and they
>> can never be anything else?
> Yes, they are built-in devices and I know exactly what they are.
>
>> Or is there a slot or other way to connect a user-chosen device?
> No, not that way.
>
>> If you included "lspci -vv" output, that would make this much easier
>> to talk about.
> I'm sorry the busybox shrinks lspci options so it doesn't have
> meaningful ouput excpet for DID:VID and class code.

All right, I applied this and will ask Linus to put it in v3.10.

Bjorn

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[H. Peter Anvin]

On 05/06/2013 10:38 AM, Bjorn Helgaas wrote:
> On Wed, May 1, 2013 at 11:53 PM, Bin Gao <bin.gao@linux.intel.com> wrote:
>> On Wed, May 01, 2013 at 11:17:10AM -0600, Bjorn Helgaas wrote:
>>> Is there any possibility of multi-function devices at bus 0, device 0, 2, or 3?
>>>
>>> What about bridges -- can any of these be a bridge?
>>>
>>> If either of those could happen, these checks could be too specific.
>> 0:2:0 and 0:3:0 are the only 2 devices behind the bridge 0:0:0.
> 
> Please use the conventional Linux PCI address formatting
> (DDDD:BB:dd.f, where DDDD = domain (optional, often omitted if DDDD ==
> 0), BB = bus, dd = device, f = function), because this is quite
> confusing.
> 
> You say "0:2:0 and 0:3:0" are behind the bridge "0:0:0", but the patch
> you sent clearly applies only to devices on bus 0.  The patch applies
> to devices 00:00.0, 00:02.0, and 00:03.0.  These are all on the same
> bus, so none of them can be behind a bridge.
> 
> If the 00:00.0 device is in fact a bridge, its secondary bus will be
> something other than 0, so any devices behind the bridge will be on a
> non-zero bus number.  And I assume you would want to use config
> mechanism #1 to reach those devices, too.  Your current patch doesn't
> do that -- it only applies to devices on bus 0.
> 

0000:00:00.0 is presumably the *host bridge*, which is unique in PCI
space in that it is a type 0 header and sits on what normally would be
the client bus.

	-hpa

Re: x86/pci/mrst: force all pci config toward 0:0:0, 0:2:0 and 0:3:0 to type 1

[Bin Gao]

On Mon, May 06, 2013 at 10:38:55AM -0700, Bjorn Helgaas wrote:
> Please use the conventional Linux PCI address formatting
> (DDDD:BB:dd.f, where DDDD = domain (optional, often omitted if DDDD ==
> 0), BB = bus, dd = device, f = function), because this is quite
> confusing.
Yes, will do.

> You say "0:2:0 and 0:3:0" are behind the bridge "0:0:0", but the patch
> you sent clearly applies only to devices on bus 0.  The patch applies
> to devices 00:00.0, 00:02.0, and 00:03.0.  These are all on the same
> bus, so none of them can be behind a bridge.
Sorry my bad, "behind" could be confusing(or wrong:-)).
0000:00:00.0 is the host bridge, 0000:00:02.0 and 0000:00:03.0 are
the 2 devices on bus 00. So these 3 devides are on the same bus.

> If the 00:00.0 device is in fact a bridge, its secondary bus will be
> something other than 0, so any devices behind the bridge will be on a
> non-zero bus number.  And I assume you would want to use config
> mechanism #1 to reach those devices, too.  Your current patch doesn't
> do that -- it only applies to devices on bus 0.
No pci-pci bridge, no secondary bus (see above explanation).
0000:00:00.0 (host bridge), 0000:00:02.0 and 0000:00:03.0 are the only
3 real pci devices in the chip.
 
> I dropped this patch until this gets straightened out.
Will re-submit together with other cleanup for the same file. Thanks.