[PATCH v4 00/14] of: fix bugs and improve codes

[PATCH v4 00/14] of: fix bugs and improve codes

[Zijun Hu]

This patch series is to fix bugs and improve codes for drivers/of/*.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v4:
- Remove 2 modalias relevant patches, and add more patches.
- Link to v3: https://lore.kernel.org/r/20241217-of_core_fix-v3-0-3bc49a2e8bda@quicinc.com

Changes in v3:
- Drop 2 applied patches and pick up patch 4/7 again
- Fix build error for patch 6/7.
- Include of_private.h instead of function declaration for patch 2/7
- Correct tile and commit messages.
- Link to v2: https://lore.kernel.org/r/20241216-of_core_fix-v2-0-e69b8f60da63@quicinc.com

Changes in v2:
- Drop applied/conflict/TBD patches.
- Correct based on Rob's comments.
- Link to v1: https://lore.kernel.org/r/20241206-of_core_fix-v1-0-dc28ed56bec3@quicinc.com

---
Zijun Hu (14):
      of: Correct child specifier used as input of the 2nd nexus node
      of: Do not expose of_alias_scan() and correct its comments
      of: Make of_property_present() applicable to all kinds of property
      of: property: Use of_property_present() for of_fwnode_property_present()
      of: Fix available buffer size calculating error in API of_device_uevent_modalias()
      of: property: Avoiding using uninitialized variable @imaplen in parse_interrupt_map()
      of: property: Fix potential fwnode reference's argument count got out of range
      of: Remove a duplicated code block
      of: reserved-memory: Fix using wrong number of cells to get property 'alignment'
      of: reserved-memory: Do not make kmemleak ignore freed address
      of: reserved-memory: Warn for missing static reserved memory regions
      of: reserved-memory: Move an assignment to effective place in __reserved_mem_alloc_size()
      of/fdt: Check fdt_get_mem_rsv() error in early_init_fdt_scan_reserved_mem()
      of: Improve __of_add_property_sysfs() readability

 drivers/of/address.c         | 21 +++------------------
 drivers/of/base.c            |  7 +++----
 drivers/of/device.c          | 14 ++++++++++----
 drivers/of/fdt.c             |  7 ++++++-
 drivers/of/fdt_address.c     | 21 ++++-----------------
 drivers/of/kobj.c            |  3 ++-
 drivers/of/of_private.h      | 20 ++++++++++++++++++++
 drivers/of/of_reserved_mem.c | 15 ++++++++++-----
 drivers/of/pdt.c             |  2 ++
 drivers/of/property.c        |  9 +++++++--
 include/linux/of.h           | 24 ++++++++++++------------
 11 files changed, 79 insertions(+), 64 deletions(-)
---
base-commit: 456f3000f82571697d23c255c451cfcfb5c9ae75
change-id: 20241206-of_core_fix-dc3021a06418

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH v4 01/14] of: Correct child specifier used as input of the 2nd nexus node

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

API of_parse_phandle_with_args_map() will use wrong input for nexus node
Nexus_2 as shown below:

    Node_1		Nexus_1                              Nexus_2
&Nexus_1,arg_1 -> arg_1,&Nexus_2,arg_2' -> &Nexus_2,arg_2 -> arg_2,...
		  map-pass-thru=<...>

Nexus_1's output arg_2 should be used as input of Nexus_2, but the API
wrongly uses arg_2' instead which != arg_2 due to Nexus_1's map-pass-thru.

Fix by always making @match_array point to @initial_match_array into
which to store nexus output.

Fixes: bd6f2fd5a1d5 ("of: Support parsing phandle argument lists through a nexus node")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index bf18d5997770eb81e47e749198dd505a35203d10..969b99838655534915882abe358814d505c6f748 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1536,7 +1536,6 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 		 * specifier into the out_args structure, keeping the
 		 * bits specified in <list>-map-pass-thru.
 		 */
