Re: Enabling regulators form userspace

Re: Enabling regulators form userspace

[Dmitry Torokhov]

On Wed, Apr 22, 2015 at 09:11:12PM +0100, Mark Brown wrote:
> On Wed, Apr 22, 2015 at 12:56:21PM -0700, Dmitry Torokhov wrote:
> 
> Please keep the list in the Ccs unless there's a reason not to, other
> people could benefit from the discussion or even answer.

[ resending with LKML on CC ]

> 
> > I am considering adding a debugs feature/attribute "enable" to allow
> > doing "regulatror_enable" and "regulator_disable" from userspace, how
> > do you feel about it?
> 
> There's already a userspace consumer driver you can bind for test
> purposes which could be used here.

Thanks for the pointer, but I do not think this would work for this use
case, mainly because the controllers in question often impose timing
constraints on the supplies. I.e. while holding reset GPIO you turn on
vdd and then, after at least X msec, avdd, and then release reset GPIO.

>  I guess ideally the I2C userspace
> interface would have code to do something like enumerate all supplies on
> the device and keep them enabled while the device is open from
> userspace, though that won't help things running as shell scripts which
> are probably a common case.

Same as above, the interactions are quite often a bit more complex to be
easily described generically, but is no problem to implement in a
userspace utility that is already product-specific.

> 
> I'm not keen on having something in there as a standard feature, it's
> just this massive "abuse me" flag - there's not really much of a
> production use case for it but lots of "let's just hack around our buggy
> drivers" use case.

I would contend that the drivers are not necessarily buggy: we just do
not want to encumber the kernel driver with all the details about test
interface that is very much controller specific (i.e. we can't easily
generalize it for all permutations of Atmel/Cypress/Eantech/Stnaptics
etc controllers) and that is not going to be used during normal
operation.  However it is very much a production issue as these test
utilities are used during factory runs to ensure quality of produced
hardware.

Thanks.

-- 
Dmitry

Re: Enabling regulators form userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Wed, Apr 22, 2015 at 01:56:44PM -0700, Dmitry Torokhov wrote:
> On Wed, Apr 22, 2015 at 09:11:12PM +0100, Mark Brown wrote:

> > There's already a userspace consumer driver you can bind for test
> > purposes which could be used here.

> Thanks for the pointer, but I do not think this would work for this use
> case, mainly because the controllers in question often impose timing
> constraints on the supplies. I.e. while holding reset GPIO you turn on
> vdd and then, after at least X msec, avdd, and then release reset GPIO.

This really sounds like you should be writing a device driver here, even
if it's just a little module you write separately to the actual device
driver used in real systems.

> > I'm not keen on having something in there as a standard feature, it's
> > just this massive "abuse me" flag - there's not really much of a
> > production use case for it but lots of "let's just hack around our buggy
> > drivers" use case.

> I would contend that the drivers are not necessarily buggy: we just do
> not want to encumber the kernel driver with all the details about test
> interface that is very much controller specific (i.e. we can't easily
> generalize it for all permutations of Atmel/Cypress/Eantech/Stnaptics
> etc controllers) and that is not going to be used during normal
> operation.  However it is very much a production issue as these test
> utilities are used during factory runs to ensure quality of produced
> hardware.

It's not about the quality of an individual driver, it's about the
potential for misuse - making the kernel interfaces such that it's easy
to do the right thing and hard to do the wrong thing.  Having the
ability to just bang on this stuff from userspace seems like it's
encouraging problematic behaviour.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Enabling regulators form userspace

[Dmitry Torokhov]

On Thu, Apr 23, 2015 at 11:35:45AM +0100, Mark Brown wrote:
> On Wed, Apr 22, 2015 at 01:56:44PM -0700, Dmitry Torokhov wrote:
> > On Wed, Apr 22, 2015 at 09:11:12PM +0100, Mark Brown wrote:
> 
> > > There's already a userspace consumer driver you can bind for test
> > > purposes which could be used here.
> 
> > Thanks for the pointer, but I do not think this would work for this use
> > case, mainly because the controllers in question often impose timing
> > constraints on the supplies. I.e. while holding reset GPIO you turn on
> > vdd and then, after at least X msec, avdd, and then release reset GPIO.
> 
> This really sounds like you should be writing a device driver here, even
> if it's just a little module you write separately to the actual device
> driver used in real systems.

