Re: [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT

[Marek Vasut <marex@denx.de>]

On 1/19/24 08:42, Uwe Kleine-König wrote:
> On Fri, Jan 19, 2024 at 02:47:41AM +0100, Marek Vasut wrote:
>> This driver assumes that domain->power_dev is non-NULL in its suspend/resume
>> path. The assumption is valid, since all the devices that are being looked up
>> here should be described in DT. In case they are not described in DT, beause
>> the DT is faulty, suspend/resume attempt would trigger NULL pointer dereference.
>> To avoid this failure, check whether the power_dev assignment is not NULL right
>> away in probe callback and fail early if it is.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Jindong Yue <jindong.yue@nxp.com>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Marco Felsch <m.felsch@pengutronix.de>
>> Cc: NXP Linux Team <linux-imx@nxp.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-pm@vger.kernel.org
>> ---
>> V2: Add extra check for domain being NULL (thanks Peng)
>> ---
>>   drivers/pmdomain/imx/imx8m-blk-ctrl.c  | 9 ++++++---
>>   drivers/pmdomain/imx/imx8mp-blk-ctrl.c | 9 ++++++---
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
>> index 1341a707f61bc..ca942d7929c2b 100644
>> --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
>> +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
>> @@ -258,11 +258,14 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
>>   
>>   		domain->power_dev =
>>   			dev_pm_domain_attach_by_name(dev, data->gpc_name);
>> -		if (IS_ERR(domain->power_dev)) {
>> -			dev_err_probe(dev, PTR_ERR(domain->power_dev),
>> +		if (IS_ERR_OR_NULL(domain->power_dev)) {
>> +			if (!domain->power_dev)
>> +				ret = -ENODEV;
>> +			else
>> +				ret = PTR_ERR(domain->power_dev);
>> +			dev_err_probe(dev, ret,
>>   				      "failed to attach power domain \"%s\"\n",
>>   				      data->gpc_name);
>> -			ret = PTR_ERR(domain->power_dev);
>>   			goto cleanup_pds;
>>   		}
>>   
>> diff --git a/drivers/pmdomain/imx/imx8mp-blk-ctrl.c b/drivers/pmdomain/imx/imx8mp-blk-ctrl.c
>> index e3203eb6a0229..e488cf79b8007 100644
>> --- a/drivers/pmdomain/imx/imx8mp-blk-ctrl.c
>> +++ b/drivers/pmdomain/imx/imx8mp-blk-ctrl.c
>> @@ -687,11 +687,14 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev)
>>   
>>   		domain->power_dev =
>>   			dev_pm_domain_attach_by_name(dev, data->gpc_name);
>> -		if (IS_ERR(domain->power_dev)) {
>> -			dev_err_probe(dev, PTR_ERR(domain->power_dev),
>> +		if (IS_ERR_OR_NULL(domain->power_dev)) {
>> +			if (!domain->power_dev)
>> +				ret = -ENODEV;
>> +			else
>> +				ret = PTR_ERR(domain->power_dev);
>> +			dev_err_probe(dev, ret,
>>   				      "failed to attach power domain %s\n",
>>   				      data->gpc_name);
>> -			ret = PTR_ERR(domain->power_dev);
> 
> This could be made a bit more compact using:
> 
> 	domain->power_dev =
> 		dev_pm_domain_attach_by_name(dev, data->gpc_name) ||
> 		ERR_PTR(-ENODEV);
> 	if (IS_ERR(domain->power_dev)) {
> 		ret = PTR_ERR(domain->power_dev);
> 		...
> 
> I'm unsure though if this is human friendly enough?!

I think it is only more cryptic and doesn't improve readability.

> Having said that I wonder about dev_pm_domain_attach_by_name(). IMHO if
> NULL is an error case it and other errors are signaled by error
> pointers, there is something to fix there.

I don't think dev_pm_domain_attach_by_name() returning NULL is an error 
-- the domain may be missing from DT and that is legitimate use case I 
think.

But not here, where all the domains should be described in DT because 
the driver makes assumptions about their presence in the suspend/resume 
part, and because the DT should fully describe the domains of this 
hardware anyway, so we better catch such DT issues.