[PATCH v2 0/3] Fix DT based address translations

[PATCH v2 0/3] Fix DT based address translations

[Herve Codina]

Hi,

This patch series fixes a DT based address translation (translations
using the ranges property).

The issue is present with a ranges property made of a 3 cells child
address, a 3 cells parent address and a 2 cells child size.
This can happen with the recent addition of of_pci_prop_ranges() in
commit 407d1a51921e ("PCI: Create device tree node for bridge")

The issue description is fully described in the first patch commit log.

In this series,
  - The first patch fixes the issue.
  - The second patch avoids duplicated code.
  - The third patch adds unit tests related address translations.

I previously sent the first patch alone:
  https://lore.kernel.org/linux-kernel/20231003065236.121987-1-herve.codina@bootlin.com/
This series v2 has to be considered as the next iteration based on the
review done on my previous patch sent alone.

Best regards,
Hervé

Changes v1 -> v2

 - Patch 1
   Simplify of_bus_default_flags_map().
   Fix the commit log (pci-ep-bus ranges[0] size is 0x200_0000 instead
   of 0x2000_0000).

 - Patch 2 (new in v2)
   Remove duplicated code.

 - Patch 3 (new in v2)
   Add unit tests.

Herve Codina (3):
  of: address: Fix address translation when address-size is greater than
    2
  of: address: Remove duplicated functions
  of: unittest: Add tests for address translations

 drivers/of/address.c                        |  43 ++++++---
 drivers/of/unittest-data/tests-address.dtsi | 101 ++++++++++++++++++++
 drivers/of/unittest.c                       |  74 ++++++++++++++
 3 files changed, 205 insertions(+), 13 deletions(-)

-- 
2.41.0

[PATCH v2 1/3] of: address: Fix address translation when address-size is greater than 2

[Herve Codina]

With the recent addition of of_pci_prop_ranges() in commit 407d1a51921e
("PCI: Create device tree node for bridge"), the ranges property can
have a 3 cells child address, a 3 cells parent address and a 2 cells
child size.

A range item property for a PCI device is filled as follow:
  <BAR_nbr> 0 0 <phys.hi> <phys.mid> <phys.low> <BAR_sizeh> <BAR_sizel>
  <-- Child --> <-- Parent (PCI definition) --> <- BAR size (64bit) -->

This allow to translate BAR addresses from the DT. For instance:
pci@0,0 {
  #address-cells = <0x03>;
  #size-cells = <0x02>;
  device_type = "pci";
  compatible = "pci11ab,100\0pciclass,060400\0pciclass,0604";
  ranges = <0x82000000 0x00 0xe8000000
            0x82000000 0x00 0xe8000000
	    0x00 0x4400000>;
  ...
  dev@0,0 {
    #address-cells = <0x03>;
    #size-cells = <0x02>;
    compatible = "pci1055,9660\0pciclass,020000\0pciclass,0200";
    /* Translations for BAR0 to BAR5 */
    ranges = <0x00 0x00 0x00 0x82010000 0x00 0xe8000000 0x00 0x2000000
              0x01 0x00 0x00 0x82010000 0x00 0xea000000 0x00 0x1000000
              0x02 0x00 0x00 0x82010000 0x00 0xeb000000 0x00 0x800000
              0x03 0x00 0x00 0x82010000 0x00 0xeb800000 0x00 0x800000
              0x04 0x00 0x00 0x82010000 0x00 0xec000000 0x00 0x20000
              0x05 0x00 0x00 0x82010000 0x00 0xec020000 0x00 0x2000>;
    ...
    pci-ep-bus@0 {
      #address-cells = <0x01>;
      #size-cells = <0x01>;
      compatible = "simple-bus";
      /* Translate 0xe2000000 to BAR0 and 0xe0000000 to BAR1 */
      ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
                0xe0000000 0x01 0x00 0x00 0x1000000>;
      ...
    };
  };
};

During the translation process, the "default-flags" map() function is
used to select the matching item in the ranges table and determine the
address offset from this matching item.
This map() function simply calls of_read_number() and when address-size
is greater than 2, the map() function skips the extra high address part
(ie part over 64bit). This lead to a wrong matching item and a wrong
offset computation.
Also during the translation itself, the extra high part related to the
parent address is not present in the translated address.

