| Message ID | 20200903081550.6012-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Support running driver's probe for a device powered off | expand |
Hi Sakari, On 03/09/20 10:15, Sakari Ailus wrote: > > 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. Apologies for having joined this discussion this late. This patchset seems a good base to cover a different use case, where I also cannot access the physical device at probe time. I'm going to try these patches, but in my case there are a few differences that need a better understanding. First, I'm using device tree, not ACPI. In addition to adding OF support similar to the work you've done for ACPI, I think instead of acpi_dev_state_low_power() we should have a function that works for both ACPI and DT. Second, even though all the chips I'm interested in are connected via I2C, some of them (IIO sensors) can alternatively be connected via SPI and it would make perfectly sense to use SPI instead of I2C. The "i2c-" prefix added in v8 on the ACPI property looks like a limitation in that respect. The same reasoning applies to the implementation in the I2C core, but implementation could be generalized later. I'd love to know your opinion on the above points.
Hi Luca, On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote: > Hi Sakari, > > On 03/09/20 10:15, Sakari Ailus wrote: > > > > 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. > > Apologies for having joined this discussion this late. No worries. But thanks for the comments. > > This patchset seems a good base to cover a different use case, where I > also cannot access the physical device at probe time. > > I'm going to try these patches, but in my case there are a few > differences that need a better understanding. > > First, I'm using device tree, not ACPI. In addition to adding OF support > similar to the work you've done for ACPI, I think instead of > acpi_dev_state_low_power() we should have a function that works for both > ACPI and DT. acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI power state of the device during probe or remove. It is not needed on DT since the power state of the device is controlled directly by the driver. On I²C ACPI devices, it's the framework that powers them on for probe. You could have a helper function on DT to tell a driver what to do in probe, but the functionality in that case is unrelated. I'll answer to the second point later on.
Hi Sakari, On 11/09/20 15:01, Sakari Ailus wrote: > Hi Luca, > > On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote: >> Hi Sakari, >> >> On 03/09/20 10:15, Sakari Ailus wrote: >>> >>> 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. >> >> Apologies for having joined this discussion this late. > > No worries. But thanks for the comments. > >> >> This patchset seems a good base to cover a different use case, where I >> also cannot access the physical device at probe time. >> >> I'm going to try these patches, but in my case there are a few >> differences that need a better understanding. >> >> First, I'm using device tree, not ACPI. In addition to adding OF support >> similar to the work you've done for ACPI, I think instead of >> acpi_dev_state_low_power() we should have a function that works for both >> ACPI and DT. > > acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI > power state of the device during probe or remove. It is not needed on DT > since the power state of the device is controlled directly by the driver. > On I²C ACPI devices, it's the framework that powers them on for probe. I see, thanks for clarifying. I'm not used to ACPI so I didn't get that. > You could have a helper function on DT to tell a driver what to do in > probe, but the functionality in that case is unrelated. So in case of DT we might think of a function that just tells whether the device is marked to allow low-power probe, but it's just an info from DT: int mydriver_probe(struct i2c_client *client) { ... low_power = of_dev_state_low_power(&client->dev); if (!low_power) { mydriver_initialize(); /* power+clocks, write regs */ } ... } ...and, if (low_power), call mydriver_initialize() at first usage. I'm wondering whether this might make sense in mainline.
Hi Sakari, On 14/09/20 11:47, Sakari Ailus wrote: > Hi Luca, > > On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote: >> Hi Sakari, >> >> On 11/09/20 15:01, Sakari Ailus wrote: >>> Hi Luca, >>> >>> On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote: >>>> Hi Sakari, >>>> >>>> On 03/09/20 10:15, Sakari Ailus wrote: >>>>> >>>>> 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. >>>> >>>> Apologies for having joined this discussion this late. >>> >>> No worries. But thanks for the comments. >>> >>>> >>>> This patchset seems a good base to cover a different use case, where I >>>> also cannot access the physical device at probe time. >>>> >>>> I'm going to try these patches, but in my case there are a few >>>> differences that need a better understanding. >>>> >>>> First, I'm using device tree, not ACPI. In addition to adding OF support >>>> similar to the work you've done for ACPI, I think instead of >>>> acpi_dev_state_low_power() we should have a function that works for both >>>> ACPI and DT. >>> >>> acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI >>> power state of the device during probe or remove. It is not needed on DT >>> since the power state of the device is controlled directly by the driver. >>> On I²C ACPI devices, it's the framework that powers them on for probe. >> >> I see, thanks for clarifying. I'm not used to ACPI so I didn't get that. >> >>> You could have a helper function on DT to tell a driver what to do in >>> probe, but the functionality in that case is unrelated. >> >> So in case of DT we might think of a function that just tells whether >> the device is marked to allow low-power probe, but it's just an info >> from DT: >> >> int mydriver_probe(struct i2c_client *client) >> { >> ... >> low_power = of_dev_state_low_power(&client->dev); >> if (!low_power) { >> mydriver_initialize(); /* power+clocks, write regs */ >> } >> ... >> } >> >> ...and, if (low_power), call mydriver_initialize() at first usage. >> >> I'm wondering whether this might make sense in mainline. > > Quite possibly, if there are drivers that would need it. > > The function should probably be called differently though as what it does > is quite different after all. > > Unless... we did the following: > > - Redefine the I²C driver flag added by this patchset into what tells the > I²C framework whether the driver does its own power management > independently of the I²C framework. It could be called e.g. > I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power > management of the device, and the I²C framework would not power on the > device for probe or remove. > > - Add a firmware function to tell whether the device identification should > take place during probe or not. For this is what we're really doing here > from driver's point of view: lazy device probing. Indeed my needs have nothing to do with power management. What I need is lazy device probing as the I2C bus may need time before it can be used. From the driver code point of view it looks similar (there's an if() around initializations in probe() and init is done later if needed), but the usage is different. Another approach would be to add a new I2C driver operation [say init_hw()], then move code for lazy init out of probe() into init_hw(). probe() would still allocate resources. init_hw() would be called by the framework (or the controller driver?) when it knows eveything is ready. Just wild thoughts while I'm trying to focus the problem... > There are no dependencies between the two but they can be used together to > implement the same functionality as this patchset currently does. This way > also the differences between driver implementations for ACPI and DT can be > reduced as the logic is the same. > > Further on, with this approach, if other busses happen to need this > functionality in the future, it would be straightforward to add support --- > it only takes to query whether it's indicated by the DT or ACPI property. > > Thoughts, opinions? >
Hi Luca, On Mon, Sep 14, 2020 at 06:49:29PM +0200, Luca Ceresoli wrote: > Hi Sakari, > > On 14/09/20 11:47, Sakari Ailus wrote: > > Hi Luca, > > > > On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote: > >> Hi Sakari, > >> > >> On 11/09/20 15:01, Sakari Ailus wrote: > >>> Hi Luca, > >>> > >>> On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote: > >>>> Hi Sakari, > >>>> > >>>> On 03/09/20 10:15, Sakari Ailus wrote: > >>>>> > >>>>> 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. > >>>> > >>>> Apologies for having joined this discussion this late. > >>> > >>> No worries. But thanks for the comments. > >>> > >>>> > >>>> This patchset seems a good base to cover a different use case, where I > >>>> also cannot access the physical device at probe time. > >>>> > >>>> I'm going to try these patches, but in my case there are a few > >>>> differences that need a better understanding. > >>>> > >>>> First, I'm using device tree, not ACPI. In addition to adding OF support > >>>> similar to the work you've done for ACPI, I think instead of > >>>> acpi_dev_state_low_power() we should have a function that works for both > >>>> ACPI and DT. > >>> > >>> acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI > >>> power state of the device during probe or remove. It is not needed on DT > >>> since the power state of the device is controlled directly by the driver. > >>> On I²C ACPI devices, it's the framework that powers them on for probe. > >> > >> I see, thanks for clarifying. I'm not used to ACPI so I didn't get that. > >> > >>> You could have a helper function on DT to tell a driver what to do in > >>> probe, but the functionality in that case is unrelated. > >> > >> So in case of DT we might think of a function that just tells whether > >> the device is marked to allow low-power probe, but it's just an info > >> from DT: > >> > >> int mydriver_probe(struct i2c_client *client) > >> { > >> ... > >> low_power = of_dev_state_low_power(&client->dev); > >> if (!low_power) { > >> mydriver_initialize(); /* power+clocks, write regs */ > >> } > >> ... > >> } > >> > >> ...and, if (low_power), call mydriver_initialize() at first usage. > >> > >> I'm wondering whether this might make sense in mainline. > > > > Quite possibly, if there are drivers that would need it. > > > > The function should probably be called differently though as what it does > > is quite different after all. > > > > Unless... we did the following: > > > > - Redefine the I²C driver flag added by this patchset into what tells the > > I²C framework whether the driver does its own power management > > independently of the I²C framework. It could be called e.g. > > I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power > > management of the device, and the I²C framework would not power on the > > device for probe or remove. > > > > - Add a firmware function to tell whether the device identification should > > take place during probe or not. For this is what we're really doing here > > from driver's point of view: lazy device probing. > > Indeed my needs have nothing to do with power management. What I need is > lazy device probing as the I2C bus may need time before it can be used. > From the driver code point of view it looks similar (there's an if() > around initializations in probe() and init is done later if needed), but > the usage is different. > > Another approach would be to add a new I2C driver operation [say > init_hw()], then move code for lazy init out of probe() into init_hw(). > probe() would still allocate resources. init_hw() would be called by the > framework (or the controller driver?) when it knows eveything is ready. > Just wild thoughts while I'm trying to focus the problem... What makes the controller driver not ready to operate the controller when the client devices are probed?
Hi Sakari, On 23/09/20 13:08, Sakari Ailus wrote: > Hi Luca, > > On Mon, Sep 14, 2020 at 06:49:29PM +0200, Luca Ceresoli wrote: >> Hi Sakari, >> >> On 14/09/20 11:47, Sakari Ailus wrote: >>> Hi Luca, >>> >>> On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote: >>>> Hi Sakari, >>>> >>>> On 11/09/20 15:01, Sakari Ailus wrote: >>>>> Hi Luca, >>>>> >>>>> On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote: >>>>>> Hi Sakari, >>>>>> >>>>>> On 03/09/20 10:15, Sakari Ailus wrote: >>>>>>> >>>>>>> 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. >>>>>> >>>>>> Apologies for having joined this discussion this late. >>>>> >>>>> No worries. But thanks for the comments. >>>>> >>>>>> >>>>>> This patchset seems a good base to cover a different use case, where I >>>>>> also cannot access the physical device at probe time. >>>>>> >>>>>> I'm going to try these patches, but in my case there are a few >>>>>> differences that need a better understanding. >>>>>> >>>>>> First, I'm using device tree, not ACPI. In addition to adding OF support >>>>>> similar to the work you've done for ACPI, I think instead of >>>>>> acpi_dev_state_low_power() we should have a function that works for both >>>>>> ACPI and DT. >>>>> >>>>> acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI >>>>> power state of the device during probe or remove. It is not needed on DT >>>>> since the power state of the device is controlled directly by the driver. >>>>> On I²C ACPI devices, it's the framework that powers them on for probe. >>>> >>>> I see, thanks for clarifying. I'm not used to ACPI so I didn't get that. >>>> >>>>> You could have a helper function on DT to tell a driver what to do in >>>>> probe, but the functionality in that case is unrelated. >>>> >>>> So in case of DT we might think of a function that just tells whether >>>> the device is marked to allow low-power probe, but it's just an info >>>> from DT: >>>> >>>> int mydriver_probe(struct i2c_client *client) >>>> { >>>> ... >>>> low_power = of_dev_state_low_power(&client->dev); >>>> if (!low_power) { >>>> mydriver_initialize(); /* power+clocks, write regs */ >>>> } >>>> ... >>>> } >>>> >>>> ...and, if (low_power), call mydriver_initialize() at first usage. >>>> >>>> I'm wondering whether this might make sense in mainline. >>> >>> Quite possibly, if there are drivers that would need it. >>> >>> The function should probably be called differently though as what it does >>> is quite different after all. >>> >>> Unless... we did the following: >>> >>> - Redefine the I²C driver flag added by this patchset into what tells the >>> I²C framework whether the driver does its own power management >>> independently of the I²C framework. It could be called e.g. >>> I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power >>> management of the device, and the I²C framework would not power on the >>> device for probe or remove. >>> >>> - Add a firmware function to tell whether the device identification should >>> take place during probe or not. For this is what we're really doing here >>> from driver's point of view: lazy device probing. >> >> Indeed my needs have nothing to do with power management. What I need is >> lazy device probing as the I2C bus may need time before it can be used. >> From the driver code point of view it looks similar (there's an if() >> around initializations in probe() and init is done later if needed), but >> the usage is different. >> >> Another approach would be to add a new I2C driver operation [say >> init_hw()], then move code for lazy init out of probe() into init_hw(). >> probe() would still allocate resources. init_hw() would be called by the >> framework (or the controller driver?) when it knows eveything is ready. >> Just wild thoughts while I'm trying to focus the problem... > > What makes the controller driver not ready to operate the controller when > the client devices are probed? I'm working with a camera module connected via a serializer/deserializer chip pair, and the link can go away and get back at any moment for various reasons: mechanical, electromagnetic, cable not connected etc. This scenario is not well handled with current kernel structures, so I'm handling it partially with some local hacks but I'm always curious about any possible kernel improvement that can improve the situation. See these links for some info about my troubles: https://lucaceresoli.net/plumbers-i2c-bof/ https://elinux.org/images/f/fc/Ceresoli-elce2019-video-serdes-linux.pdf https://youtu.be/7hLv6fYAW-E?list=PLbzoR-pLrL6pamOj4UifcMJf560Ph6mJp Your patchset looks very similar to something I need: not communicating with the device during probe. Another piece would be to trigger a device configuration "in some way" when the link is known to be back.
On Mon, Sep 14, 2020 at 12:47:27PM +0300, Sakari Ailus wrote: > Hi Luca, > > On Mon, Sep 14, 2020 at 09:58:24AM +0200, Luca Ceresoli wrote: > > Hi Sakari, > > > > On 11/09/20 15:01, Sakari Ailus wrote: > > > Hi Luca, > > > > > > On Fri, Sep 11, 2020 at 02:49:26PM +0200, Luca Ceresoli wrote: > > >> Hi Sakari, > > >> > > >> On 03/09/20 10:15, Sakari Ailus wrote: > > >>> > > >>> 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. > > >> > > >> Apologies for having joined this discussion this late. > > > > > > No worries. But thanks for the comments. > > > > > >> > > >> This patchset seems a good base to cover a different use case, where I > > >> also cannot access the physical device at probe time. > > >> > > >> I'm going to try these patches, but in my case there are a few > > >> differences that need a better understanding. > > >> > > >> First, I'm using device tree, not ACPI. In addition to adding OF support > > >> similar to the work you've done for ACPI, I think instead of > > >> acpi_dev_state_low_power() we should have a function that works for both > > >> ACPI and DT. > > > > > > acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI > > > power state of the device during probe or remove. It is not needed on DT > > > since the power state of the device is controlled directly by the driver. > > > On I²C ACPI devices, it's the framework that powers them on for probe. > > > > I see, thanks for clarifying. I'm not used to ACPI so I didn't get that. > > > > > You could have a helper function on DT to tell a driver what to do in > > > probe, but the functionality in that case is unrelated. > > > > So in case of DT we might think of a function that just tells whether > > the device is marked to allow low-power probe, but it's just an info > > from DT: > > > > int mydriver_probe(struct i2c_client *client) > > { > > ... > > low_power = of_dev_state_low_power(&client->dev); > > if (!low_power) { > > mydriver_initialize(); /* power+clocks, write regs */ > > } > > ... > > } > > > > ...and, if (low_power), call mydriver_initialize() at first usage. > > > > I'm wondering whether this might make sense in mainline. > > Quite possibly, if there are drivers that would need it. > > The function should probably be called differently though as what it does > is quite different after all. > > Unless... we did the following: > > - Redefine the I²C driver flag added by this patchset into what tells the > I²C framework whether the driver does its own power management > independently of the I²C framework. It could be called e.g. > I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power > management of the device, and the I²C framework would not power on the > device for probe or remove. > > - Add a firmware function to tell whether the device identification should > take place during probe or not. For this is what we're really doing here > from driver's point of view: lazy device probing. > > There are no dependencies between the two but they can be used together to > implement the same functionality as this patchset currently does. This way > also the differences between driver implementations for ACPI and DT can be > reduced as the logic is the same. > > Further on, with this approach, if other busses happen to need this > functionality in the future, it would be straightforward to add support --- > it only takes to query whether it's indicated by the DT or ACPI property. > > Thoughts, opinions? I think we might be overly complicating things. IMHO the series as is with the "i2c_" prefix removed from the flags introduced would be reusable as is for any other subsystem that needs it. Of course, for now, the handling of the flag would remain implemented only in the I2C subsystem. Best regards, Tomasz
Hi Sakari, On Thu, Sep 03, 2020 at 11:15:44AM +0300, Sakari Ailus wrote: > > 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. Besides the suggestion to make the defintions added less specific to i2c (but still keeping the implementation so for now), feel free to add: Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
> I think we might be overly complicating things. IMHO the series as is > with the "i2c_" prefix removed from the flags introduced would be > reusable as is for any other subsystem that needs it. Of course, for > now, the handling of the flag would remain implemented only in the I2C > subsystem. Just to be clear: you are suggesting to remove "i2c" from the DSD binding "i2c-allow-low-power-probe". And you are not talking about moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I recall the latter has been NACKed by gkh so far.
On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > I think we might be overly complicating things. IMHO the series as is > > with the "i2c_" prefix removed from the flags introduced would be > > reusable as is for any other subsystem that needs it. Of course, for > > now, the handling of the flag would remain implemented only in the I2C > > subsystem. > > Just to be clear: you are suggesting to remove "i2c" from the DSD > binding "i2c-allow-low-power-probe". And you are not talking about > moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I > recall the latter has been NACKed by gkh so far. > I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all the implementation would remain where it is in the code. IOW, I'm just suggesting a naming change to avoid proliferating duplicate flags of the same meaning across subsystems.
On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa <tfiga@chromium.org> wrote: > > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > > > > I think we might be overly complicating things. IMHO the series as is > > > with the "i2c_" prefix removed from the flags introduced would be > > > reusable as is for any other subsystem that needs it. Of course, for > > > now, the handling of the flag would remain implemented only in the I2C > > > subsystem. > > > > Just to be clear: you are suggesting to remove "i2c" from the DSD > > binding "i2c-allow-low-power-probe". And you are not talking about > > moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I > > recall the latter has been NACKed by gkh so far. > > > > I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all > the implementation would remain where it is in the code. IOW, I'm just > suggesting a naming change to avoid proliferating duplicate flags of > the same meaning across subsystems. But that would indicate that the property was recognized by other subsystems which wouldn't be the case, so it would be confusing. That's why it cannot be documented as a general property ATM too.
On Mon, Sep 28, 2020 at 4:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa <tfiga@chromium.org> wrote: > > > > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > > > > > > > I think we might be overly complicating things. IMHO the series as is > > > > with the "i2c_" prefix removed from the flags introduced would be > > > > reusable as is for any other subsystem that needs it. Of course, for > > > > now, the handling of the flag would remain implemented only in the I2C > > > > subsystem. > > > > > > Just to be clear: you are suggesting to remove "i2c" from the DSD > > > binding "i2c-allow-low-power-probe". And you are not talking about > > > moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I > > > recall the latter has been NACKed by gkh so far. > > > > > > > I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all > > the implementation would remain where it is in the code. IOW, I'm just > > suggesting a naming change to avoid proliferating duplicate flags of > > the same meaning across subsystems. > > But that would indicate that the property was recognized by other > subsystems which wouldn't be the case, so it would be confusing. > > That's why it cannot be documented as a general property ATM too. I guess that's true. Well, this is kAPI in the end, so if we have more subsystems, it could be always renamed. So feel free to ignore my previous comment. Best regards, Tomasz
Hi Tomasz, On Mon, Sep 28, 2020 at 06:49:22PM +0200, Tomasz Figa wrote: > On Mon, Sep 28, 2020 at 4:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Sun, Sep 27, 2020 at 9:44 PM Tomasz Figa <tfiga@chromium.org> wrote: > > > > > > On Sun, Sep 27, 2020 at 9:39 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > > > > > > > > > > I think we might be overly complicating things. IMHO the series as is > > > > > with the "i2c_" prefix removed from the flags introduced would be > > > > > reusable as is for any other subsystem that needs it. Of course, for > > > > > now, the handling of the flag would remain implemented only in the I2C > > > > > subsystem. > > > > > > > > Just to be clear: you are suggesting to remove "i2c" from the DSD > > > > binding "i2c-allow-low-power-probe". And you are not talking about > > > > moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I > > > > recall the latter has been NACKed by gkh so far. > > > > > > > > > > I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all > > > the implementation would remain where it is in the code. IOW, I'm just > > > suggesting a naming change to avoid proliferating duplicate flags of > > > the same meaning across subsystems. > > > > But that would indicate that the property was recognized by other > > subsystems which wouldn't be the case, so it would be confusing. > > > > That's why it cannot be documented as a general property ATM too. > > I guess that's true. Well, this is kAPI in the end, so if we have more > subsystems, it could be always renamed. So feel free to ignore my > previous comment. I wouldn't expect this flag to be needed outside I²C since the other potential use case (I3C) appears to be entirely free of power management, so it's up to the drivers on ACPI, too. The property itself, though, might be.
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 v7 <URL:https://lore.kernel.org/linux-acpi/20200901210333.8462-1-sakari.ailus@linux.intel.com/>: - Reorder documentation patch right after the implemenation in the I²C framework. - Rename allow-low-power-probe property as i2c-allow-low-power-probe. - Remove extra "property" from the description of the i2c-allow-low-power-probe property and mention it's a device property. - Add an example to the documentation and refer to the _DSD property spec. since v6 <URL:https://lore.kernel.org/linux-acpi/20200826115432.6103-1-sakari.ailus@linux.intel.com/>: - Use u32 for the flags field in struct i2c_driver. - Use acpi_dev_get_property to read the allow-low-power-probe property. since v5 <URL:https://lore.kernel.org/linux-acpi/20200810142747.12400-1-sakari.ailus@linux.intel.com/>: - Identify sensors when they're first powered on. In previous versions, if this wasn't in probe, it was not done at all. - Return allow_low_power_probe() only for ACPI devices, i.e. OF systems are not affected by these changes. - Document that I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag only applies to ACPI drivers. - Fix extra regulator_disable in at24 driver's remove function when the device was already in low power state. since v4 <URL:https://lore.kernel.org/linux-acpi/20200121134157.20396-1-sakari.ailus@linux.intel.com/>: - 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/20200109154529.19484-1-sakari.ailus@linux.intel.com/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 an ACPI 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 | 19 ++++- drivers/media/i2c/imx319.c | 74 +++++++++++------- drivers/media/i2c/ov5670.c | 76 +++++++++++-------- drivers/misc/eeprom/at24.c | 43 ++++++----- include/linux/acpi.h | 5 ++ include/linux/i2c.h | 14 ++++ 9 files changed, 212 insertions(+), 79 deletions(-) create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst