linux-kernel - Re: [PATCH 1/3] media: i2c: dw9768: Fix pm_runtime_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZySMfQAJ-NkYmUoP@kekkonen.localdomain>
Date: Fri, 1 Nov 2024 08:08:29 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Jinjie Ruan <ruanjinjie@...wei.com>
Cc: mchehab@...nel.org, ming.qian@....com, eagle.zhou@....com,
	stanimir.k.varbanov@...il.com, quic_vgarodia@...cinc.com,
	bryan.odonoghue@...aro.org, hans.verkuil@...co.com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 1/3] media: i2c: dw9768: Fix pm_runtime_set_suspended()
 with runtime pm enabled

Hi Jinjie,

Thanks for the patch.

On Mon, Sep 23, 2024 at 11:51:13AM +0800, Jinjie Ruan wrote:
> It is not valid to call pm_runtime_set_suspended() and
> pm_runtime_set_active() for devices with runtime PM enabled because it
> returns -EAGAIN if it is enabled already and working. So, adjust the
> order to fix it.
> 
> Fixes: 5f9a089b6de3 ("dw9768: Enable low-power probe on ACPI")
> Signed-off-by: Jinjie Ruan <ruanjinjie@...wei.com>
> ---
>  drivers/media/i2c/dw9768.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
> index 18ef2b35c9aa..61db1e8d2727 100644
> --- a/drivers/media/i2c/dw9768.c
> +++ b/drivers/media/i2c/dw9768.c
> @@ -471,7 +471,6 @@ static int dw9768_probe(struct i2c_client *client)
>  	 * to be powered on in an ACPI system. Similarly for power off in
>  	 * remove.
>  	 */
> -	pm_runtime_enable(dev);
>  	full_power = (is_acpi_node(dev_fwnode(dev)) &&
>  		      acpi_dev_state_d0(dev)) ||
>  		     (is_of_node(dev_fwnode(dev)) && !pm_runtime_enabled(dev));

pm_runtime_enabled() should be replaced by IS_ENABLED(CONFIG_PM) here.

> @@ -484,6 +483,7 @@ static int dw9768_probe(struct i2c_client *client)
>  		pm_runtime_set_active(dev);
>  	}
>  
> +	pm_runtime_enable(dev);
>  	ret = v4l2_async_register_subdev(&dw9768->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> @@ -495,12 +495,12 @@ static int dw9768_probe(struct i2c_client *client)
>  	return 0;
>  
>  err_power_off:
> +	pm_runtime_disable(dev);
>  	if (full_power) {
>  		dw9768_runtime_suspend(dev);
>  		pm_runtime_set_suspended(dev);
>  	}
>  err_clean_entity:
> -	pm_runtime_disable(dev);
>  	media_entity_cleanup(&dw9768->sd.entity);
>  err_free_handler:
>  	v4l2_ctrl_handler_free(&dw9768->ctrls);
> @@ -517,12 +517,12 @@ static void dw9768_remove(struct i2c_client *client)
>  	v4l2_async_unregister_subdev(&dw9768->sd);
>  	v4l2_ctrl_handler_free(&dw9768->ctrls);
>  	media_entity_cleanup(&dw9768->sd.entity);
> +	pm_runtime_disable(dev);
>  	if ((is_acpi_node(dev_fwnode(dev)) && acpi_dev_state_d0(dev)) ||
>  	    (is_of_node(dev_fwnode(dev)) && !pm_runtime_enabled(dev))) {

And similarly here. The patch seems fine with that change.

>  		dw9768_runtime_suspend(dev);
>  		pm_runtime_set_suspended(dev);
>  	}
> -	pm_runtime_disable(dev);
>  }
>  
>  static const struct of_device_id dw9768_of_table[] = {

-- 
Kind regards,

Sakari Ailus