Hi all,
These patches enable calling (and finishing) a driver's probe function
without powering on the respective device on busses where the practice is
to power on the device for probe. While it generally is a driver's job to
check the that the device is there, there are cases where it might be
undesirable. (In this case it stems from a combination of hardware design
and user expectations; see below.) The downside with this change is that
if there is something wrong with the device, it will only be found at the
time the device is used. In this case (the camera sensors + EEPROM in a
sensor) I don't see any tangible harm from that though.
An indication both from the driver and the firmware is required to allow
the device's power state to remain off during probe (see the first patch).
The use case is such that there is a privacy LED next to an integrated
user-facing laptop camera, and this LED is there to signal the user that
the camera is recording a video or capturing images. That LED also happens
to be wired to one of the power supplies of the camera, so whenever you
power on the camera, the LED will be lit, whether images are captured from
the camera --- or not. There's no way to implement this differently
without additional software control (allowing of which is itself a
hardware design decision) on most CSI-2-connected camera sensors as they
simply have no pin to signal the camera streaming state.
This is also what happens during driver probe: the camera will be powered
on by the I²C subsystem calling dev_pm_domain_attach() and the device is
already powered on when the driver's own probe function is called. To the
user this visible during the boot process as a blink of the privacy LED,
suggesting that the camera is recording without the user having used an
application to do that. From the end user's point of view the behaviour is
not expected and for someone unfamiliar with internal workings of a
computer surely seems quite suspicious --- even if images are not being
actually captured.
I've tested these on linux-next master. They also apply to Wolfram's
i2c/for-next branch, there's a patch that affects the I²C core changes
here (see below). The patches apart from that apply to Bartosz's
at24/for-next as well as Mauro's linux-media master branch.
since v4 <URL:https://lore.kernel.org/linux-acpi/[email protected]/>:
- Rename "probe-low-power" property as "allow-low-power-probe". This is
taken into account in function and file naming, too.
- Turn probe_low_power field in struct i2c_driver into flags field.
- Rebase on Wolfram's i2c/for-next branch that contains the removal of the
support for disabling I²C core IRQ mappings (commit
0c2a34937f7e4c4776bb261114c475392da2355c).
- Change wording for "allow-low-power-probe" property in ACPI
documentation.
since v3 <URL:https://lore.kernel.org/linux-acpi/[email protected]/T/#t>:
- Rework the 2nd patch based on Rafael's comments
- Rework description of the ACPI low power state helper function,
according to Rafael's text.
- Rename and rework the same function as
acpi_dev_state_low_power().
- Reflect the changes in commit message as well.
- Added a patch to document the probe-low-power _DSD property.
since v2 <URL:https://patchwork.kernel.org/cover/11114255/>:
- Remove extra CONFIG_PM ifdefs; these are not needed.
- Move the checks for power state hints from drivers/base/dd.c to
drivers/i2c/i2c-base-core.c; these are I²C devices anyway.
- Move the probe_low_power field from struct device_driver to struct
i2c_driver.
since v1:
- Rename probe_powered_off struct device field as probe_low_power and
reflect the similar naming to the patches overall.
- Work with CONFIG_PM disabled, too.
Rajmohan Mani (1):
media: i2c: imx319: Support probe while the device is off
Sakari Ailus (5):
i2c: Allow driver to manage the device's power state during probe
ACPI: Add a convenience function to tell a device is in low power
state
ov5670: Support probe whilst the device is in a low power state
at24: Support probing while off
Documentation: ACPI: Document allow-low-power-probe _DSD property
.../acpi/dsd/allow-low-power-probe.rst | 28 +++++++++++++
Documentation/firmware-guide/acpi/index.rst | 1 +
drivers/acpi/device_pm.c | 31 ++++++++++++++
drivers/i2c/i2c-core-base.c | 17 ++++++--
drivers/media/i2c/imx319.c | 23 ++++++-----
drivers/media/i2c/ov5670.c | 23 ++++++-----
drivers/misc/eeprom/at24.c | 40 ++++++++++++-------
include/linux/acpi.h | 5 +++
include/linux/i2c.h | 14 +++++++
9 files changed, 146 insertions(+), 36 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
--
2.20.1
On 8/10/20 10:27 PM, Sakari Ailus wrote:
> Hi all,
>
...snip...
>
> The use case is such that there is a privacy LED next to an integrated
> user-facing laptop camera, and this LED is there to signal the user that
> the camera is recording a video or capturing images. That LED also happens
> to be wired to one of the power supplies of the camera, so whenever you
> power on the camera, the LED will be lit, whether images are captured from
> the camera --- or not. There's no way to implement this differently
> without additional software control (allowing of which is itself a
> hardware design decision) on most CSI-2-connected camera sensors as they
> simply have no pin to signal the camera streaming state.
>
> This is also what happens during driver probe: the camera will be powered
> on by the I?C subsystem calling dev_pm_domain_attach() and the device is
> already powered on when the driver's own probe function is called. To the
> user this visible during the boot process as a blink of the privacy LED,
> suggesting that the camera is recording without the user having used an
> application to do that. From the end user's point of view the behaviour is
> not expected and for someone unfamiliar with internal workings of a
> computer surely seems quite suspicious --- even if images are not being
> actually captured.
>
> I've tested these on linux-next master. They also apply to Wolfram's
> i2c/for-next branch, there's a patch that affects the I?C core changes
> here (see below). The patches apart from that apply to Bartosz's
> at24/for-next as well as Mauro's linux-media master branch.
Sakari, we meet one issue - once the vcm sub-device registered, the user space
will try to open the VCM (I have not figure out who did that), it will also
trigger the acpi pm resume/suspend, as the VCM always shares same power rail
with camera sensor, so the privacy LED still has a blink.
>
...snip...
--
Best regards,
Bingbu Cao
On 8/14/20 12:11 PM, Bingbu Cao wrote:
>
>
> On 8/10/20 10:27 PM, Sakari Ailus wrote:
>> Hi all,
>>
> ...snip...
>>
>> The use case is such that there is a privacy LED next to an integrated
>> user-facing laptop camera, and this LED is there to signal the user that
>> the camera is recording a video or capturing images. That LED also happens
>> to be wired to one of the power supplies of the camera, so whenever you
>> power on the camera, the LED will be lit, whether images are captured from
>> the camera --- or not. There's no way to implement this differently
>> without additional software control (allowing of which is itself a
>> hardware design decision) on most CSI-2-connected camera sensors as they
>> simply have no pin to signal the camera streaming state.
>>
>> This is also what happens during driver probe: the camera will be powered
>> on by the I?C subsystem calling dev_pm_domain_attach() and the device is
>> already powered on when the driver's own probe function is called. To the
>> user this visible during the boot process as a blink of the privacy LED,
>> suggesting that the camera is recording without the user having used an
>> application to do that. From the end user's point of view the behaviour is
>> not expected and for someone unfamiliar with internal workings of a
>> computer surely seems quite suspicious --- even if images are not being
>> actually captured.
>>
>> I've tested these on linux-next master. They also apply to Wolfram's
>> i2c/for-next branch, there's a patch that affects the I?C core changes
>> here (see below). The patches apart from that apply to Bartosz's
>> at24/for-next as well as Mauro's linux-media master branch.
>
> Sakari, we meet one issue - once the vcm sub-device registered, the user space
> will try to open the VCM (I have not figure out who did that), it will also
> trigger the acpi pm resume/suspend, as the VCM always shares same power rail
> with camera sensor, so the privacy LED still has a blink.
Sakari, please ignore my previous comment, it is not related to this change. I
see the sub device open is caused by v4l_id program from udev.
>
>>
> ...snip...
>
--
Best regards,
Bingbu Cao
On Fri, Aug 14, 2020 at 6:12 AM Bingbu Cao <[email protected]> wrote:
>
>
>
> On 8/10/20 10:27 PM, Sakari Ailus wrote:
> > Hi all,
> >
> ...snip...
> >
> > The use case is such that there is a privacy LED next to an integrated
> > user-facing laptop camera, and this LED is there to signal the user that
> > the camera is recording a video or capturing images. That LED also happens
> > to be wired to one of the power supplies of the camera, so whenever you
> > power on the camera, the LED will be lit, whether images are captured from
> > the camera --- or not. There's no way to implement this differently
> > without additional software control (allowing of which is itself a
> > hardware design decision) on most CSI-2-connected camera sensors as they
> > simply have no pin to signal the camera streaming state.
> >
> > This is also what happens during driver probe: the camera will be powered
> > on by the I²C subsystem calling dev_pm_domain_attach() and the device is
> > already powered on when the driver's own probe function is called. To the
> > user this visible during the boot process as a blink of the privacy LED,
> > suggesting that the camera is recording without the user having used an
> > application to do that. From the end user's point of view the behaviour is
> > not expected and for someone unfamiliar with internal workings of a
> > computer surely seems quite suspicious --- even if images are not being
> > actually captured.
> >
> > I've tested these on linux-next master. They also apply to Wolfram's
> > i2c/for-next branch, there's a patch that affects the I²C core changes
> > here (see below). The patches apart from that apply to Bartosz's
> > at24/for-next as well as Mauro's linux-media master branch.
>
> Sakari, we meet one issue - once the vcm sub-device registered, the user space
> will try to open the VCM (I have not figure out who did that), it will also
> trigger the acpi pm resume/suspend, as the VCM always shares same power rail
> with camera sensor, so the privacy LED still has a blink.
It's not always the case, as on some designs there are multiple power
rails to the sensor and one drives the LED, while the other drives the
VCM. That said, it would be still good to solve it in either case.
Perhaps we need some more general discussion on the side effects of
simply opening and querying a device. Most of V4L2 drivers these days
are designed to avoid powering up the hardware until it's absolutely
needed to do so. However, for non-streaming subdevs that are directly
controlled by the userspace, like VCM, it's a common practice to power
up on open and down on release. This is because they don't have a
"streaming" state, so the driver has no way to determine when the
power is needed. I wonder if there is a way to improve this.
Best regards,
Tomasz
Hi Tomasz, Bingbu,
On Fri, Aug 14, 2020 at 03:17:58PM +0200, Tomasz Figa wrote:
> On Fri, Aug 14, 2020 at 6:12 AM Bingbu Cao <[email protected]> wrote:
> >
> >
> >
> > On 8/10/20 10:27 PM, Sakari Ailus wrote:
> > > Hi all,
> > >
> > ...snip...
> > >
> > > The use case is such that there is a privacy LED next to an integrated
> > > user-facing laptop camera, and this LED is there to signal the user that
> > > the camera is recording a video or capturing images. That LED also happens
> > > to be wired to one of the power supplies of the camera, so whenever you
> > > power on the camera, the LED will be lit, whether images are captured from
> > > the camera --- or not. There's no way to implement this differently
> > > without additional software control (allowing of which is itself a
> > > hardware design decision) on most CSI-2-connected camera sensors as they
> > > simply have no pin to signal the camera streaming state.
> > >
> > > This is also what happens during driver probe: the camera will be powered
> > > on by the I?C subsystem calling dev_pm_domain_attach() and the device is
> > > already powered on when the driver's own probe function is called. To the
> > > user this visible during the boot process as a blink of the privacy LED,
> > > suggesting that the camera is recording without the user having used an
> > > application to do that. From the end user's point of view the behaviour is
> > > not expected and for someone unfamiliar with internal workings of a
> > > computer surely seems quite suspicious --- even if images are not being
> > > actually captured.
> > >
> > > I've tested these on linux-next master. They also apply to Wolfram's
> > > i2c/for-next branch, there's a patch that affects the I?C core changes
> > > here (see below). The patches apart from that apply to Bartosz's
> > > at24/for-next as well as Mauro's linux-media master branch.
> >
> > Sakari, we meet one issue - once the vcm sub-device registered, the user space
> > will try to open the VCM (I have not figure out who did that), it will also
> > trigger the acpi pm resume/suspend, as the VCM always shares same power rail
> > with camera sensor, so the privacy LED still has a blink.
>
> It's not always the case, as on some designs there are multiple power
> rails to the sensor and one drives the LED, while the other drives the
> VCM. That said, it would be still good to solve it in either case.
>
> Perhaps we need some more general discussion on the side effects of
> simply opening and querying a device. Most of V4L2 drivers these days
> are designed to avoid powering up the hardware until it's absolutely
> needed to do so. However, for non-streaming subdevs that are directly
> controlled by the userspace, like VCM, it's a common practice to power
> up on open and down on release. This is because they don't have a
> "streaming" state, so the driver has no way to determine when the
> power is needed. I wonder if there is a way to improve this.
Good question.
I think in this particular case the device could be powered on only when
there's an open file handle and current is applied on the coil (i.e. the
control's value is non-zero).
But in general case more IOCTLs would be needed. PM QoS framework could be
used for the purpose based on wakeup latency. I guess no-one has needed it
badly enough to implement the support? This would be actually a better
approach. In any case in the case of the lens it requires that the current
behaviour of powering on the device based on just open file handles would
have to change.
--
Kind regards,
Sakari Ailus