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

[Uwe Kleine-König]

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 at denx.de>
> ---
> Cc: "Uwe Kleine-König" <u.kleine-koenig at pengutronix.de>
> Cc: Fabio Estevam <festevam at gmail.com>
> Cc: Jindong Yue <jindong.yue at nxp.com>
> Cc: Lucas Stach <l.stach at pengutronix.de>
> Cc: Marco Felsch <m.felsch at pengutronix.de>
> Cc: NXP Linux Team <linux-imx at nxp.com>
> Cc: Peng Fan <peng.fan at nxp.com>
> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
> Cc: Sascha Hauer <s.hauer at pengutronix.de>
> Cc: Shawn Guo <shawnguo at kernel.org>
> Cc: Ulf Hansson <ulf.hansson at linaro.org>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-pm at 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?!

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20240119/32dbc325/attachment.sig>