On Thu, Aug 8, 2024, at 08:12, Marco Felsch wrote:
>
> On 24-08-08, Ma Ke wrote:
>> Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using
>> IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_power_dev
>> is either error or NULL.
>>
>> In case a power domain attached by dev_pm_domain_attach_by_name() is not
>> described in DT, dev_pm_domain_attach_by_name() returns NULL, which is
>> then used, which leads to NULL pointer dereference.
>
> Argh.. there are other users of this API getting this wrong too. This
> make me wonder why dev_pm_domain_attach_by_name() return NULL instead of
> the error code returned by of_property_match_string().
>
> IMHO to fix once and for all users we should fix the return code of
> dev_pm_domain_attach_by_name().
Agreed, in general any use of IS_ERR_OR_NULL() indicates that there
is a bad API that should be fixed instead, and this is probably the
case for genpd_dev_pm_attach_by_id().
One common use that is widely accepted is returning NULL when
a subsystem is completely disabled. In this case an IS_ERR()
check returns false on a NULL pointer and the returned structure
should be opaque so callers are unable to dereference that
NULL pointer.
genpd_dev_pm_attach_by_{id,name}() is documented to also return
a NULL pointer when no PM domain is needed, but they return
a normal 'struct device' that can easily be used in an unsafe
way after checking for IS_ERR().
Fortunately it seems that there are only a few callers at the
moment, so coming up with a safer interface is still possible.
Arnd