-		match_array = map - new_size;
 		for (i = 0; i < new_size; i++) {
 			__be32 val = *(map - new_size + i);
 
@@ -1545,6 +1544,7 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 				val |= cpu_to_be32(out_args->args[i]) & pass[i];
 			}
 
+			initial_match_array[i] = val;
 			out_args->args[i] = be32_to_cpu(val);
 		}
 		out_args->args_count = list_size = new_size;

-- 
2.34.1

[PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

According to DT spec, size of property 'alignment' is based on parent
node’s #size-cells property.

But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
the property obviously.

Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.

Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/of_reserved_mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
 
 	prop = of_get_flat_dt_prop(node, "alignment", &len);
 	if (prop) {
-		if (len != dt_root_addr_cells * sizeof(__be32)) {
+		if (len != dt_root_size_cells * sizeof(__be32)) {
 			pr_err("invalid alignment property in '%s' node.\n",
 				uname);
 			return -EINVAL;
 		}
-		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
+		align = dt_mem_next_cell(dt_root_size_cells, &prop);
 	}
 
 	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;

-- 
2.34.1

[PATCH v4 11/14] of: reserved-memory: Warn for missing static reserved memory regions

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For child node of /reserved-memory, its property 'reg' may contain
multiple regions, but fdt_scan_reserved_mem_reg_nodes() only takes
into account the first region, and miss remaining regions.

Give warning message when missing remaining regions.

Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved memory regions are processed")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/of_reserved_mem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 03a8f03ed1da165d6d7bf907d931857260888225..e2da88d7706ab3c208386e29f31350178e67cd14 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -263,6 +263,11 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
 			       uname);
 			continue;
 		}
+
+		if (len > t_len)
+			pr_warn("%s() ignores %d regions in node '%s'\n",
+				__func__, len / t_len - 1, uname);
+
 		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
 		size = dt_mem_next_cell(dt_root_size_cells, &prop);
 

-- 
2.34.1

Re: [PATCH v4 01/14] of: Correct child specifier used as input of the 2nd nexus node

[Rob Herring (Arm)]

On Thu, 09 Jan 2025 21:26:52 +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> API of_parse_phandle_with_args_map() will use wrong input for nexus node
> Nexus_2 as shown below:
> 
>     Node_1		Nexus_1                              Nexus_2
> &Nexus_1,arg_1 -> arg_1,&Nexus_2,arg_2' -> &Nexus_2,arg_2 -> arg_2,...
> 		  map-pass-thru=<...>
> 
> Nexus_1's output arg_2 should be used as input of Nexus_2, but the API
> wrongly uses arg_2' instead which != arg_2 due to Nexus_1's map-pass-thru.
> 
> Fix by always making @match_array point to @initial_match_array into
> which to store nexus output.
> 
> Fixes: bd6f2fd5a1d5 ("of: Support parsing phandle argument lists through a nexus node")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks!

Re: [PATCH v4 11/14] of: reserved-memory: Warn for missing static reserved memory regions

[Rob Herring]

On Thu, Jan 09, 2025 at 09:27:02PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For child node of /reserved-memory, its property 'reg' may contain
> multiple regions, but fdt_scan_reserved_mem_reg_nodes() only takes
> into account the first region, and miss remaining regions.
> 
> Give warning message when missing remaining regions.

Can't we just fix it to support more than 1 entry?

> 
> Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved memory regions are processed")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/of_reserved_mem.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 03a8f03ed1da165d6d7bf907d931857260888225..e2da88d7706ab3c208386e29f31350178e67cd14 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -263,6 +263,11 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
>  			       uname);
>  			continue;
>  		}
> +
> +		if (len > t_len)
> +			pr_warn("%s() ignores %d regions in node '%s'\n",
> +				__func__, len / t_len - 1, uname);
> +
>  		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
>  		size = dt_mem_next_cell(dt_root_size_cells, &prop);
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Rob Herring]

