Date:	Thu, 23 Apr 2015 11:19:50 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	Doug Anderson <dianders@...omium.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: Enabling regulators form userspace

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists