sysfs and power management

sysfs and power management

[Onkalo Samu]

Hi

I have recently sent couple of sensor drivers to kernel.
Interface is purely sysfs based. The hard part is to get
power management to work reasonable way.

With /dev/* interface power management is quite easy to do since
driver gets clear open and release calls. With sysfs, there is only
read / write methods available.
My solution has been separate sysfs entry which controls operating
state. However, there are some problems with that.
In case of boolean type enable / disable control, there must be
only one userspace component which decides sensor state.
In case of counting type of control, there is a change that
sensor is permanently left on if the userspace component crashes.
This would lead to unbalanced enable / disable count.

If the sensor functionality is based on interrupts (like threshold
interrupt), it may take long time before there is any activity.
In this kind of case timeout from the previous sysfs access doesn't make
much sense. 
I started to wonder if it makes sense to enhance sysfs so that it
optionally prodives open / close call backs. Internally sysfs has some 
bookkeeping about the refcount but this is not visible to the driver.
Of course majority of the sysfs users doesn't need that at all
and for them this is just overhead. 

Is it out of the question to provide call back to driver when 
some sysfs entry is opened (first time) and when refcnt goes back
to 0?

Regards,
Samu

Re: sysfs and power management

[Alan Cox]

> I started to wonder if it makes sense to enhance sysfs so that it
> optionally prodives open / close call backs. Internally sysfs has
> some bookkeeping about the refcount but this is not visible to the
> driver. Of course majority of the sysfs users doesn't need that at all
> and for them this is just overhead. 

I think we need it. There doesn't need to be much overhead however as
there is no need (or sense) in providing per sysfs node open/close
hooks.

The pm layer also lacks a clean race-free way to actually ascertain
when the device was last kicked out of pm saving.

RE: sysfs and power management

[samu.p.onkalo]

>-----Original Message-----
>From: ext Alan Cox [mailto:alan@linux.intel.com]
>
>> I started to wonder if it makes sense to enhance sysfs so that it
>> optionally prodives open / close call backs. Internally sysfs has
>> some bookkeeping about the refcount but this is not visible to the
>> driver. Of course majority of the sysfs users doesn't need that at all
>> and for them this is just overhead.
>
>I think we need it. There doesn't need to be much overhead however as
>there is no need (or sense) in providing per sysfs node open/close
>hooks.

Do you mean per device hook and each sysfs open / close uses 
per device ref-counting.

So the driver would just know that some sysfs  entry is open 
or all the entries are closed. I think that this kind of behavior
is ok for this purpose.

>
>The pm layer also lacks a clean race-free way to actually ascertain
>when the device was last kicked out of pm saving.

Re: sysfs and power management

[Alan Cox]

> Do you mean per device hook and each sysfs open / close uses 
> per device ref-counting.

Or you can do it by having an optional per device hook which is passed
the sysfs node for each open/close and leave the rest up to the driver.
If it needs to do clever stuff it can.

Alan

Re: sysfs and power management

[Greg KH]

On Wed, Oct 27, 2010 at 03:28:09PM +0100, Alan Cox wrote:
> > Do you mean per device hook and each sysfs open / close uses 
> > per device ref-counting.
> 
> Or you can do it by having an optional per device hook which is passed
> the sysfs node for each open/close and leave the rest up to the driver.
> If it needs to do clever stuff it can.

I really don't want to add open/close to the sysfs file model for the
driver core.

What is the specific problem with not doing any sensor work until
userspace asks for the data?  Then do the read from the hardware and go
back to sleep.  That's probably the best way to do this, as userspace
isn't going to open the sysfs file and not close it instantly anyway
after it has read the data (seeking on a sysfs file isn't really
recommended, even if it sometimes seems to work.)

thanks,

greg k-h

Re: sysfs and power management

[Henrique de Moraes Holschuh]

On Fri, 29 Oct 2010, Greg KH wrote:
> back to sleep.  That's probably the best way to do this, as userspace
> isn't going to open the sysfs file and not close it instantly anyway
> after it has read the data (seeking on a sysfs file isn't really
> recommended, even if it sometimes seems to work.)

Well, it is documented that seek(start of file) on sysfs works, and it
is ABI already (some userspace uses it on poll/select-capable
attributes).  So, maybe seek(somewhere that is not the start of the
file) doesn't work -- and it should return an error in that case if it
doesn't already...  but it is a lot more deterministic than "sometimes"
;-)

So yes, userspace will open() and not close() a sysfs attribute immediately
afterwards.  It is not only shell crap that interfaces to the kernel over
sysfs :-)

It would make a lot of sense to support the poll/select model on any
sensor for which you have event-driven notifications of change from the
hardware or firmware.

I don't know about open/close notifications, however.  Usually you need
those when you're going to stream something to userspace, and there are FAR
better interfaces to use in that case, such as the ring buffers used by the
data acquisition devices, netlink (which userspace programmers seems not to
like much :p), input devices, and generic character devices...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: sysfs and power management

[Onkalo Samu]

On Sat, 2010-10-30 at 16:00 +0200, ext Henrique de Moraes Holschuh
wrote:
> On Fri, 29 Oct 2010, Greg KH wrote:
> > back to sleep.  That's probably the best way to do this, as userspace
> > isn't going to open the sysfs file and not close it instantly anyway
> > after it has read the data (seeking on a sysfs file isn't really
> > recommended, even if it sometimes seems to work.)
> 
> Well, it is documented that seek(start of file) on sysfs works, and it
> is ABI already (some userspace uses it on poll/select-capable
> attributes).  So, maybe seek(somewhere that is not the start of the
> file) doesn't work -- and it should return an error in that case if it
> doesn't already...  but it is a lot more deterministic than "sometimes"
> ;-)
> 
> So yes, userspace will open() and not close() a sysfs attribute immediately
> afterwards.  It is not only shell crap that interfaces to the kernel over
> sysfs :-)
> 

What I would like to do is:

Control sensors operating mode and regulators based on the userspace
activity. If no-one is interested about the sensor, it can be turned
totally off including its operating power via regulator framework.

So far the only accepted interface for the small sensor seems to be
sysfs. I tried use misc device but it was not accepted.

For example ambient light sensor or proximity sensor may operate fully
under interrupts. There is no need to poll status which is fine with
sysfs. However, problem is that kernel driver have no idea if someone
keeps some sysfs entry open and waits for the data.

What I would like to have is just an indication that at least one of the
sysfs entries is opened by some application. When that condition is
true, the sensor would be powered on and kept running.
And similarly, then all the users has gone, turn the device off.

I can prepare a patch to show what I mean.

-Samu

> It would make a lot of sense to support the poll/select model on any
> sensor for which you have event-driven notifications of change from the
> hardware or firmware.
> 
> I don't know about open/close notifications, however.  Usually you need
> those when you're going to stream something to userspace, and there are FAR
> better interfaces to use in that case, such as the ring buffers used by the
> data acquisition devices, netlink (which userspace programmers seems not to
> like much :p), input devices, and generic character devices...
> 

Re: sysfs and power management

[Greg KH]

On Sun, Oct 31, 2010 at 01:57:55PM +0200, Onkalo Samu wrote:
> On Sat, 2010-10-30 at 16:00 +0200, ext Henrique de Moraes Holschuh
> wrote:
> > On Fri, 29 Oct 2010, Greg KH wrote:
> > > back to sleep.  That's probably the best way to do this, as userspace
> > > isn't going to open the sysfs file and not close it instantly anyway
> > > after it has read the data (seeking on a sysfs file isn't really
> > > recommended, even if it sometimes seems to work.)
> > 
> > Well, it is documented that seek(start of file) on sysfs works, and it
> > is ABI already (some userspace uses it on poll/select-capable
> > attributes).  So, maybe seek(somewhere that is not the start of the
> > file) doesn't work -- and it should return an error in that case if it
> > doesn't already...  but it is a lot more deterministic than "sometimes"
> > ;-)
> > 
> > So yes, userspace will open() and not close() a sysfs attribute immediately
> > afterwards.  It is not only shell crap that interfaces to the kernel over
> > sysfs :-)
> > 
> 
> What I would like to do is:
> 
> Control sensors operating mode and regulators based on the userspace
> activity. If no-one is interested about the sensor, it can be turned
> totally off including its operating power via regulator framework.
> 
> So far the only accepted interface for the small sensor seems to be
> sysfs. I tried use misc device but it was not accepted.

Look at the drivers/staging/iio/ subsystem.  It is working on a
framework that you can use through a character device (I think) to
properly manage your drivers in this manner.

Try working with those developers as I think it is what you are looking
for here.

thanks,

greg k-h

Re: sysfs and power management

[Onkalo Samu]

On Sun, 2010-10-31 at 15:25 +0100, ext Greg KH wrote:

> > 
> > What I would like to do is:
> > 
> > Control sensors operating mode and regulators based on the userspace
> > activity. If no-one is interested about the sensor, it can be turned
> > totally off including its operating power via regulator framework.
> > 
> > So far the only accepted interface for the small sensor seems to be
> > sysfs. I tried use misc device but it was not accepted.
> 
> Look at the drivers/staging/iio/ subsystem.  It is working on a
> framework that you can use through a character device (I think) to
> properly manage your drivers in this manner.
> 
> Try working with those developers as I think it is what you are looking
> for here.
> 

I took a look to that. It seems that iio is more or less sysfs based.
There are ring buffers and event device which are chardev based
but still the data outside ring buffer and the control is sysfs based.

By getting open and close from sysfs would be nice from the driver
point of view. However, I understand that this is just overhead for
majority of the cases.

Regards,
Samu

Re: sysfs and power management

[Alan Cox]

> I took a look to that. It seems that iio is more or less sysfs based.
> There are ring buffers and event device which are chardev based
> but still the data outside ring buffer and the control is sysfs based.

IIO is sysfs dependant, heavyweight and makes no sense for some of the
sysfs based drivers. IIO is also staging based and Linus already threw
out the last attempt to unify these drivers sanely with an ALS layer -
which was smaller, cleaner and better !

> By getting open and close from sysfs would be nice from the driver
> point of view. However, I understand that this is just overhead for
> majority of the cases.

The alternative really is to end up with a parallel 'not quite sysfs'
which is sysfs + open/close. My feeling is its cleaner to have a hook
at the per device level (so we don't bloat the sysfs nodes at all) than
have two copies of sysfs.

Alan

Re: sysfs and power management

[Greg KH]

On Mon, Nov 01, 2010 at 04:57:01PM +0000, Alan Cox wrote:
> > I took a look to that. It seems that iio is more or less sysfs based.
> > There are ring buffers and event device which are chardev based
> > but still the data outside ring buffer and the control is sysfs based.
> 
> IIO is sysfs dependant, heavyweight and makes no sense for some of the
> sysfs based drivers. IIO is also staging based and Linus already threw
> out the last attempt to unify these drivers sanely with an ALS layer -
> which was smaller, cleaner and better !

I think we need to revisit this issue again, before iio is merged to the
main kernel tree.  I've been totally ignoring the iio user/kernel api at
the moment, waiting for things to settle down there.

thanks,

greg k-h

Re: sysfs and power management

[Alan Cox]

On Mon, 1 Nov 2010 11:07:40 -0700
Greg KH <gregkh@suse.de> wrote:

> On Mon, Nov 01, 2010 at 04:57:01PM +0000, Alan Cox wrote:
> > > I took a look to that. It seems that iio is more or less sysfs
> > > based. There are ring buffers and event device which are chardev
> > > based but still the data outside ring buffer and the control is
> > > sysfs based.
> > 
> > IIO is sysfs dependant, heavyweight and makes no sense for some of
> > the sysfs based drivers. IIO is also staging based and Linus
> > already threw out the last attempt to unify these drivers sanely
> > with an ALS layer - which was smaller, cleaner and better !
> 
> I think we need to revisit this issue again, before iio is merged to
> the main kernel tree.  I've been totally ignoring the iio user/kernel
> api at the moment, waiting for things to settle down there

Actually I think there is another way to do it cleanly

Keep a flag per device (or per runtime pm struct of device)

And on the open/close do

	if (runtime_pm on device && device has SYSFS_PM set)
		pm_runtime_foo

so that devices that need to be powered up to handle sysfs requests can
set a single flag and just work.

RE: sysfs and power management

[samu.p.onkalo]

>-----Original Message-----
>From: ext Alan Cox [mailto:alan@linux.intel.com]
>Sent: 03 November, 2010 11:45
>To: Greg KH
>Cc: Onkalo Samu.P (Nokia-MS/Tampere); ext Henrique de Moraes Holschuh;
>Alan Cox; akpm@linux-foundation.org; linux-kernel@vger.kernel.org
>Subject: Re: sysfs and power management
>
>On Mon, 1 Nov 2010 11:07:40 -0700
>Greg KH <gregkh@suse.de> wrote:
>
>> On Mon, Nov 01, 2010 at 04:57:01PM +0000, Alan Cox wrote:
>> > > I took a look to that. It seems that iio is more or less sysfs
>> > > based. There are ring buffers and event device which are chardev
>> > > based but still the data outside ring buffer and the control is
>> > > sysfs based.
>> >
>> > IIO is sysfs dependant, heavyweight and makes no sense for some of
>> > the sysfs based drivers. IIO is also staging based and Linus
>> > already threw out the last attempt to unify these drivers sanely
>> > with an ALS layer - which was smaller, cleaner and better !
>>
>> I think we need to revisit this issue again, before iio is merged to
>> the main kernel tree.  I've been totally ignoring the iio user/kernel
>> api at the moment, waiting for things to settle down there
>
>Actually I think there is another way to do it cleanly
>
>Keep a flag per device (or per runtime pm struct of device)
>
>And on the open/close do
>
>	if (runtime_pm on device && device has SYSFS_PM set)
>		pm_runtime_foo
>
>so that devices that need to be powered up to handle sysfs requests can
>set a single flag and just work.

That is one quite clean way.
sysfs_ops still needs function pointer to device core function which does pm_runtime
calls.
And there is one drawback. Driver doesn't know about new users after the first one.
It may want to refresh results whenever a new user appears. But that is probably not
a big issue.

-Samu

Re: sysfs and power management

[Greg KH]

On Wed, Nov 03, 2010 at 09:44:52AM +0000, Alan Cox wrote:
> On Mon, 1 Nov 2010 11:07:40 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> > On Mon, Nov 01, 2010 at 04:57:01PM +0000, Alan Cox wrote:
> > > > I took a look to that. It seems that iio is more or less sysfs
> > > > based. There are ring buffers and event device which are chardev
> > > > based but still the data outside ring buffer and the control is
> > > > sysfs based.
> > > 
> > > IIO is sysfs dependant, heavyweight and makes no sense for some of
> > > the sysfs based drivers. IIO is also staging based and Linus
> > > already threw out the last attempt to unify these drivers sanely
> > > with an ALS layer - which was smaller, cleaner and better !
> > 
> > I think we need to revisit this issue again, before iio is merged to
> > the main kernel tree.  I've been totally ignoring the iio user/kernel
> > api at the moment, waiting for things to settle down there
> 
> Actually I think there is another way to do it cleanly
> 
> Keep a flag per device (or per runtime pm struct of device)
> 
> And on the open/close do
> 
> 	if (runtime_pm on device && device has SYSFS_PM set)
> 		pm_runtime_foo
> 
> so that devices that need to be powered up to handle sysfs requests can
> set a single flag and just work.

That sounds like a reasonable idea.

thanks,

greg k-h

RE: sysfs and power management

[samu.p.onkalo]

>-----Original Message-----
>From: ext Greg KH [mailto:gregkh@suse.de]
>Sent: 03 November, 2010 15:09
>To: Alan Cox
>Cc: Onkalo Samu.P (Nokia-MS/Tampere); ext Henrique de Moraes Holschuh;
>Alan Cox; akpm@linux-foundation.org; linux-kernel@vger.kernel.org
>Subject: Re: sysfs and power management
>
>On Wed, Nov 03, 2010 at 09:44:52AM +0000, Alan Cox wrote:
>> On Mon, 1 Nov 2010 11:07:40 -0700
>> Greg KH <gregkh@suse.de> wrote:
>>
>> > On Mon, Nov 01, 2010 at 04:57:01PM +0000, Alan Cox wrote:
>> > > > I took a look to that. It seems that iio is more or less sysfs
>> > > > based. There are ring buffers and event device which are chardev
>> > > > based but still the data outside ring buffer and the control is
>> > > > sysfs based.
>> > >
>> > > IIO is sysfs dependant, heavyweight and makes no sense for some of
>> > > the sysfs based drivers. IIO is also staging based and Linus
>> > > already threw out the last attempt to unify these drivers sanely
>> > > with an ALS layer - which was smaller, cleaner and better !
>> >
>> > I think we need to revisit this issue again, before iio is merged to
>> > the main kernel tree.  I've been totally ignoring the iio
>user/kernel
>> > api at the moment, waiting for things to settle down there
>>
>> Actually I think there is another way to do it cleanly
>>
>> Keep a flag per device (or per runtime pm struct of device)
>>
>> And on the open/close do
>>
>> 	if (runtime_pm on device && device has SYSFS_PM set)
>> 		pm_runtime_foo
>>
>> so that devices that need to be powered up to handle sysfs requests
>can
>> set a single flag and just work.
>
>That sounds like a reasonable idea.
>

I'm working with the implementation which adds possibility to add per attribute
open_close_notification method with minimal overhead to current system.

-Samu