| 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
| ||
|
Message-ID: <20240902200416.GT1995@pendragon.ideasonboard.com>
Date: Mon, 2 Sep 2024 23:04:16 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Benjamin Bara <bbara93@...il.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.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>,
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 v3 7/7] media: i2c: imx290: Implement a "privacy mode"
for probe()
On Mon, Sep 02, 2024 at 09:49:33PM +0200, Benjamin Bara wrote:
> On Mon, 2 Sept 2024 at 20:10, Dave Stevenson wrote:
> > On Mon, 2 Sept 2024 at 16:59, <bbara93@...il.com> wrote:
> > >
> > > From: Benjamin Bara <benjamin.bara@...data.com>
> > >
> > > Currently, we have a trade-off between potentially enabling the privacy
> > > LED and reading out the connection state of the sensor during probe().
> > >
> > > To have a somewhat defined policy for now, make a decision based on the
> > > power supplies of the sensor. If they are enabled anyways, communicate
> > > with the powered sensor for an availability check. Otherwise, create the
> > > subdevice without knowing whether the sensor is connected or not.
> >
> > Almost all the camera modules used on Raspberry Pi have regulators
> > controlled via a GPIO, but no privacy LED. The preference from us is
> > very definitely to query the sensor during probe where possible to
> > flag up any connectivity issues, and indeed I've had a number of
> > support threads with imx290 where it's just not been connected but it
> > probed fully and showed up in libcamera.
> >
> > How can I opt in to patch 6 checking basic I2C to the sensor during
> > probe when I have a controllable regulator? (This is where the
> > discussions over a dtbinding for privacy LEDs and not powering up
> > sensors during probe comes in).
>
> When you want to probe only during boot time, you can use the
> "regulator-boot-on" DT binding on your controllable regulator. This
> enables the regulator while it is probed and disables it later if not
> used (in comparison to "always-on"). Should also work for modules.
This seems like a hack, I'm sorry :-( I think we should instead have a
DT property standardized for camera sensors that tell whether or not
there is a privacy LED, and skip the detection in that case.
> Unfortunately, I don't have a clean solution (which also autosuspends)
> for "any probe time". I think it is not possible to enable a regulator
> from user space without having a consuming DT node. A somewhat clean
> workaround might be CONFIG_REGULATOR_USERSPACE_CONSUMER, which gives you
> the possibility to change the state of a regulator via sysfs (after
> creating a DT node). This gives you the possibility to enable it any
> time. However, the userspace-consumer driver gets the regulators
> exclusive, which means you cannot add the sensor driver as consumer and
> therefore cannot use the autosuspend feature of the imx290. Not really
> "nice", but probably "feasible" if you have special constraints when you
> are allowed to probe (e.g. the temperature as mentioned by Laurent). A
> DT binding would be easier for this case.
>
> > > Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>
> > > ---
> > > Changes since v2:
> > > - new
> > > ---
> > > drivers/media/i2c/imx290.c | 82 ++++++++++++++++++++++++++++++++--------------
> > > 1 file changed, 57 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 6b292bbb0856..338b2c5ea547 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -1354,6 +1354,17 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
> > > * Power management
> > > */
> > >
> > > +static bool is_imx290_power_on(struct imx290 *imx)
> > > +{
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(imx->supplies); i++)
> > > + if (!regulator_is_enabled(imx->supplies[i].consumer))
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > > +
> > > static int imx290_power_on(struct imx290 *imx290)
> > > {
> > > int ret;
> > > @@ -1571,6 +1582,7 @@ static int imx290_probe(struct i2c_client *client)
> > > {
> > > struct device *dev = &client->dev;
> > > struct imx290 *imx290;
> > > + bool power_on;
> > > u64 val;
> > > int ret;
> > >
> > > @@ -1611,36 +1623,54 @@ static int imx290_probe(struct i2c_client *client)
> > > return ret;
> > >
> > > /*
> > > - * Enable power management. The driver supports runtime PM, but needs to
> > > - * work when runtime PM is disabled in the kernel. To that end, power
> > > - * the sensor on manually here.
> > > + * Privacy mode: if the regulators are not enabled, avoid enabling them.
> > > + * In case the regulators are enabled, we still want to make sure that
> > > + * the regulators know that they have another consumer, therefore run
> > > + * the powering sequence.
> > > */
> > > - ret = imx290_power_on(imx290);
> > > - if (ret < 0) {
> > > - dev_err(dev, "Could not power on the device\n");
> > > - return ret;
> > > + power_on = is_imx290_power_on(imx290);
> > > + dev_dbg(dev, "%s: power on: %d\n", __func__, power_on);
> > > + if (power_on) {
> > > + /*
> > > + * Enable power management. The driver supports runtime PM, but
> > > + * needs to work when runtime PM is disabled in the kernel. To
> > > + * that end, power the sensor on manually here.
> > > + */
> > > + ret = imx290_power_on(imx290);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Could not power on the device\n");
> > > + return ret;
> > > + }
> > > +
> > > + /*
> > > + * Enable runtime PM with autosuspend. As the device has been
> > > + * powered manually, mark it as active, and increase the usage
> > > + * count without resuming the device.
> > > + */
> > > + pm_runtime_set_active(dev);
> > > + pm_runtime_get_noresume(dev);
> > > }
> > >
> > > - /*
> > > - * Enable runtime PM with autosuspend. As the device has been powered
> > > - * manually, mark it as active, and increase the usage count without
> > > - * resuming the device.
> > > - */
> > > - pm_runtime_set_active(dev);
> > > - pm_runtime_get_noresume(dev);
> > > pm_runtime_enable(dev);
> > > 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, &val, NULL);
> > > - if (ret) {
> > > - ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> > > - goto err_pm;
> > > - }
> > > - if (val != IMX290_STANDBY_STANDBY) {
> > > - ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> > > - goto err_pm;
> > > + /*
> > > + * Make sure the sensor is available before V4L2 subdev init.
> > > + * This only works when the sensor is powered.
> > > + */
> > > + if (power_on) {
> > > + ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > > + if (ret) {
> > > + ret = dev_err_probe(dev, -ENODEV,
> > > + "Failed to detect sensor\n");
> > > + goto err_pm;
> > > + }
> > > + if (val != IMX290_STANDBY_STANDBY) {
> > > + ret = dev_err_probe(dev, -ENODEV,
> > > + "Sensor is not in standby\n");
> > > + goto err_pm;
> > > + }
> > > }
> > >
> > > /* Initialize the V4L2 subdev. */
> > > @@ -1666,8 +1696,10 @@ static int imx290_probe(struct i2c_client *client)
> > > * Decrease the PM usage count. The device will get suspended after the
> > > * autosuspend delay, turning the power off.
> > > */
> > > - pm_runtime_mark_last_busy(dev);
> > > - pm_runtime_put_autosuspend(dev);
> > > + if (power_on) {
> > > + pm_runtime_mark_last_busy(dev);
> > > + pm_runtime_put_autosuspend(dev);
> > > + }
> > >
> > > return 0;
> > >
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists