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

[Rob Herring <robh@kernel.org>]

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