On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> According to DT spec, size of property 'alignment' is based on parent
> node’s #size-cells property.
> 
> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> the property obviously.
> 
> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.

I wonder if changing this might break someone. It's been this way for 
a long time. It might be better to change the spec or just read 
'alignment' as whatever size it happens to be (len / 4). It's not really 
the kernel's job to validate the DT. We should first have some 
validation in place to *know* if there are any current .dts files that 
would break. That would probably be easier to implement in dtc than 
dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
but that was the default for OpenFirmware.

As the alignment is the base address alignment, it can be argued that 
"#address-cells" makes more sense to use than "#size-cells". So maybe 
the spec was a copy-n-paste error.

> 
> Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/of/of_reserved_mem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
>  
>  	prop = of_get_flat_dt_prop(node, "alignment", &len);
>  	if (prop) {
> -		if (len != dt_root_addr_cells * sizeof(__be32)) {
> +		if (len != dt_root_size_cells * sizeof(__be32)) {
>  			pr_err("invalid alignment property in '%s' node.\n",
>  				uname);
>  			return -EINVAL;
>  		}
> -		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +		align = dt_mem_next_cell(dt_root_size_cells, &prop);
>  	}
>  
>  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 11/14] of: reserved-memory: Warn for missing static reserved memory regions

[Zijun Hu]

On 2025/1/14 07:16, Rob Herring wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> For child node of /reserved-memory, its property 'reg' may contain
>> multiple regions, but fdt_scan_reserved_mem_reg_nodes() only takes
>> into account the first region, and miss remaining regions.
>>
>> Give warning message when missing remaining regions.
> Can't we just fix it to support more than 1 entry?

actually, i ever considered supporting more entries.
and find out there are no simple approach to achieve this aim.

it may need to change 'struct reserved_mem' to support multiple regions
that may also involve 4 RESERVEDMEM_OF_DECLARE instances.

RESERVEDMEM_OF_DECLARE(tegra210_emc_table, "nvidia,tegra210-emc-table",
                       tegra210_emc_table_init);
RESERVEDMEM_OF_DECLARE(dma, "shared-dma-pool", rmem_dma_setup);
RESERVEDMEM_OF_DECLARE(cma, "shared-dma-pool", rmem_cma_setup);
RESERVEDMEM_OF_DECLARE(dma, "restricted-dma-pool", rmem_swiotlb_setup);

so i finally chose this warning way due to simplification and low risk.

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[William McVicker]

Hi Zijun and Rob,

On 01/13/2025, Rob Herring wrote:
> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> > From: Zijun Hu <quic_zijuhu@quicinc.com>
> > 
> > According to DT spec, size of property 'alignment' is based on parent
> > node’s #size-cells property.
> > 
> > But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> > the property obviously.
> > 
> > Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
> 
> I wonder if changing this might break someone. It's been this way for 
> a long time. It might be better to change the spec or just read 
> 'alignment' as whatever size it happens to be (len / 4). It's not really 
> the kernel's job to validate the DT. We should first have some 
> validation in place to *know* if there are any current .dts files that 
> would break. That would probably be easier to implement in dtc than 
> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
> but that was the default for OpenFirmware.
> 
> As the alignment is the base address alignment, it can be argued that 
> "#address-cells" makes more sense to use than "#size-cells". So maybe 
> the spec was a copy-n-paste error.

Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
tree has cases where #address-cells != #size-cells.

I would prefer to not have this change, but if that's not possible, could we
not backport it to all the stable branches? That way we can just force new
devices to fix this instead of existing devices on older LTS kernels?

Thanks,
Will

