Re: [PATCH] pmdomain: rockchip: fix domain check in rockchip_pm_add_one_domain()

[Sergey Shtylyov <s.shtylyov@auroraos.dev>]

On 9/24/26 11:48 AM, Ulf Hansson wrote:

[...]

>>>> In rockchip_pm_add_one_domain(), there's the check (most probably for
>>>> the case where the "reg" prop has an unexpected value?) which doesn't
>>>> make much sense as the variable pd_info will be NULL iff pmu->info->
>>>> domain_info is NULL and the variable id is 0.  What does make sense
>>>> there is checking pd_info->pwr_mask and pd_info->req_mask which (as
>>>> seems to follow from the code) can't both be 0 for a valid domain...
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with the Svace
>>>> static analysis tool.
>>>>
>>>> Fixes: 7c696693a4f5 ("soc: rockchip: power-domain: Add power domain driver")
>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
>>>
>>> Applied for next, but without the fixes tag, thanks!

   One detail I've just remembered about: if the patch won't be backported to
the stable kernels, it would be easier to just check rockchip_domain_info::name
for NULL instead of the {pwr,req}_mask fields checked by the patch; this field
seems to always be set for all domains the driver cares about, it just was added
in 5.14-rc1 (I think), see this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0a69452e03564c5eaf99f729de398cd94ee90851

>>> Note, that I am dropping the fixes tag because if there was a real
>>> problem, we would require yet another fix on top to get the

   The problem can (probably) happen iff a device tree has a domain # in
the "reg" property that the driver doesn't care about -- in this case the
.{pwr,req}_mask fields (and .name as well) will both be 0 (since IIRC a
C compiler should set not explicitly initialized entried to 0). My patch
was intended for checking for the domain #s that the driver doesn't care
about (and so doesn't init in the *_pm_domains[] definitions, not for the
validity of the domains it does care about; try looking at at e.g.
include/dt-bindings/power/px30-power.h and you'll see that the "valid"
domains start at 5 (and the domain #s are not even contiguous as e.g.
PX30_PD_{CRYPTO,DDR} indices remain uninitialized)...

>>> corresponding domain declaration to be correct. Yet, the patch is
>>> useful as is!  >>>>    I'm not sure I understand you. The patch should hopefully be
>> enough to detect the undefined entries (implicitly init'ed with
>> all 0s). What fix for the domain definitions do you have in mind,
>> turning the arrays into lookup tables by adding a domain ID as a
>> field to *struct* rockchip_domain_info?
> 
> Apologize if I was vague, but you kind of point out what I just said.
> 
> The patch helps us to *detect* incorrect entries. If it turns out we
> find one, we need to fix that entry.

   No, the intent was not to detect the improperly initialized entries but
to verify the supplied DT is correct; I suppose this was the intention of
the original (bogus) check in rockchip_pm_add_one_domain()...

> Kind regards
> Uffe

MBR, Sergey