No, no way. If it is lives in kernel I have to review it and then either
upstream and maintain it in mainline or maintain it as out-of-tree
driver. If it is in userspace I do not even have to look at it.

> 
> > > I'm not keen on having something in there as a standard feature, it's
> > > just this massive "abuse me" flag - there's not really much of a
> > > production use case for it but lots of "let's just hack around our buggy
> > > drivers" use case.
> 
> > I would contend that the drivers are not necessarily buggy: we just do
> > not want to encumber the kernel driver with all the details about test
> > interface that is very much controller specific (i.e. we can't easily
> > generalize it for all permutations of Atmel/Cypress/Eantech/Stnaptics
> > etc controllers) and that is not going to be used during normal
> > operation.  However it is very much a production issue as these test
> > utilities are used during factory runs to ensure quality of produced
> > hardware.
> 
> It's not about the quality of an individual driver, it's about the
> potential for misuse - making the kernel interfaces such that it's easy
> to do the right thing and hard to do the wrong thing.  Having the
> ability to just bang on this stuff from userspace seems like it's
> encouraging problematic behaviour.

What kind of misuse are you concerned with? I can see that we may not
necessarily want to allow setting arbitrary voltage directly from
userspace, but asking kernel nicely to enable regulator with
regulator_enable() (we may refuse for regulators that are requested in
exclusive use) and similarly disable it with regulator_disable() should
not be any more dangerous than having a random kernel module do that.

Sometimes shoving everything into the kernel is not the best idea; some
tasks are better suited for userspace. We already allow raw userspace
access to i2c, usb, spi, pci, PS/2 ports, parallel ports, and probably
more. Why regulators should be special in this regard and accessible
only through kernel? This causes programs that work fine on one
architecture (x86) to fail when moved to another (arm) with no way of
fixing it.

Thanks.

-- 
Dmitry

Re: Enabling regulators form userspace

[Doug Anderson]

Hi,

On Thu, Apr 23, 2015 at 9:08 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>> It's not about the quality of an individual driver, it's about the
>> potential for misuse - making the kernel interfaces such that it's easy
>> to do the right thing and hard to do the wrong thing.  Having the
>> ability to just bang on this stuff from userspace seems like it's
>> encouraging problematic behaviour.
>
> What kind of misuse are you concerned with? I can see that we may not
> necessarily want to allow setting arbitrary voltage directly from
> userspace, but asking kernel nicely to enable regulator with
> regulator_enable() (we may refuse for regulators that are requested in
> exclusive use) and similarly disable it with regulator_disable() should
> not be any more dangerous than having a random kernel module do that.
>
> Sometimes shoving everything into the kernel is not the best idea; some
> tasks are better suited for userspace. We already allow raw userspace
> access to i2c, usb, spi, pci, PS/2 ports, parallel ports, and probably
> more. Why regulators should be special in this regard and accessible
> only through kernel? This causes programs that work fine on one
> architecture (x86) to fail when moved to another (arm) with no way of
> fixing it.

Personally, I have been annoyed several times by the lack of an easy
way to control regulators from userspace.  While doing development of
regulator code it is very handy to be able to change the voltages from
userspace so I can probe them with a scope.  I have a terribly hacky /
wrong CL <https://chromium-review.googlesource.com/#/c/214475/> that I
actually wrote for this.  Note that userspace consumers were a bit
cumbersome for this purpose.

I wonder whether we could achieve "safety" and "don't abuse" problems
by doing something like:

1. Put the interface in "debugfs", not sysfs.  Hopefully folks know
that userspace isn't supposed to rely on debugfs.  ...though perhaps
userspace in the factory (test tools) are OK to use it as long as they
understand that it is fragile (API may change) and dangerous (could
shoot yourself in the foot).

2. Require a config option to enable it with suitable warnings in the
KConfig option.

3. Force the user to write a "y" into a file named
"i_know_this_is_unsafe", or something of the sort.  Writing this could
write a suitable warning to the kernel log saying "If you let the
magic smoke out, don't blame me".


-Doug

Re: Enabling regulators form userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3224 bytes --]

On Thu, Apr 23, 2015 at 09:08:07AM -0700, Dmitry Torokhov wrote:
> On Thu, Apr 23, 2015 at 11:35:45AM +0100, Mark Brown wrote:

> > This really sounds like you should be writing a device driver here, even
> > if it's just a little module you write separately to the actual device
> > driver used in real systems.

