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

From: Sven Peter <sven@kernel.org>
To: Janne Grunau <j@jannau.net>
Cc: Neal Gompa <neal@gompa.dev>, Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	asahi@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Alyssa Rosenzweig <alyssa@rosenzweig.io>
Subject: Re: [PATCH 1/3] soc: apple: Add hardware tunable support
Date: Sun, 2 Nov 2025 14:09:42 +0100	[thread overview]
Message-ID: <c8a88007-aaa0-4f8c-bf47-186b41428502@kernel.org> (raw)
In-Reply-To: <20251029192101.GA458701@robin.jannau.net>

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@rosenzweig.io>
>> Reviewed-by: Neal Gompa <neal@gompa.dev>
>> Signed-off-by: Sven Peter <sven@kernel.org>

[...]

>> +
>> +	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@jannau.net>

thanks!

Sven

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy