linux-kernel - Re: [PATCH 4/6] media: imx208: Support device probe in non-zero ACPI D state

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] [day] [month] [year] [list]

Message-ID: <YbmV4hHrixPAeQYD@paasikivi.fi.intel.com>
Date:   Wed, 15 Dec 2021 09:14:42 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Bingbu Cao <bingbu.cao@...ux.intel.com>
Cc:     Bingbu Cao <bingbu.cao@...el.com>, linux-media@...r.kernel.org,
        rafael@...nel.org, shawnx.tu@...el.com, tian.shu.qiu@...el.com,
        chiranjeevi.rapolu@...el.com, hyungwoo.yang@...el.com,
        tfiga@...omium.org, senozhatsky@...omium.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] media: imx208: Support device probe in non-zero ACPI
 D state

Hi Bingbu,

On Wed, Dec 15, 2021 at 11:13:03AM +0800, Bingbu Cao wrote:
> 
> 
> On 12/14/21 11:59 PM, Sakari Ailus wrote:
> > Hi Bingbu,
> > 
> > On Tue, Nov 09, 2021 at 04:48:33PM +0800, Bingbu Cao wrote:
> >> Tell ACPI device PM code that the driver supports the device being in
> >> non-zero ACPI D state when the driver's probe function is entered.
> >>
> >> Also do identification on the first access of the device, whether in probe
> >> or when starting streaming.
> >>
> >> Signed-off-by: Bingbu Cao <bingbu.cao@...el.com>
> >> ---
> >>  drivers/media/i2c/imx208.c | 77 +++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 48 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c
> >> index 6f3d9c1b5879..b9f6d5f33b58 100644
> >> --- a/drivers/media/i2c/imx208.c
> >> +++ b/drivers/media/i2c/imx208.c
> >> @@ -296,6 +296,9 @@ struct imx208 {
> >>  	/* OTP data */
> >>  	bool otp_read;
> >>  	char otp_data[IMX208_OTP_SIZE];
> >> +
> >> +	/* True if the device has been identified */
> >> +	bool identified;
> >>  };
> >>  
> >>  static inline struct imx208 *to_imx208(struct v4l2_subdev *_sd)
> >> @@ -619,6 +622,34 @@ static int imx208_set_pad_format(struct v4l2_subdev *sd,
> >>  	return 0;
> >>  }
> >>  
> >> +static int imx208_identify_module(struct imx208 *imx208)
> >> +{
> >> +	struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
> >> +	int ret;
> >> +	u32 val;
> >> +
> >> +	if (imx208->identified)
> >> +		return 0;
> > 
> > Otp access requires this as well.
> 
> Sakari, thanks for your review.
> 
> Yes, OTP read will trigger the LED blink, but I am not sure it makes sense that camera
> framework try to read the OTP data without streaming, and it would complain once fail to
> access the OTP data.

That might be the case, but the interface this driver provides is
accessible in the user space before streaming is enabled. Accessing
sensor's other registers shouldn't be done before the sensor is identified.

> > 
> > How about adding a runtime PM resume callback for this?
> 
> For the runtime PM callback, what is the benefit adding a callback here as will call try
> pm_runtime_get_sync() each stream on?

My suggestion is to identify the sensor from the runtime PM resume callback
(which this driver doesn't have yet), i.e. not here.

-- 
Kind regards,

Sakari Ailus