> No, no way. If it is lives in kernel I have to review it and then either
> upstream and maintain it in mainline or maintain it as out-of-tree
> driver. If it is in userspace I do not even have to look at it.

I'm sure someone else has extensive validation requirements for their
userspace code!  Or, for that matter, if this ABI is in your vendor tree
then I do not even have to look at it :)

> > It's not about the quality of an individual driver, it's about the
> > potential for misuse - making the kernel interfaces such that it's easy
> > to do the right thing and hard to do the wrong thing.  Having the
> > ability to just bang on this stuff from userspace seems like it's
> > encouraging problematic behaviour.

> What kind of misuse are you concerned with? I can see that we may not
> necessarily want to allow setting arbitrary voltage directly from
> userspace, but asking kernel nicely to enable regulator with
> regulator_enable() (we may refuse for regulators that are requested in
> exclusive use) and similarly disable it with regulator_disable() should
> not be any more dangerous than having a random kernel module do that.

Things like people putting parts of their power management for things
with actual drivers into userspace, and epecially people mixing user
space and kernel power management.

I also don't buy the idea that a userspace interface wouldn't involve
setting voltages, if we can perform some operations people are going to
want to be able to perform all of them.

> Sometimes shoving everything into the kernel is not the best idea; some
> tasks are better suited for userspace. We already allow raw userspace
> access to i2c, usb, spi, pci, PS/2 ports, parallel ports, and probably
> more. Why regulators should be special in this regard and accessible
> only through kernel? This causes programs that work fine on one
> architecture (x86) to fail when moved to another (arm) with no way of
> fixing it.

Well, they're going to fail on systems that require regulator code
without magic userpace code anyway so we're no worse off here.

I'm not saying we shouldn't have userspace control at all, I'm saying
that we should export coherent devices to userspace rather than just
some random unstructured stuff asking for hacks - something like the
facility I suggested for the userspace interface for the control
interfaces to also export control of the power for the device.  That
seems like it should also be more helpful for userspace programmers to
work with, they ought to be able to readily do things like "turn on all
the power for the device" (and probably have it done for them by
default) rather than have to discover which hoops they have to jump
through on which system.

Userspace control of devices seems like a reasonable thing to want but
that doesn't mean that it's a good idea to just provide userspace with
an unstructured pile of stuff to sort through.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Enabling regulators form userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

On Thu, Apr 23, 2015 at 09:52:40AM -0700, Doug Anderson wrote:

> Personally, I have been annoyed several times by the lack of an easy
> way to control regulators from userspace.  While doing development of
> regulator code it is very handy to be able to change the voltages from
> userspace so I can probe them with a scope.  I have a terribly hacky /
> wrong CL <https://chromium-review.googlesource.com/#/c/214475/> that I
> actually wrote for this.  Note that userspace consumers were a bit
> cumbersome for this purpose.

There's a test consumer in mainline for precisely this purpose, I used
to use it a lot when I was doing bringup of regulator drivers on
evaluation boards - look at virtual.c (which is separate to the
userspace consumer in that it throws usability to the wind, though some
trivial shell scripts fix that).

> I wonder whether we could achieve "safety" and "don't abuse" problems
> by doing something like:

> 1. Put the interface in "debugfs", not sysfs.  Hopefully folks know
> that userspace isn't supposed to rely on debugfs.  ...though perhaps
> userspace in the factory (test tools) are OK to use it as long as they
> understand that it is fragile (API may change) and dangerous (could
> shoot yourself in the foot).

I'm not optimistic about this achieving the desired results, we export
too much stuff through debugfs that's useful in production so all it
really does is give us a bit more ability to say "I told you so".

> 2. Require a config option to enable it with suitable warnings in the
> KConfig option.

> 3. Force the user to write a "y" into a file named
> "i_know_this_is_unsafe", or something of the sort.  Writing this could
> write a suitable warning to the kernel log saying "If you let the
> magic smoke out, don't blame me".

I don't think either of those accomplishes anything meaningful, it's not
like we've got a good track record on getting people to pay attention to
warnings or do transitions.  With echoing into a file I'd just expect to
see patches turning off the warning because of course everyone thinks
that their own use case is totally sensible, reasonable and well thought
through (and some of them actually are).

Like I say I think if we're delegating this to userspace we should
actively choose to do so, not just throw everything over the wall.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Enabling regulators form userspace

[Dmitry Torokhov]

