diff mbox series

[v2] PCIe: limit Max Read Request Size on i.MX to 512 bytes

Message ID m3k0klzl1x.fsf@t19.piap.pl (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [v2] PCIe: limit Max Read Request Size on i.MX to 512 bytes | expand

Commit Message

Krzysztof Hałasa Aug. 16, 2021, 11:27 a.m. UTC 
DWC PCIe controller imposes limits on the Read Request Size that it can
handle. For i.MX6 family it's fixed at 512 bytes by default.

If a memory read larger than the limit is requested, the CPU responds
with Completer Abort (CA) (on i.MX6 Unsupported Request (UR) is returned
instead due to a design error).

The i.MX6 documentation states that the limit can be changed by writing
to the PCIE_PL_MRCCR0 register, however there is a fixed (and
undocumented) maximum (CX_REMOTE_RD_REQ_SIZE constant). Tests indicate
that values larger than 512 bytes don't work, though.

This patch makes the RTL8111 work on i.MX6.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
---
This version drops CONFIG_NEED_PCIE_MAX_MRRS.

Comments

Rob Herring (Arm) Aug. 16, 2021, 7:34 p.m. UTC | #1 
On Mon, Aug 16, 2021 at 6:27 AM Krzysztof Hałasa <khalasa@piap.pl> wrote:
>
> DWC PCIe controller imposes limits on the Read Request Size that it can
> handle. For i.MX6 family it's fixed at 512 bytes by default.
>
> If a memory read larger than the limit is requested, the CPU responds
> with Completer Abort (CA) (on i.MX6 Unsupported Request (UR) is returned
> instead due to a design error).
>
> The i.MX6 documentation states that the limit can be changed by writing
> to the PCIE_PL_MRCCR0 register, however there is a fixed (and
> undocumented) maximum (CX_REMOTE_RD_REQ_SIZE constant). Tests indicate
> that values larger than 512 bytes don't work, though.
>
> This patch makes the RTL8111 work on i.MX6.
>
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> ---
> This version drops CONFIG_NEED_PCIE_MAX_MRRS.
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 80fc98acf097..225380e75fff 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1148,6 +1148,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>                 imx6_pcie->vph = NULL;
>         }
>
> +       max_pcie_mrrs = 512;
>         platform_set_drvdata(pdev, imx6_pcie);
>
>         ret = imx6_pcie_attach_pd(dev);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aacf575c15cf..44815af4ad85 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -112,6 +112,10 @@ enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_PEER2PEER;
>  enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_DEFAULT;
>  #endif
>
> +#ifdef CONFIG_ARM
> +u16 max_pcie_mrrs = 4096; // no limit - needed mostly for DWC PCIe
> +#endif
> +
>  /*
>   * The default CLS is used if arch didn't set CLS explicitly and not
>   * all pci devices agree on the same value.  Arch can override either
> @@ -5816,6 +5820,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>                         rq = mps;
>         }
>
> +#ifdef CONFIG_ARM
> +       if (rq > max_pcie_mrrs)
> +               rq = max_pcie_mrrs;
> +#endif

My objection wasn't having another kconfig option so much as I don't
think we need one at all here unless Bjorn feels otherwise. It's 2
bytes of data and about 3 instructions (load, cmp, store).

If we do have a config option, using or basing on the arch is wrong.
Has nothing to do with the arch. Are the other platforms needing this
arm32 as well?

Also, when you do use kconfig options, use IS_ENABLED() whenever possible.

Rob
kernel test robot Aug. 16, 2021, 11 p.m. UTC | #2 
Hi "Krzysztof,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v5.14-rc6 next-20210816]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Ha-asa/PCIe-limit-Max-Read-Request-Size-on-i-MX-to-512-bytes/20210816-193000
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/63e17da24ae7c5ee07f3a34fc32cff6455e5b0c5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Ha-asa/PCIe-limit-Max-Read-Request-Size-on-i-MX-to-512-bytes/20210816-193000
        git checkout 63e17da24ae7c5ee07f3a34fc32cff6455e5b0c5
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=parisc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   hppa-linux-ld: drivers/pci/controller/dwc/pci-imx6.o: in function `imx6_pcie_probe':
>> (.text+0x698): undefined reference to `max_pcie_mrrs'
>> hppa-linux-ld: (.text+0x6a4): undefined reference to `max_pcie_mrrs'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Aug. 17, 2021, 1:57 a.m. UTC | #3 
Hi "Krzysztof,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v5.14-rc6 next-20210816]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Ha-asa/PCIe-limit-Max-Read-Request-Size-on-i-MX-to-512-bytes/20210816-193000
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm64-randconfig-p001-20210816 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/63e17da24ae7c5ee07f3a34fc32cff6455e5b0c5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Ha-asa/PCIe-limit-Max-Read-Request-Size-on-i-MX-to-512-bytes/20210816-193000
        git checkout 63e17da24ae7c5ee07f3a34fc32cff6455e5b0c5
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   aarch64-linux-ld: drivers/pci/controller/dwc/pci-imx6.o: in function `imx6_pcie_probe':
   pci-imx6.c:(.text+0x7c8): undefined reference to `max_pcie_mrrs'
   aarch64-linux-ld: drivers/pci/controller/dwc/pci-imx6.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `max_pcie_mrrs' which may bind externally can not be used when making a shared object; recompile with -fPIC
   pci-imx6.c:(.text+0x7c8): dangerous relocation: unsupported relocation
>> aarch64-linux-ld: pci-imx6.c:(.text+0x7d4): undefined reference to `max_pcie_mrrs'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Krzysztof Hałasa Aug. 17, 2021, 4:29 a.m. UTC | #4 
Rob Herring <robh@kernel.org> writes:

>> +#ifdef CONFIG_ARM
>> +       if (rq > max_pcie_mrrs)
>> +               rq = max_pcie_mrrs;
>> +#endif
>
> My objection wasn't having another kconfig option so much as I don't
> think we need one at all here unless Bjorn feels otherwise. It's 2
> bytes of data and about 3 instructions (load, cmp, store).
>
> If we do have a config option, using or basing on the arch is wrong.
> Has nothing to do with the arch. Are the other platforms needing this
> arm32 as well?

Yes,

I can buy the "universal ARM32 kernel" argument, but otherwise it's just
nonfunctional bloat. A small one, yes.
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 80fc98acf097..225380e75fff 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1148,6 +1148,7 @@  static int imx6_pcie_probe(struct platform_device *pdev)
 		imx6_pcie->vph = NULL;
 	}
 
+	max_pcie_mrrs = 512;
 	platform_set_drvdata(pdev, imx6_pcie);
 
 	ret = imx6_pcie_attach_pd(dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aacf575c15cf..44815af4ad85 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -112,6 +112,10 @@  enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_PEER2PEER;
 enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_DEFAULT;
 #endif
 
+#ifdef CONFIG_ARM
+u16 max_pcie_mrrs = 4096; // no limit - needed mostly for DWC PCIe
+#endif
+
 /*
  * The default CLS is used if arch didn't set CLS explicitly and not
  * all pci devices agree on the same value.  Arch can override either
@@ -5816,6 +5820,11 @@  int pcie_set_readrq(struct pci_dev *dev, int rq)
 			rq = mps;
 	}
 
+#ifdef CONFIG_ARM
+	if (rq > max_pcie_mrrs)
+		rq = max_pcie_mrrs;
+#endif
+
 	v = (ffs(rq) - 8) << 12;
 
 	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 06ff1186c1ef..1f21ec662b8e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -996,6 +996,7 @@  enum pcie_bus_config_types {
 };
 
 extern enum pcie_bus_config_types pcie_bus_config;
+extern u16 max_pcie_mrrs;	// currently ARM only
 
 extern struct bus_type pci_bus_type;