PCI: Add two more values for PCIe Max_Read_Request_Size

Message ID 90260d62-e136-44a3-a35b-19fac1bcfb20@gmail.com
State Not Applicable
Headers show
Series
  • PCI: Add two more values for PCIe Max_Read_Request_Size
Related show

Commit Message

Heiner Kallweit April 12, 2018, 9:08 p.m.
This patch adds missing values for the max read request size.
E.g. network driver r8169 uses a value of 4K.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/uapi/linux/pci_regs.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Bjorn Helgaas April 12, 2018, 10:55 p.m. | #1
Hello Heiner,

On Thu, Apr 12, 2018 at 11:08:04PM +0200, Heiner Kallweit wrote:
> This patch adds missing values for the max read request size.
> E.g. network driver r8169 uses a value of 4K.

Is there a r8169 patch that adds uses of PCI_EXP_DEVCTL_READRQ_4096B?
If so, we should probably keep it together with this one since this
one would have to be merged first.

There's also a larger issue in that when Linux configures MPS and MRRS
at enumeration-time, it makes some assumptions about how MRRS is set.
If a driver like r8169 changes MRRS later, it may break those
assumptions, which might lead to PCIe errors.

If a user boots with "pci=pcie_bus_perf", we use PCIE_BUS_PERFORMANCE
mode, and in that case we may set MPS to something larger than some
devices can support, and we rely on MRRS to avoid problems.

I don't really *like* that scheme because it makes assumptions like
"drivers never change MRRS", but that's what we have right now.

So just be aware that if r8169 changes MRRS and users boot with
"pci=pcie_bus_perf", there is the potential for PCIe bus errors.

There was some recent discussion about this; [1] is a good place to
start.

[1] http://lkml.kernel.org/r/20180119205153.GB160618@bhelgaas-glaptop.roam.corp.google.com

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/uapi/linux/pci_regs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 0c79eac5..699257fb 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -506,6 +506,8 @@
>  #define  PCI_EXP_DEVCTL_READRQ_256B  0x1000 /* 256 Bytes */
>  #define  PCI_EXP_DEVCTL_READRQ_512B  0x2000 /* 512 Bytes */
>  #define  PCI_EXP_DEVCTL_READRQ_1024B 0x3000 /* 1024 Bytes */
> +#define  PCI_EXP_DEVCTL_READRQ_2048B 0x4000 /* 2048 Bytes */
> +#define  PCI_EXP_DEVCTL_READRQ_4096B 0x5000 /* 4096 Bytes */
>  #define  PCI_EXP_DEVCTL_BCR_FLR 0x8000  /* Bridge Configuration Retry / FLR */
>  #define PCI_EXP_DEVSTA		10	/* Device Status */
>  #define  PCI_EXP_DEVSTA_CED	0x0001	/* Correctable Error Detected */
> -- 
> 2.17.0
>
Heiner Kallweit April 13, 2018, 5:48 a.m. | #2
Am 13.04.2018 um 00:55 schrieb Bjorn Helgaas:
> Hello Heiner,
> 
> On Thu, Apr 12, 2018 at 11:08:04PM +0200, Heiner Kallweit wrote:
>> This patch adds missing values for the max read request size.
>> E.g. network driver r8169 uses a value of 4K.
> 
> Is there a r8169 patch that adds uses of PCI_EXP_DEVCTL_READRQ_4096B?
> If so, we should probably keep it together with this one since this
> one would have to be merged first.
> 
Not yet, so far r8169 uses the following to set a MRRS of 4K.

#define MAX_READ_REQUEST_SHIFT	12
0x5 << MAX_READ_REQUEST_SHIFT

I have to check when setting 4K MRRS was added, but it seems
that so far it's fine with all users.

Before submitting the r8169 patch I wanted to check for feedback
regarding use of the two additional MRRS values, so what you write
in the following is exactly the type of feedback I was looking for.

I will read through the linked discussion and then also submit the
r8169 patch.

Thanks, Heiner