On Thu, Apr 23, 2015 at 06:13:28PM +0100, Mark Brown wrote:
> On Thu, Apr 23, 2015 at 09:08:07AM -0700, Dmitry Torokhov wrote:
> > On Thu, Apr 23, 2015 at 11:35:45AM +0100, Mark Brown wrote:
> 
> > > This really sounds like you should be writing a device driver here, even
> > > if it's just a little module you write separately to the actual device
> > > driver used in real systems.
> 
> > No, no way. If it is lives in kernel I have to review it and then either
> > upstream and maintain it in mainline or maintain it as out-of-tree
> > driver. If it is in userspace I do not even have to look at it.
> 
> I'm sure someone else has extensive validation requirements for their
> userspace code!

There sure is someone. But that someone is not me ;)

> Or, for that matter, if this ABI is in your vendor tree
> then I do not even have to look at it :)

I want it to be in mainline because I am not necessarily wearing my
vendor hat when I ask them to either remove or not implement
drivers-specific sysfs ABI in their driver.

> 
> > > It's not about the quality of an individual driver, it's about the
> > > potential for misuse - making the kernel interfaces such that it's easy
> > > to do the right thing and hard to do the wrong thing.  Having the
> > > ability to just bang on this stuff from userspace seems like it's
> > > encouraging problematic behaviour.
> 
> > What kind of misuse are you concerned with? I can see that we may not
> > necessarily want to allow setting arbitrary voltage directly from
> > userspace, but asking kernel nicely to enable regulator with
> > regulator_enable() (we may refuse for regulators that are requested in
> > exclusive use) and similarly disable it with regulator_disable() should
> > not be any more dangerous than having a random kernel module do that.
> 
> Things like people putting parts of their power management for things
> with actual drivers into userspace, and epecially people mixing user
> space and kernel power management.
> 
> I also don't buy the idea that a userspace interface wouldn't involve
> setting voltages, if we can perform some operations people are going to
> want to be able to perform all of them.

OK, fair enough.

In another message you mentioned virtual.c Unfortunately the patch to
allow driving it through device tree did not seem to go anywhere. IN the
similar fashion, do you think we could add annotation to dveice tree to
mark regulators that we explicitly allow controlling from userspace and
then automatically create sysfs attributes similar to the ones from
vitrual.c?

> 
> > Sometimes shoving everything into the kernel is not the best idea; some
> > tasks are better suited for userspace. We already allow raw userspace
> > access to i2c, usb, spi, pci, PS/2 ports, parallel ports, and probably
> > more. Why regulators should be special in this regard and accessible
> > only through kernel? This causes programs that work fine on one
> > architecture (x86) to fail when moved to another (arm) with no way of
> > fixing it.
> 
> Well, they're going to fail on systems that require regulator code
> without magic userpace code anyway so we're no worse off here.

They are worse: there is not way to fix them so that they work on all
systems because currently we do not export regulator control. If it was
exported the userspace programs would attempt to look up supplies that
given controller uses in the same way the kernel does (i.e my
touchscreen driver looks up vdd and then avdd; if they are not there
kernel gives me dummies, in userspace I'll just shrug and continue). The
regulator knowledge is not board-specific but rather
controller-specific.

> 
> I'm not saying we shouldn't have userspace control at all, I'm saying
> that we should export coherent devices to userspace rather than just
> some random unstructured stuff asking for hacks - something like the
> facility I suggested for the userspace interface for the control
> interfaces to also export control of the power for the device.  That
> seems like it should also be more helpful for userspace programmers to
> work with, they ought to be able to readily do things like "turn on all
> the power for the device" (and probably have it done for them by
> default) rather than have to discover which hoops they have to jump
> through on which system.
> 
> Userspace control of devices seems like a reasonable thing to want but
> that doesn't mean that it's a good idea to just provide userspace with
> an unstructured pile of stuff to sort through.

We give them all the tools necessary and that should be enough. Maybe it
can be generalized in a library if it proves too much of a pain - you
give device description (gpio, supplies, clocks, whatever) and a library
call with do a right thing for you - it does not mean it has to be in
kernel. Right now all these dependencies live either in individual
driver or in userspace; if we provide "coherent access" we need to put
this knowledge into interface layer somehow. I really do not think
i2c-dev should know about gpio, clocks, regulators, timings, ordering,
etc. Another option, also unappealing, is having driver implement
interface to keep the device powered, not suspended, not runtime
suspended, but otherwise untouched, until userspace is done with
accessing it, and then go through re-initialization, similar to
unbind/rebind, bit not quite. This is quite complex and has to be done
in each driver. And we would need to have a kernel driver even for
devices that we intend to drive purely from userspace. I do not think
that this is the right direction to go in.

