On Thu, 8 Aug 2024 at 08:53, Arnd Bergmann arnd@arndb.de 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