> 
> > 
> > Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> > ---
> >  drivers/of/of_reserved_mem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
> > --- a/drivers/of/of_reserved_mem.c
> > +++ b/drivers/of/of_reserved_mem.c
> > @@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
> >  
> >  	prop = of_get_flat_dt_prop(node, "alignment", &len);
> >  	if (prop) {
> > -		if (len != dt_root_addr_cells * sizeof(__be32)) {
> > +		if (len != dt_root_size_cells * sizeof(__be32)) {
> >  			pr_err("invalid alignment property in '%s' node.\n",
> >  				uname);
> >  			return -EINVAL;
> >  		}
> > -		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > +		align = dt_mem_next_cell(dt_root_size_cells, &prop);
> >  	}
> >  
> >  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> > 
> > -- 
> > 2.34.1
> > 

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2/25/2025 9:18 AM, William McVicker wrote:
> Hi Zijun and Rob,
> 
> On 01/13/2025, Rob Herring wrote:
>> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>
>>> According to DT spec, size of property 'alignment' is based on parent
>>> node’s #size-cells property.
>>>
>>> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
>>> the property obviously.
>>>
>>> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
>>
>> I wonder if changing this might break someone. It's been this way for 
>> a long time. It might be better to change the spec or just read 
>> 'alignment' as whatever size it happens to be (len / 4). It's not really 
>> the kernel's job to validate the DT. We should first have some 
>> validation in place to *know* if there are any current .dts files that 
>> would break. That would probably be easier to implement in dtc than 
>> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
>> but that was the default for OpenFirmware.
>>
>> As the alignment is the base address alignment, it can be argued that 
>> "#address-cells" makes more sense to use than "#size-cells". So maybe 
>> the spec was a copy-n-paste error.
> 
> Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
> tree has cases where #address-cells != #size-cells.
> 

it seems upstream upstream Pixel 6 has no property 'alignment'
git grep alignment arch/arm64/boot/dts/exynos/google/
so it should not be broken.

> I would prefer to not have this change, but if that's not possible, could we
> not backport it to all the stable branches? That way we can just force new
> devices to fix this instead of existing devices on older LTS kernels?
> 

the fix have stable and fix tags. not sure if we can control its
backporting. the fix has been backported to 6.1/6.6/6.12/6.13 automatically.


> Thanks,
> Will
> 
>>
>>>
>>> Fixes: 3f0c82066448 ("drivers: of: add initialization code for dynamic reserved memory")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>> ---
>>>  drivers/of/of_reserved_mem.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>>> index 45517b9e57b1add36bdf2109227ebbf7df631a66..d2753756d7c30adcbd52f57338e281c16d821488 100644
>>> --- a/drivers/of/of_reserved_mem.c
>>> +++ b/drivers/of/of_reserved_mem.c
>>> @@ -409,12 +409,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
>>>  
>>>  	prop = of_get_flat_dt_prop(node, "alignment", &len);
>>>  	if (prop) {
>>> -		if (len != dt_root_addr_cells * sizeof(__be32)) {
>>> +		if (len != dt_root_size_cells * sizeof(__be32)) {
>>>  			pr_err("invalid alignment property in '%s' node.\n",
>>>  				uname);
>>>  			return -EINVAL;
>>>  		}
>>> -		align = dt_mem_next_cell(dt_root_addr_cells, &prop);
>>> +		align = dt_mem_next_cell(dt_root_size_cells, &prop);
>>>  	}
>>>  
>>>  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
>>>
>>> -- 
>>> 2.34.1
>>>

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[William McVicker]

On 02/25/2025, Zijun Hu wrote:
> On 2/25/2025 9:18 AM, William McVicker wrote:
> > Hi Zijun and Rob,
> > 
> > On 01/13/2025, Rob Herring wrote:
> >> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> >>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>
> >>> According to DT spec, size of property 'alignment' is based on parent
> >>> node’s #size-cells property.
> >>>
> >>> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> >>> the property obviously.
> >>>
> >>> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
> >>
> >> I wonder if changing this might break someone. It's been this way for 
> >> a long time. It might be better to change the spec or just read 
> >> 'alignment' as whatever size it happens to be (len / 4). It's not really 
> >> the kernel's job to validate the DT. We should first have some 
> >> validation in place to *know* if there are any current .dts files that 
> >> would break. That would probably be easier to implement in dtc than 
> >> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
> >> but that was the default for OpenFirmware.
> >>
> >> As the alignment is the base address alignment, it can be argued that 
> >> "#address-cells" makes more sense to use than "#size-cells". So maybe 
> >> the spec was a copy-n-paste error.
> > 
> > Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
> > tree has cases where #address-cells != #size-cells.
> > 
> 
> it seems upstream upstream Pixel 6 has no property 'alignment'
> git grep alignment arch/arm64/boot/dts/exynos/google/
> so it should not be broken.

That's right. I was responding to Rob's statement about #address-cells !=
#size-cells being pretty rare. And wanted to give credance to the idea that
this change could possible break someone.

> 
> > I would prefer to not have this change, but if that's not possible, could we
> > not backport it to all the stable branches? That way we can just force new
> > devices to fix this instead of existing devices on older LTS kernels?
> > 
> 
> the fix have stable and fix tags. not sure if we can control its
> backporting. the fix has been backported to 6.1/6.6/6.12/6.13 automatically.

Right, I think it's already backported to the LTS kernels, but if it breaks any
in-tree users then we'd have to revert it. I just like Rob's idea to instead
change the spec for obvious reasons :)

Regards,
Will

<snip>

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Rob Herring]