> There's also a larger issue in that when Linux configures MPS and MRRS
> at enumeration-time, it makes some assumptions about how MRRS is set.
> If a driver like r8169 changes MRRS later, it may break those
> assumptions, which might lead to PCIe errors.
> 
> If a user boots with "pci=pcie_bus_perf", we use PCIE_BUS_PERFORMANCE
> mode, and in that case we may set MPS to something larger than some
> devices can support, and we rely on MRRS to avoid problems.
> 
> I don't really *like* that scheme because it makes assumptions like
> "drivers never change MRRS", but that's what we have right now.
> 
> So just be aware that if r8169 changes MRRS and users boot with
> "pci=pcie_bus_perf", there is the potential for PCIe bus errors.
> 
> There was some recent discussion about this; [1] is a good place to
> start.
> 
> [1] http://lkml.kernel.org/r/20180119205153.GB160618@bhelgaas-glaptop.roam.corp.google.com
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  include/uapi/linux/pci_regs.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index 0c79eac5..699257fb 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -506,6 +506,8 @@
>>  #define  PCI_EXP_DEVCTL_READRQ_256B  0x1000 /* 256 Bytes */
>>  #define  PCI_EXP_DEVCTL_READRQ_512B  0x2000 /* 512 Bytes */
>>  #define  PCI_EXP_DEVCTL_READRQ_1024B 0x3000 /* 1024 Bytes */
>> +#define  PCI_EXP_DEVCTL_READRQ_2048B 0x4000 /* 2048 Bytes */
>> +#define  PCI_EXP_DEVCTL_READRQ_4096B 0x5000 /* 4096 Bytes */
>>  #define  PCI_EXP_DEVCTL_BCR_FLR 0x8000  /* Bridge Configuration Retry / FLR */
>>  #define PCI_EXP_DEVSTA		10	/* Device Status */
>>  #define  PCI_EXP_DEVSTA_CED	0x0001	/* Correctable Error Detected */
>> -- 
>> 2.17.0
>>
>
Heiner Kallweit April 16, 2018, 7:21 p.m. | #3
Am 13.04.2018 um 07:48 schrieb Heiner Kallweit:
> Am 13.04.2018 um 00:55 schrieb Bjorn Helgaas:
>> Hello Heiner,
>>
>> On Thu, Apr 12, 2018 at 11:08:04PM +0200, Heiner Kallweit wrote:
>>> This patch adds missing values for the max read request size.
>>> E.g. network driver r8169 uses a value of 4K.
>>
>> Is there a r8169 patch that adds uses of PCI_EXP_DEVCTL_READRQ_4096B?
>> If so, we should probably keep it together with this one since this
>> one would have to be merged first.
>>
> Not yet, so far r8169 uses the following to set a MRRS of 4K.
> 
> #define MAX_READ_REQUEST_SHIFT	12
> 0x5 << MAX_READ_REQUEST_SHIFT
> 
> I have to check when setting 4K MRRS was added, but it seems
> that so far it's fine with all users.
> 
> Before submitting the r8169 patch I wanted to check for feedback
> regarding use of the two additional MRRS values, so what you write
> in the following is exactly the type of feedback I was looking for.
> 
> I will read through the linked discussion and then also submit the
> r8169 patch.
> 
Hello Bjorn,

I checked and using 4K MRRS was added to the r8169 driver about
10 years ago. So it seems the described potential problems didn't
hit anybody in reality.

When submitting the patch series consisting of pci core change
and r8169 change I will address it to pci and netdev.
Then I leave it to David and you to agree on through which tree
the series should go.

To mention it briefly as I added David:
The patch won't include any functional change.

Regards, Heiner

> Thanks, Heiner
> 
>> There's also a larger issue in that when Linux configures MPS and MRRS
>> at enumeration-time, it makes some assumptions about how MRRS is set.
>> If a driver like r8169 changes MRRS later, it may break those
>> assumptions, which might lead to PCIe errors.
>>
>> If a user boots with "pci=pcie_bus_perf", we use PCIE_BUS_PERFORMANCE
>> mode, and in that case we may set MPS to something larger than some
>> devices can support, and we rely on MRRS to avoid problems.
>>
>> I don't really *like* that scheme because it makes assumptions like
>> "drivers never change MRRS", but that's what we have right now.
>>
>> So just be aware that if r8169 changes MRRS and users boot with
>> "pci=pcie_bus_perf", there is the potential for PCIe bus errors.
>>
>> There was some recent discussion about this; [1] is a good place to
>> start.
>>
>> [1] http://lkml.kernel.org/r/20180119205153.GB160618@bhelgaas-glaptop.roam.corp.google.com
>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  include/uapi/linux/pci_regs.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>>> index 0c79eac5..699257fb 100644
>>> --- a/include/uapi/linux/pci_regs.h
>>> +++ b/include/uapi/linux/pci_regs.h
>>> @@ -506,6 +506,8 @@
>>>  #define  PCI_EXP_DEVCTL_READRQ_256B  0x1000 /* 256 Bytes */
>>>  #define  PCI_EXP_DEVCTL_READRQ_512B  0x2000 /* 512 Bytes */
>>>  #define  PCI_EXP_DEVCTL_READRQ_1024B 0x3000 /* 1024 Bytes */
>>> +#define  PCI_EXP_DEVCTL_READRQ_2048B 0x4000 /* 2048 Bytes */
>>> +#define  PCI_EXP_DEVCTL_READRQ_4096B 0x5000 /* 4096 Bytes */
>>>  #define  PCI_EXP_DEVCTL_BCR_FLR 0x8000  /* Bridge Configuration Retry / FLR */
>>>  #define PCI_EXP_DEVSTA		10	/* Device Status */
>>>  #define  PCI_EXP_DEVSTA_CED	0x0001	/* Correctable Error Detected */
>>> -- 
>>> 2.17.0
>>>
>>
>

Patch

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 0c79eac5..699257fb 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -506,6 +506,8 @@ 
 #define  PCI_EXP_DEVCTL_READRQ_256B  0x1000 /* 256 Bytes */
 #define  PCI_EXP_DEVCTL_READRQ_512B  0x2000 /* 512 Bytes */
 #define  PCI_EXP_DEVCTL_READRQ_1024B 0x3000 /* 1024 Bytes */
+#define  PCI_EXP_DEVCTL_READRQ_2048B 0x4000 /* 2048 Bytes */
+#define  PCI_EXP_DEVCTL_READRQ_4096B 0x5000 /* 4096 Bytes */
 #define  PCI_EXP_DEVCTL_BCR_FLR 0x8000  /* Bridge Configuration Retry / FLR */
 #define PCI_EXP_DEVSTA		10	/* Device Status */
 #define  PCI_EXP_DEVSTA_CED	0x0001	/* Correctable Error Detected */