* RE: [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT
2024-01-19 1:47 [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT Marek Vasut
@ 2024-01-19 1:54 ` Peng Fan
2024-01-19 7:42 ` Uwe Kleine-König
2024-01-22 15:21 ` Ulf Hansson
2 siblings, 0 replies; 5+ messages in thread
From: Peng Fan @ 2024-01-19 1:54 UTC (permalink / raw)
To: Marek Vasut, linux-pm@vger.kernel.org
Cc: Uwe Kleine-König, Fabio Estevam, Jindong Yue, Lucas Stach,
Marco Felsch, dl-linux-imx, Pengutronix Kernel Team, Sascha Hauer,
Shawn Guo, Ulf Hansson, linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if
> domains are missing in DT
>
> 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>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
> 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);
> goto cleanup_pds;
> }
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT
2024-01-19 1:47 [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT Marek Vasut
2024-01-19 1:54 ` Peng Fan
@ 2024-01-19 7:42 ` Uwe Kleine-König
2024-01-19 10:58 ` Marek Vasut
2024-01-22 15:21 ` Ulf Hansson
2 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2024-01-19 7:42 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pm, Peng Fan, Shawn Guo, Sascha Hauer, Jindong Yue,
Marco Felsch, NXP Linux Team, Pengutronix Kernel Team,
Ulf Hansson, Fabio Estevam, linux-arm-kernel, Lucas Stach
[-- Attachment #1: Type: text/plain, Size: 3710 bytes --]
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?!
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/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT
2024-01-19 7:42 ` Uwe Kleine-König
@ 2024-01-19 10:58 ` Marek Vasut
0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2024-01-19 10:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pm, Peng Fan, Shawn Guo, Sascha Hauer, Jindong Yue,
Marco Felsch, NXP Linux Team, Pengutronix Kernel Team,
Ulf Hansson, Fabio Estevam, linux-arm-kernel, Lucas Stach
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT
2024-01-19 1:47 [PATCH v2] pmdomain: imx8m-blk-ctrl: imx8mp-blk-ctrl: Error out if domains are missing in DT Marek Vasut
2024-01-19 1:54 ` Peng Fan
2024-01-19 7:42 ` Uwe Kleine-König
@ 2024-01-22 15:21 ` Ulf Hansson
2 siblings, 0 replies; 5+ messages in thread
From: Ulf Hansson @ 2024-01-22 15:21 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pm, Uwe Kleine-König, Fabio Estevam, Jindong Yue,
Lucas Stach, Marco Felsch, NXP Linux Team, Peng Fan,
Pengutronix Kernel Team, Sascha Hauer, Shawn Guo,
linux-arm-kernel
On Fri, 19 Jan 2024 at 02:48, Marek Vasut <marex@denx.de> 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>
Applied for next, thanks!
Kind regards
Uffe
> ---
> 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);
> goto cleanup_pds;
> }
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread