On Thu, 8 Aug 2024 at 08:53, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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.
I am not sure it's worth the effort, but I may be wrong.
It's been a bit tricky to keep the interfaces above consistent with
the legacy interface (dev_pm_domain_attach()). Moreover, we need a way
to allow a PM domain to be optional. By returning NULL (or 0), we are
telling the consumer that there is no PM domain described that we can
attach the device to.
Kind regards
Uffe
