linux-kernel - Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

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: <20240807094957.GE21319@pendragon.ideasonboard.com>
Date: Wed, 7 Aug 2024 12:49:57 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Benjamin Bara <bbara93@...il.com>
Cc: Alexander Stein <alexander.stein@...tq-group.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Hans de Goede <hdegoede@...hat.com>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Benjamin Bara <benjamin.bara@...data.com>
Subject: Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

Hi Benjamin,

On Wed, Aug 07, 2024 at 10:47:39AM +0200, Benjamin Bara wrote:
> On Wed, 7 Aug 2024 at 10:33, Alexander Stein wrote:
> > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > Currently, the V4L2 subdevice is also created when the device is not
> > > available/connected. In this case, dmesg shows the following:
> > >
> > > [   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
> > > [   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
> > > [   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
> > > [   10.454018] imx290 7-001a: Error writing reg 0x3020: -6
> > >
> > > which seems to come from imx290_ctrl_update() after the subdev init is
> > > finished. However, as the errors are ignored, the subdev is initialized
> > > but simply does not work. From userspace perspective, there is no
> > > visible difference between a working and not-working subdevice (except
> > > when trying it out or watching for the error message).
> > >
> > > This commit adds a simple availability check before starting with the
> > > subdev initialization to error out instead.
> >
> > There is already a patch reading the ID register at [1]. This also reads the
> > ID register. But I don't have any documentation regarding that register,
> > neither address nor values definitions. If there is known information about
> > that I would prefer reading the ID and compare it to expected values.
> 
> Thanks for the link - it seems like Laurent has dropped the patch for
> the more recent kernel versions on their GitLab.

It was a patch that I wrote as a test, and I decided not to upstream it
as it had limited value to me. The downside with reading registers at
probe time is that you have to power up the sensor. This can have
undesired side effects, such as flashing a privacy LED on at boot time
in devices that have one. There's also the increase in boot time due to
the power up sequence, which one may want to avoid.

The imx290 driver already powers up the device unconditionally at probe
time, so reading the version register wouldn't be much of an issue I
suppose. I would be fine merging that patch.

> This was also my initial intention, but similar to you, I don't have a
> docu describing this register, so I am not sure where the info is coming
> from and if it really contains the identification/type info. Probably
> Laurent has more infos on that.

That's a good question. I don't see a mention of that register in the
IMX290 datasheet I've found online
(https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf).
Looking at the git history, the IMX290_CHIP_ID register macro was
introduced in an unrelated commit, without an explanation. I don't
recall where it comes from, but I don't think I've added it randomly. It
may have come from an out-of-tree driver.

I don't have an IMX290 plugged in at the moment, what's the value of the
register ?

> > [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47
> >
> > > Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>
> > > ---
> > >  drivers/media/i2c/imx290.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 4150e6e4b9a6..a86076e42a36 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -1580,6 +1580,11 @@ static int imx290_probe(struct i2c_client *client)
> > >       pm_runtime_set_autosuspend_delay(dev, 1000);
> > >       pm_runtime_use_autosuspend(dev);
> > >
> > > +     /* Make sure the sensor is available before V4L2 subdev init. */
> > > +     ret = cci_read(imx290->regmap, IMX290_STANDBY, NULL, NULL);
> > > +     if (ret)
> > > +             goto err_pm;
> > > +
> > >       /* Initialize the V4L2 subdev. */
> > >       ret = imx290_subdev_init(imx290);
> > >       if (ret)

-- 
Regards,

Laurent Pinchart