Thanks.

-- 
Dmitry

Re: Enabling regulators form userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]

On Thu, Apr 23, 2015 at 11:19:50AM -0700, Dmitry Torokhov wrote:
> On Thu, Apr 23, 2015 at 06:13:28PM +0100, Mark Brown wrote:

> > Or, for that matter, if this ABI is in your vendor tree
> > then I do not even have to look at it :)

> I want it to be in mainline because I am not necessarily wearing my
> vendor hat when I ask them to either remove or not implement
> drivers-specific sysfs ABI in their driver.

So, that's an interesting but separate discussion I think which isn't
just confined to test interfaces - we also have some fun things going on
with callibration data for example.

> In another message you mentioned virtual.c Unfortunately the patch to
> allow driving it through device tree did not seem to go anywhere. IN the
> similar fashion, do you think we could add annotation to dveice tree to
> mark regulators that we explicitly allow controlling from userspace and
> then automatically create sysfs attributes similar to the ones from
> vitrual.c?

I'd still be a lot happier if that annotation were in the driver for the
device, there's the whole DT as hardware description thing going on here
and punting to userspace definitely seems like something we might
change our minds on later (in either direction).  For a tightly
integrated embedded system it makes no odds but they're not the only
users.

Please bear in mind that the userbase here also includes the RPi/BBB
communities where it seems difficult to get things like DT overlays
adopted, there's a lot of "just export everything to userspace,
whatever" going on which can lead to misery later on when things break
and you have to unpick what's going on.

> > > Sometimes shoving everything into the kernel is not the best idea; some
> > > tasks are better suited for userspace. We already allow raw userspace
> > > access to i2c, usb, spi, pci, PS/2 ports, parallel ports, and probably
> > > more. Why regulators should be special in this regard and accessible
> > > only through kernel? This causes programs that work fine on one
> > > architecture (x86) to fail when moved to another (arm) with no way of
> > > fixing it.

> > Well, they're going to fail on systems that require regulator code
> > without magic userpace code anyway so we're no worse off here.

> They are worse: there is not way to fix them so that they work on all
> systems because currently we do not export regulator control. If it was
> exported the userspace programs would attempt to look up supplies that
> given controller uses in the same way the kernel does (i.e my
> touchscreen driver looks up vdd and then avdd; if they are not there
> kernel gives me dummies, in userspace I'll just shrug and continue). The
> regulator knowledge is not board-specific but rather
> controller-specific.

So, that's sounding a lot more like the organized export I was asking
for...

> > Userspace control of devices seems like a reasonable thing to want but
> > that doesn't mean that it's a good idea to just provide userspace with
> > an unstructured pile of stuff to sort through.

> We give them all the tools necessary and that should be enough. Maybe it
> can be generalized in a library if it proves too much of a pain - you
> give device description (gpio, supplies, clocks, whatever) and a library
> call with do a right thing for you - it does not mean it has to be in
> kernel. Right now all these dependencies live either in individual
> driver or in userspace; if we provide "coherent access" we need to put

No, they don't live in drivers - they live in DT and board files.  It's
not the "I need resource X" bit that's interesting, it's how you figure
out what resource that is on a random board.

> this knowledge into interface layer somehow. I really do not think
> i2c-dev should know about gpio, clocks, regulators, timings, ordering,
> etc. Another option, also unappealing, is having driver implement

If we're going down this road I'd expect us to provide the actual
implementation of the interface in the regulator subsystem and then have
something that I2C, SPI and so on can just call into (eg, ioctl() and
sysfs hooks).  I do agree that having every bus explicitly worry about
every resource would be ugly.

Timings and things probably are best punted to userspace, though I would
expect the overwhelming majority of these devices to be OK with just
being powered up.  I guess it's safer to just leave userspace to worry
about things though.

> interface to keep the device powered, not suspended, not runtime
> suspended, but otherwise untouched, until userspace is done with
> accessing it, and then go through re-initialization, similar to
> unbind/rebind, bit not quite. This is quite complex and has to be done
> in each driver. And we would need to have a kernel driver even for
> devices that we intend to drive purely from userspace. I do not think
> that this is the right direction to go in.

I'm not sure I follow this entirely.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]