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