On Tue, Feb 25, 2025 at 09:46:54AM -0800, William McVicker wrote:
> On 02/25/2025, Zijun Hu wrote:
> > On 2/25/2025 9:18 AM, William McVicker wrote:
> > > Hi Zijun and Rob,
> > > 
> > > On 01/13/2025, Rob Herring wrote:
> > >> On Thu, Jan 09, 2025 at 09:27:00PM +0800, Zijun Hu wrote:
> > >>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> > >>>
> > >>> According to DT spec, size of property 'alignment' is based on parent
> > >>> node’s #size-cells property.
> > >>>
> > >>> But __reserved_mem_alloc_size() wrongly uses @dt_root_addr_cells to get
> > >>> the property obviously.
> > >>>
> > >>> Fix by using @dt_root_size_cells instead of @dt_root_addr_cells.
> > >>
> > >> I wonder if changing this might break someone. It's been this way for 
> > >> a long time. It might be better to change the spec or just read 
> > >> 'alignment' as whatever size it happens to be (len / 4). It's not really 
> > >> the kernel's job to validate the DT. We should first have some 
> > >> validation in place to *know* if there are any current .dts files that 
> > >> would break. That would probably be easier to implement in dtc than 
> > >> dtschema. Cases of #address-cells != #size-cells should be pretty rare, 
> > >> but that was the default for OpenFirmware.
> > >>
> > >> As the alignment is the base address alignment, it can be argued that 
> > >> "#address-cells" makes more sense to use than "#size-cells". So maybe 
> > >> the spec was a copy-n-paste error.
> > > 
> > > Yes, this breaks our Pixel downstream DT :( Also, the upstream Pixel 6 device
> > > tree has cases where #address-cells != #size-cells.
> > >

I thought downstream kept kernels and DTs in sync, so the dts could be 
fixed?
 
> > 
> > it seems upstream upstream Pixel 6 has no property 'alignment'
> > git grep alignment arch/arm64/boot/dts/exynos/google/
> > so it should not be broken.
> 
> That's right. I was responding to Rob's statement about #address-cells !=
> #size-cells being pretty rare. And wanted to give credance to the idea that
> this change could possible break someone.
> 
> > 
> > > I would prefer to not have this change, but if that's not possible, could we
> > > not backport it to all the stable branches? That way we can just force new
> > > devices to fix this instead of existing devices on older LTS kernels?
> > > 
> > 
> > the fix have stable and fix tags. not sure if we can control its
> > backporting. the fix has been backported to 6.1/6.6/6.12/6.13 automatically.
> 
> Right, I think it's already backported to the LTS kernels, but if it breaks any
> in-tree users then we'd have to revert it. I just like Rob's idea to instead
> change the spec for obvious reasons :)