Fix the "default-flags" map() and translate() in order to take into
account the child extra high address part in map() and the parent extra
high address part in translate() and so having a correct address
translation for ranges patterns such as the one given in the example
above.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/address.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index e692809ff822..3219c5177750 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -100,6 +100,32 @@ static unsigned int of_bus_default_get_flags(const __be32 *addr)
 	return IORESOURCE_MEM;
 }
 
+static u64 of_bus_default_flags_map(__be32 *addr, const __be32 *range, int na,
+				    int ns, int pna)
+{
+	u64 cp, s, da;
+
+	/* Check that flags match */
+	if (*addr != *range)
+		return OF_BAD_ADDR;
+
+	/* Read address values, skipping high cell */
+	cp = of_read_number(range + 1, na - 1);
+	s  = of_read_number(range + na + pna, ns);
+	da = of_read_number(addr + 1, na - 1);
+
+	pr_debug("default flags map, cp=%llx, s=%llx, da=%llx\n", cp, s, da);
+
+	if (da < cp || da >= (cp + s))
+		return OF_BAD_ADDR;
+	return da - cp;
+}
+
+static int of_bus_default_flags_translate(__be32 *addr, u64 offset, int na)
+{
+	/* Keep "flags" part (high cell) in translated address */
+	return of_bus_default_translate(addr + 1, offset, na - 1);
+}
 
 #ifdef CONFIG_PCI
 static unsigned int of_bus_pci_get_flags(const __be32 *addr)
@@ -374,8 +400,8 @@ static struct of_bus of_busses[] = {
 		.addresses = "reg",
 		.match = of_bus_default_flags_match,
 		.count_cells = of_bus_default_count_cells,
-		.map = of_bus_default_map,
-		.translate = of_bus_default_translate,
+		.map = of_bus_default_flags_map,
+		.translate = of_bus_default_flags_translate,
 		.has_flags = true,
 		.get_flags = of_bus_default_flags_get_flags,
 	},
-- 
2.41.0

[PATCH v2 2/3] of: address: Remove duplicated functions

[Herve Codina]

The recently added of_bus_default_flags_translate() performs the exact
same operation as of_bus_pci_translate() and of_bus_isa_translate().

Avoid duplicated code replacing both of_bus_pci_translate() and
of_bus_isa_translate() with of_bus_default_flags_translate().

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/address.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 3219c5177750..d21a3b74ac56 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -216,10 +216,6 @@ static u64 of_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns,
 	return da - cp;
 }
 
