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

Hi Manivannan,

On Thu, Aug 29, 2024 at 10:02:47PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 29, 2024 at 04:19:09PM +0300, Laurent Pinchart wrote:
> 
> Hi Laurent,
> 
> [...]
> 
> > > +		dev_err(dev, "Sensor is not in standby mode\n");
> > > +		ret = -ENODEV;
> > > +		goto err_pm;
> > > +	}
> > > +
> > 
> > My last concern is about accessing hardware at probe time. There are
> > known cases where this is problematic. They can be split in two
> > categories, systems that exhibit unwanted side effects when powering the
> > sensor up, and systems where the sensor can't be accessed at probe time.
> > 
> > The two issues I can think of in the first category is devices that have
> > a camera privacy light that could cause worries among users if it
> > flashes at boot time, and devices that agressively optimize boot time.
> > 
> > In the second category, I know that some people use camera serdes
> > (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> > instead use kernel drivers for those components, upstream may not care
> > too much about this use case. Another issue I was told about was a
> > device booting in temperatures that were too low for the camera to
> > operate, which then needed half an hour to heat the device enclosure
> > before the sensor and serdes could be accessed. That's a bit extreme,
> > but it sounds like a valid use case to me.
> > 
> > What do we do with those cases ? Detecting devices at probe time does
> > have value, so I think it should be a policy decision. We may want to
> > convey some of that information through DT properties (I'm not sure what
> > would be acceptable there though). In any case, that's quite a bit of
> > yak shaving, so I'm inclined to accept this series (or rather its next
> > version), given that quite a few other camera sensor drivers detect the
> > device at probe time. I would however like feedback on the problem to
> > try and find a good solution.
> 
> Most of the issues you mentioned applies to other hardware peripherals also IMO.
> And it is common for the drivers to read registers and make sure the device is
> detected on the bus during probe().

That's true. I think the problem affects different device types
differently though, and this may (or may not) call for different
solutions.

> If an usecase doesn't want to read the
> registers during probe time, then they _should_not_ build the driver as built-in
> rather make it as a loadable module and load it whenever necessary. This applies
> to boot time optimization as well.

For most of the use cases I listed I agree with you. One exception is
the privacy light issue. Regardless of when the camera sensor driver is
loaded, powering the device at probe time will flash the privacy light.
Doing so later than boot time would probably make the issue even worse,
I would worry more if I saw my webcam privacy light flashing at a random
point after boot time.

> A DT property wouldn't be feasible as DT is supposed to describe the hardware,
> not the usecase.

I think that rule is typically slightly relaxed, by allowing in DT
system descriptions, not just hardware descriptions. Otherwise we
wouldn't allow things like reserved memory ranges. Describing that a
camera sensor has a privacy light, in a way that would allow drivers to
avoid powering up the device at probe time without requiring much
duplicated code in all drivers, would in my opinion be an acceptable DT
usage.

-- 
Regards,

Laurent Pinchart