While if it is downstream, it doesn't exist, I'm reverting this for now. 
We need the tools to check this and look at other projects to see what 
they expect. Then we can think about changing the spec.

Rob

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 03:45, Rob Herring wrote:
>> Right, I think it's already backported to the LTS kernels, but if it breaks any
>> in-tree users then we'd have to revert it. I just like Rob's idea to instead
>> change the spec for obvious reasons 🙂
> While if it is downstream, it doesn't exist, I'm reverting this for now. 

perhaps, it is better for us to slow down here.

1) This change does not break any upstream code.
   is there downstream code which is publicly visible and is broken by
   this change ?

2) IMO, the spec may be right.
   The type of size is enough to express any alignment wanted.
   For several kernel allocators. type of 'alignment' should be the type
   of 'size', NOT the type of 'address'


> We need the tools to check this and look at other projects to see what 
> they expect. Then we can think about changing the spec.

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Rob Herring]

On Wed, Feb 26, 2025 at 2:31 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2025/2/27 03:45, Rob Herring wrote:
> >> Right, I think it's already backported to the LTS kernels, but if it breaks any
> >> in-tree users then we'd have to revert it. I just like Rob's idea to instead
> >> change the spec for obvious reasons 🙂
> > While if it is downstream, it doesn't exist, I'm reverting this for now.
>
> perhaps, it is better for us to slow down here.
>
> 1) This change does not break any upstream code.
>    is there downstream code which is publicly visible and is broken by
>    this change ?

We don't know that unless you tested every dts file. We only know that
no one has reported an issue yet.

Even if we did test everything, there are DT's that aren't in the
kernel tree. It's not like this downstream DT is using some
undocumented binding or questionable things. It's a standard binding.

Every time this code is touched, it breaks. This is not even the only
breakage right now[1].

> 2) IMO, the spec may be right.
>    The type of size is enough to express any alignment wanted.
>    For several kernel allocators. type of 'alignment' should be the type
>    of 'size', NOT the type of 'address'

As I said previously, it can be argued either way.

Rob

[1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[William McVicker]

> I thought downstream kept kernels and DTs in sync, so the dts could be
> fixed?

For Pixel the kernel and DT are in sync, but I'm not sure about other devices.
The problem in general though is now everyone would need to do a special
coordination when updating to the newer LTS version to make sure their kernel
matches the new DT.

On 02/26/2025, Rob Herring wrote:
> On Wed, Feb 26, 2025 at 2:31 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >
> > On 2025/2/27 03:45, Rob Herring wrote:
> > >> Right, I think it's already backported to the LTS kernels, but if it breaks any
> > >> in-tree users then we'd have to revert it. I just like Rob's idea to instead
> > >> change the spec for obvious reasons 🙂
> > > While if it is downstream, it doesn't exist, I'm reverting this for now.
> >
> > perhaps, it is better for us to slow down here.
> >
> > 1) This change does not break any upstream code.
> >    is there downstream code which is publicly visible and is broken by
> >    this change ?
> 
> We don't know that unless you tested every dts file. We only know that
> no one has reported an issue yet.
> 
> Even if we did test everything, there are DT's that aren't in the
> kernel tree. It's not like this downstream DT is using some
> undocumented binding or questionable things. It's a standard binding.
> 
> Every time this code is touched, it breaks. This is not even the only
> breakage right now[1].

You can find the Pixel 6/7/8/9 device trees on android.googlesource.com.
You can see for zuma based devices (Pixel 9 for example) they have this [1]:

  &reserved_memory {
        #address-cells = <2>;
        #size-cells = <1>;
        vstream: vstream {
                compatible = "shared-dma-pool";
                reusable;
                size = <0x4800000>;
                alignment = <0x0 0x00010000>;
                alloc-ranges = <0x9 0x80000000 0x80000000>,
                               <0x9 0x00000000 0x80000000>,
                               <0x8 0x80000000 0x80000000>,
                               <0x0 0x80000000 0x80000000>;
        };

I understand this code is downstream, but as a general principle we shouldn't
break backwards compatibilty.

Thanks,
Will

[1] https://android.googlesource.com/kernel/devices/google/zuma/+/refs/heads/android-gs-shusky-6.1-android16-dp/dts/gs101-dma-heap.dtsi#147

> 
> > 2) IMO, the spec may be right.
> >    The type of size is enough to express any alignment wanted.
> >    For several kernel allocators. type of 'alignment' should be the type
> >    of 'size', NOT the type of 'address'
> 
> As I said previously, it can be argued either way.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 05:36, William McVicker wrote:
>> Every time this code is touched, it breaks. This is not even the only
>> breakage right now[1].
> You can find the Pixel 6/7/8/9 device trees on android.googlesource.com.
> You can see for zuma based devices (Pixel 9 for example) they have this [1]:
> 
>   &reserved_memory {
>         #address-cells = <2>;
>         #size-cells = <1>;
>         vstream: vstream {
>                 compatible = "shared-dma-pool";
>                 reusable;
>                 size = <0x4800000>;
>                 alignment = <0x0 0x00010000>;
>                 alloc-ranges = <0x9 0x80000000 0x80000000>,
>                                <0x9 0x00000000 0x80000000>,
>                                <0x8 0x80000000 0x80000000>,
>                                <0x0 0x80000000 0x80000000>;
>         };
> 
> I understand this code is downstream, but as a general principle we shouldn't
> break backwards compatibilty.

this is not backward compatibility issue. it is a downstream bug instead.

normally, you need to write DTS according to relevant DT binding spec or
DT spec.

i can't access the link you shared due to my country's GFW.
does google kernel have extra binding spec about size of property
'alignment'?

IMO, downstream maintainers may needs to fix this issue by if the
upstream fix is picked up.
- alignment = <0x0 0x00010000>;
+ alignment = <0x00010000>;

actually, "The importance of getting code into the mainline" within
Documentation/process/1.Intro.rst encourages upstream your code to
avoid such issue.

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 05:30, Rob Herring wrote:
>>    this change ?
> We don't know that unless you tested every dts file. We only know that
> no one has reported an issue yet.
> 

Sorry, my mistake to post the question here for convenience.

actually, i want to ask William this question, and he/she shared applet
of the downstream code.

> Even if we did test everything, there are DT's that aren't in the
> kernel tree. It's not like this downstream DT is using some
> undocumented binding or questionable things. It's a standard binding.
> 

IMO, that may be a downstream bug since they don't refer to binding spec
to set property 'alignment'.

> Every time this code is touched, it breaks. This is not even the only
> breakage right now[1].
> 

indeed.

>> 2) IMO, the spec may be right.
>>    The type of size is enough to express any alignment wanted.
>>    For several kernel allocators. type of 'alignment' should be the type
>>    of 'size', NOT the type of 'address'
> As I said previously, it can be argued either way.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

Re: [PATCH v4 09/14] of: reserved-memory: Fix using wrong number of cells to get property 'alignment'

[Zijun Hu]

On 2025/2/27 05:30, Rob Herring wrote:
> Every time this code is touched, it breaks. This is not even the only
> breakage right now[1].
> 
>> 2) IMO, the spec may be right.
>>    The type of size is enough to express any alignment wanted.
>>    For several kernel allocators. type of 'alignment' should be the type
>>    of 'size', NOT the type of 'address'
> As I said previously, it can be argued either way.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20250226115044.zw44p5dxlhy5eoni@pengutronix.de/

this issue may be a downstream issue as well.

based on comments. they expects the CMA area is within 4G range.

but the wrong alloc-ranges config make the range (0x42000000 +
0xc0000000) > 4G across 4G boundary.