-static int of_bus_pci_translate(__be32 *addr, u64 offset, int na)
-{
-	return of_bus_default_translate(addr + 1, offset, na - 1);
-}
 #endif /* CONFIG_PCI */
 
 /*
@@ -343,11 +339,6 @@ static u64 of_bus_isa_map(__be32 *addr, const __be32 *range, int na, int ns,
 	return da - cp;
 }
 
-static int of_bus_isa_translate(__be32 *addr, u64 offset, int na)
-{
-	return of_bus_default_translate(addr + 1, offset, na - 1);
-}
-
 static unsigned int of_bus_isa_get_flags(const __be32 *addr)
 {
 	unsigned int flags = 0;
@@ -378,7 +369,7 @@ static struct of_bus of_busses[] = {
 		.match = of_bus_pci_match,
 		.count_cells = of_bus_pci_count_cells,
 		.map = of_bus_pci_map,
-		.translate = of_bus_pci_translate,
+		.translate = of_bus_default_flags_translate,
 		.has_flags = true,
 		.get_flags = of_bus_pci_get_flags,
 	},
@@ -390,7 +381,7 @@ static struct of_bus of_busses[] = {
 		.match = of_bus_isa_match,
 		.count_cells = of_bus_isa_count_cells,
 		.map = of_bus_isa_map,
-		.translate = of_bus_isa_translate,
+		.translate = of_bus_default_flags_translate,
 		.has_flags = true,
 		.get_flags = of_bus_isa_get_flags,
 	},
-- 
2.41.0

[PATCH v2 3/3] of: unittest: Add tests for address translations

[Herve Codina]

Add tests to exercise address translations based on ranges properties.

Tests added cover "default" (2cell) address translations, "default
flags" (3cell) address translations and PCI address translations.
They also cover PCI BAR translations introduced in commit 407d1a51921e
("PCI: Create device tree node for bridge").

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/unittest-data/tests-address.dtsi | 101 ++++++++++++++++++++
 drivers/of/unittest.c                       |  74 ++++++++++++++
 2 files changed, 175 insertions(+)

diff --git a/drivers/of/unittest-data/tests-address.dtsi b/drivers/of/unittest-data/tests-address.dtsi
index bc0029cbf8ea..3344f15c3755 100644
--- a/drivers/of/unittest-data/tests-address.dtsi
+++ b/drivers/of/unittest-data/tests-address.dtsi
@@ -51,5 +51,106 @@ bus@a0000000 {
 			};
 
 		};
+
+		address-tests2 {
+			#address-cells = <2>;
+			#size-cells = <1>;
+
+			ranges = <0x10000000 0x01000000 0xa0000000 0x01000000>,
+				 <0x10000000 0x02000000 0xb0000000 0x01000000>,
+				 <0x20000000 0x01000000 0xc0000000 0x01000000>,
+				 <0x20000000 0x02000000 0xd0000000 0x01000000>,
+				 <0x00000000 0xd1000000 0xd1000000 0x01000000>,
+				 <0x00000000 0xe8000000 0xe8000000 0x07f00000>,
+				 <0x00000000 0xefff0000 0xefff0000 0x00010000>;
+
+			bus-2cell@10000000 {
+				#address-cells = <2>;
+				#size-cells = <1>;
+				ranges = <0x100000 0x10000 0x10000000 0x1a00000 0x10000>,
+					 <0x100000 0x20000 0x10000000 0x1b00000 0x10000>,
+					 <0x200000 0x10000 0x20000000 0x1c00000 0x10000>,
+					 <0x200000 0x20000 0x20000000 0x2d00000 0x10000>;
+
+				device@100000 {
+					reg = <0x100000 0x11000 0x100>,
+					      <0x100000 0x12000 0x100>,
+					      <0x200000 0x11000 0x100>,
+					      <0x200000 0x21000 0x100>;
+				};
+			};
+
+			bus-3cell@20000000 {
+				#address-cells = <3>;
+				#size-cells = <1>;
+				ranges = <0x1 0x100000 0x10000 0x10000000 0x1a00000 0x10000>,
+					 <0x2 0x100000 0x10000 0x10000000 0x1b00000 0x10000>,
+					 <0x3 0x200000 0x10000 0x20000000 0x1c00000 0x10000>,
+					 <0x4 0x200000 0x20000 0x20000000 0x2d00000 0x10000>;
+
+				local-bus@100000 {
+					#address-cells = <1>;
+					#size-cells = <1>;
+					ranges = <0xf1000000 0x1 0x100000 0x10000 0x10000>,
+						 <0xf2000000 0x2 0x100000 0x10000 0x10000>,
+						 <0xf3000000 0x3 0x200000 0x10000 0x08000>,
+						 <0xf3800000 0x3 0x200000 0x18000 0x08000>,
+						 <0xf4000000 0x4 0x200000 0x20000 0x10000>;
+
+					device@f1001000 {
+						reg = <0xf1001000 0x100>,
+						      <0xf2002000 0x100>,
+						      <0xf3001000 0x100>,
+						      <0xf3801000 0x100>,
+						      <0xf4001000 0x100>;
+					};
+				};
+			};
+
+			pcie@d1070000 {
+				#address-cells = <0x03>;
+				#size-cells = <0x02>;
+				bus-range = <0x00 0xff>;
+				device_type = "pci";
+				ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x7f00000>,
+					 <0x81000000 0 0x00000000 0 0xefff0000 0 0x0010000>;
+				reg = <0x00000000 0xd1070000 0x20000>;
+
+				pci@0,0 {
+					#address-cells = <0x03>;
+					#size-cells = <0x02>;
+					bus-range = <0x01 0x01>;
+					device_type = "pci";
+					ranges = <0x82000000 0 0xe8000000
+						  0x82000000 0 0xe8000000
+						  0 0x4400000>;
+					reg = <0x00 0x00 0x00 0x00 0x00>;
+
+					dev@0,0 {
+						#address-cells = <0x03>;
+						#size-cells = <0x02>;
+						ranges = <0 0 0 0x82010000 0 0xe8000000 0 0x2000000>,
+							 <1 0 0 0x82010000 0 0xea000000 0 0x1000000>,
+							 <2 0 0 0x82010000 0 0xeb000000 0 0x0800000>,
+							 <3 0 0 0x82010000 0 0xeb800000 0 0x0800000>,
+							 <4 0 0 0x82010000 0 0xec000000 0 0x0020000>,
+							 <5 0 0 0x82010000 0 0xec020000 0 0x0002000>;
+						reg = <0x10000 0x00 0x00 0x00 0x00>;
+
+						local-bus@0 {
+							#address-cells = <0x01>;
+							#size-cells = <0x01>;
+							ranges = <0xa0000000 0 0 0 0x2000000>,
+								 <0xb0000000 1 0 0 0x1000000>;
+
+							dev@e0000000 {
+								reg = <0xa0001000 0x1000>,
+								      <0xb0002000 0x2000>;
+							};
+						};
+					};
+				};
+			};
+		};
 	};
 };
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index ad2b7879cc67..ffbecd349899 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1186,6 +1186,79 @@ static void __init of_unittest_reg(void)
 	of_node_put(np);
 }
 
+struct of_unittest_expected_res {
+	int index;
+	struct resource res;
+};
+
+static void __init of_unittest_check_addr(const char *node_path,
+					  const struct of_unittest_expected_res *tab_exp,
+					  unsigned int tab_exp_count)
+{
+	const struct of_unittest_expected_res *expected;
+	struct device_node *np;
+	struct resource res;
+	unsigned int count;
+	int ret;
+
+	np = of_find_node_by_path(node_path);
+	if (!np) {
+		pr_err("missing testcase data (%s)\n", node_path);
+		return;
+	}
+
+	expected = tab_exp;
+	count = tab_exp_count;
+	while (count--) {
+		ret = of_address_to_resource(np, expected->index, &res);
+		unittest(!ret, "of_address_to_resource(%pOF, %d) returned error %d\n",
+			 np, expected->index, ret);
+		unittest(resource_type(&res) == resource_type(&expected->res) &&
+			 res.start == expected->res.start &&
+			 resource_size(&res) == resource_size(&expected->res),
+			"of_address_to_resource(%pOF, %d) wrong resource %pR, expected %pR\n",
+			np, expected->index, &res, &expected->res);
+		expected++;
+	}
+
+	of_node_put(np);
+}
+
+static const struct of_unittest_expected_res of_unittest_reg_2cell_expected_res[] = {
+	{.index = 0, .res = DEFINE_RES_MEM(0xa0a01000, 0x100) },
+	{.index = 1, .res = DEFINE_RES_MEM(0xa0a02000, 0x100) },
+	{.index = 2, .res = DEFINE_RES_MEM(0xc0c01000, 0x100) },
+	{.index = 3, .res = DEFINE_RES_MEM(0xd0d01000, 0x100) },
+};
+
+static const struct of_unittest_expected_res of_unittest_reg_3cell_expected_res[] = {
+	{.index = 0, .res = DEFINE_RES_MEM(0xa0a01000, 0x100) },
+	{.index = 1, .res = DEFINE_RES_MEM(0xa0b02000, 0x100) },
+	{.index = 2, .res = DEFINE_RES_MEM(0xc0c01000, 0x100) },
+	{.index = 3, .res = DEFINE_RES_MEM(0xc0c09000, 0x100) },
+	{.index = 4, .res = DEFINE_RES_MEM(0xd0d01000, 0x100) },
+};
+
+static const struct of_unittest_expected_res of_unittest_reg_pci_expected_res[] = {
+	{.index = 0, .res = DEFINE_RES_MEM(0xe8001000, 0x1000) },
+	{.index = 1, .res = DEFINE_RES_MEM(0xea002000, 0x2000) },
+};
+
+static void __init of_unittest_translate_addr(void)
+{
+	of_unittest_check_addr("/testcase-data/address-tests2/bus-2cell@10000000/device@100000",
+			       of_unittest_reg_2cell_expected_res,
+			       ARRAY_SIZE(of_unittest_reg_2cell_expected_res));
+
+	of_unittest_check_addr("/testcase-data/address-tests2/bus-3cell@20000000/local-bus@100000/device@f1001000",
+			       of_unittest_reg_3cell_expected_res,
+			       ARRAY_SIZE(of_unittest_reg_3cell_expected_res));
+
+	of_unittest_check_addr("/testcase-data/address-tests2/pcie@d1070000/pci@0,0/dev@0,0/local-bus@0/dev@e0000000",
+			       of_unittest_reg_pci_expected_res,
+			       ARRAY_SIZE(of_unittest_reg_pci_expected_res));
+}
+
 static void __init of_unittest_parse_interrupts(void)
 {
 	struct device_node *np;
@@ -4034,6 +4107,7 @@ static int __init of_unittest(void)
 	of_unittest_bus_ranges();
 	of_unittest_bus_3cell_ranges();
 	of_unittest_reg();
+	of_unittest_translate_addr();
 	of_unittest_match_node();
 	of_unittest_platform_populate();
 	of_unittest_overlay();
-- 
2.41.0

Re: [PATCH v2 0/3] Fix DT based address translations

[Rob Herring]

On Tue, Oct 17, 2023 at 01:02:15PM +0200, Herve Codina wrote:
> Hi,
> 
> This patch series fixes a DT based address translation (translations
> using the ranges property).
> 
> The issue is present with a ranges property made of a 3 cells child
> address, a 3 cells parent address and a 2 cells child size.
> This can happen with the recent addition of of_pci_prop_ranges() in
> commit 407d1a51921e ("PCI: Create device tree node for bridge")
> 
> The issue description is fully described in the first patch commit log.
> 
> In this series,
>   - The first patch fixes the issue.
>   - The second patch avoids duplicated code.
>   - The third patch adds unit tests related address translations.
> 
> I previously sent the first patch alone:
>   https://lore.kernel.org/linux-kernel/20231003065236.121987-1-herve.codina@bootlin.com/
> This series v2 has to be considered as the next iteration based on the
> review done on my previous patch sent alone.
> 
> Best regards,
> Hervé
> 
> Changes v1 -> v2
> 
>  - Patch 1
>    Simplify of_bus_default_flags_map().
>    Fix the commit log (pci-ep-bus ranges[0] size is 0x200_0000 instead
>    of 0x2000_0000).
> 
>  - Patch 2 (new in v2)
>    Remove duplicated code.
> 
>  - Patch 3 (new in v2)
>    Add unit tests.
> 
> Herve Codina (3):
>   of: address: Fix address translation when address-size is greater than
>     2
>   of: address: Remove duplicated functions
>   of: unittest: Add tests for address translations
> 
>  drivers/of/address.c                        |  43 ++++++---
>  drivers/of/unittest-data/tests-address.dtsi | 101 ++++++++++++++++++++
>  drivers/of/unittest.c                       |  74 ++++++++++++++
>  3 files changed, 205 insertions(+), 13 deletions(-)

I've applied this. I sent out this series[1] of clean-ups to apply on 
top.

Rob

[1] https://lore.kernel.org/all/20231026135358.3564307-2-robh@kernel.org/

Re: [PATCH v2 3/3] of: unittest: Add tests for address translations

[kernel test robot]

Hi Herve,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.6-rc7 next-20231026]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Herve-Codina/of-address-Fix-address-translation-when-address-size-is-greater-than-2/20231017-190414
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231017110221.189299-4-herve.codina%40bootlin.com
patch subject: [PATCH v2 3/3] of: unittest: Add tests for address translations
config: s390-randconfig-002-20231027 (https://download.01.org/0day-ci/archive/20231027/202310271513.8MUErCK4-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231027/202310271513.8MUErCK4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310271513.8MUErCK4-lkp@intel.com/

All errors (new ones prefixed by >>):

   s390-linux-ld: drivers/of/unittest.o: in function `of_unittest_check_addr':
>> unittest.c:(.init.text+0x5ad2): undefined reference to `of_address_to_resource'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki