Re: [PATCH] of: Add Google Juniper to excluded default cells list

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 25/11/24 12:38, Krzysztof Kozlowski ha scritto:
> On 25/11/2024 12:31, Krzysztof Kozlowski wrote:
>> Google Juniper platforms have a very old bootloader which populates
>> /firmware node without proper address/size-cells leading to warnings:
>>
>>    Missing '#address-cells' in /firmware
>>    WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x90/0xf0
>>    Modules linked in:
>>    CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>>    Hardware name: Google juniper sku16 board (DT)
>>    ...
>>    Missing '#size-cells' in /firmware
>>    WARNING: CPU: 0 PID: 1 at drivers/of/base.c:133 of_bus_n_size_cells+0x90/0xf0
>>    Modules linked in:
>>    CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>>    Tainted: [W]=WARN
>>    Hardware name: Google juniper sku16 board (DT)
>>
>> The platform won't receive updated bootloader/firmware so add it to
>> excluded platform list to silence the warning.
>>
>> Reported-by: Sasha Levin <sashal@kernel.org>
>> Closes: https://lore.kernel.org/all/Z0NUdoG17EwuCigT@sashalap/
>> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> Cc: Matthias Brugger <matthias.bgg@gmail.com>
>> Cc: Chen-Yu Tsai <wenst@chromium.org>
>> Cc: Conor Dooley <conor@kernel.org>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>   drivers/of/base.c | 26 ++++++++++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index a8b0c42bdc8e..13f0b2877ee0 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -56,6 +56,16 @@ DEFINE_MUTEX(of_mutex);
>>    */
>>   DEFINE_RAW_SPINLOCK(devtree_lock);
>>   
>> +/*
>> + * List of machines running old firmware without explicit #address-cells and
>> + * #size-cells values for parent nodes, which are most likely not going get any
>> + * update.
>> + */
>> +static const char * const excluded_default_cells_compats[] = {
>> +	"google,juniper",
>> +	NULL
>> +};
>> +
>>   bool of_node_name_eq(const struct device_node *np, const char *name)
>>   {
>>   	const char *node_name;
>> @@ -91,6 +101,17 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)
>>   	IS_ENABLED(CONFIG_SPARC) \
>>   )
>>   
>> +static bool excluded_default_cells_machines(void)
>> +{
>> +	/* Do not repeat the machine checks for every bus */
>> +	static int excluded_machine = -1;
>> +
>> +	if (excluded_machine < 0)
>> +		excluded_machine = of_machine_compatible_match(excluded_default_cells_compats);
>> +
>> +	return !!excluded_machine;
>> +}
>> +
>>   int of_bus_n_addr_cells(struct device_node *np)
>>   {
>>   	u32 cells;
>> @@ -103,7 +124,7 @@ int of_bus_n_addr_cells(struct device_node *np)
>>   		 * is deprecated. Any platforms which hit this warning should
>>   		 * be added to the excluded list.
>>   		 */
>> -		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
>> +		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS && !excluded_default_cells_machines(),
>>   			  "Missing '#address-cells' in %pOF\n", np);
>>   	}
>>   	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
>> @@ -125,12 +146,13 @@ int of_bus_n_size_cells(struct device_node *np)
>>   	for (; np; np = np->parent) {
>>   		if (!of_property_read_u32(np, "#size-cells", &cells))
>>   			return cells;
>> +
> 
> 
> This was not intentional, I'll fix it in v2.
> 
> Obviously this code is not really SMP aware, but even with store tearing
> I don't think it will be issue. Worst case the
> of_machine_compatible_match() will be called more than one, which is not
> fatal and might not justify atomics or locks.
> 
> Best regards,
> Krzysztof


For v2, feel free to add my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>