Re: [PATCH 1/3] soc: apple: Add hardware tunable support

[Sven Peter]

Hi,

On 29.10.25 20:21, Janne Grunau wrote:
> Hej,
>
> On Sun, Oct 26, 2025 at 01:52:01PM +0000, Sven Peter wrote:
> > Various hardware, like the Type-C PHY or the Thunderbolt/USB4 NHI,
> > present on Apple SoCs need machine-specific tunables passed from our
> > bootloader m1n1 to the device tree. Add generic helpers so that we
> > don't have to duplicate this across multiple drivers.
> >
> > Reviewed-by: Alyssa Rosenzweig <alyssa@xxxxxxxxxxxxx>
> > Reviewed-by: Neal Gompa <neal@xxxxxxxxx>
> > Signed-off-by: Sven Peter <sven@xxxxxxxxxx>

[...]

> > +
> > +	tunable = devm_kzalloc(dev,
> > +			       sizeof(*tunable) + sz * sizeof(*tunable->values),
>
> There is a struct_size macro in linux/overflow.h for this calculation.
> We do not have to care about overflows as as struct property.length
> remains (signed) int. I would expect there is a much smaller limit for of
> properties in place anyway. The macro looks nicer though:
>
> struct_size(tunable, values, sz)

Nice, I'll use that!

> > +			       GFP_KERNEL);
> > +	if (!tunable)
> > +		return ERR_PTR(-ENOMEM);
> > +	tunable->sz = sz;
> > +
> > +	for (i = 0, p = NULL; i < tunable->sz; ++i) {
> > +		p = of_prop_next_u32(prop, p, &tunable->values[i].offset);
>
> Does it make sense to add an size argument either here or in
> apple_tunable_apply() to check that the offset is within the expect MMIO
> region? Not really important but might catch a bug someday.

I would've usually said this was overkill but given that we just found a 
bug in our bootloader which caused us to copy random memory as tunables 
a week or two ago because of a stale fdt node id I'll add some sanity 
checks here.

> > +		p = of_prop_next_u32(prop, p, &tunable->values[i].mask);
> > +		p = of_prop_next_u32(prop, p, &tunable->values[i].value);

[...]

> > +/**
> > + * Apply a previously loaded hardware tunable.
> > + *
> > + * @param regs: MMIO to which the tunable will be applied.
> > + * @param tunable: Pointer to the tunable.
> > + */
> > +void apple_tunable_apply(void __iomem *regs, struct apple_tunable *tunable);
> > +
> > +#endif
>
> Reviewed-by: Janne Grunau <j@xxxxxxxxxx>

thanks!


Sven