[lm-sensors] General API and documentation discussion

[lm-sensors] General API and documentation discussion

[Lucas Stach]

Hello all,

weeks ago I proposed a patch to introduce a new hwmon-core interface to
have the sysfs handling centralised and getting a stable in-kernel API
for the hwmon drivers. This should also lead to resolving the now
unclear situation with regard to hwmon/thermal driver cooperation, but
more to this at a later time as it's not the point here.

This old patch I proposed was kind of hacky and I didn't receive a lot
of feedback, but I got some ideas to improve the code. I'm now in the
process of reworking my patch to be more maintainable.

Along the way I noticed some mismatches between the API as presented by
libsensors and the documentation in kernel. One example is the sysfs
entry "fan_max" which is mentioned in the kernel documentation, but is
not seen in the libsensors fan_matches subfeatures table. I think a
hwmon-core with all the sysfs handling is a great place to stabilize the
API between kernel and userspace and to do so I want to know who I
should trust, kernel doc or libsensors code?

Could we please sort this out whether the kernel doc is incorrect and
does not reflect actual interface or if sensors code needs some
additions to reflect API exposed by the kernel? I would really like to
help here and I'm willing to spend some time with stabilizing interface
and documentation, but as a newcomer it is hard to make a sense out of
all this conflicting information. I think a stable API both in and out
of kernel is beneficial to all hwmon drivers and systems interfacing
with them.

On a side note I could not find any API entries in libsensors for
handling trip points which are mentioned in the kernel doc, but I think
this is another issue as we really should handle trip point support as a
cooperation of hwmon and a thermal driver.

Best regards,
--Lucas


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] General API and documentation discussion

[Guenter Roeck]

On Tue, Sep 13, 2011 at 05:09:50PM -0400, Lucas Stach wrote:
> Hello all,
> 
> weeks ago I proposed a patch to introduce a new hwmon-core interface to
> have the sysfs handling centralised and getting a stable in-kernel API
> for the hwmon drivers. This should also lead to resolving the now
> unclear situation with regard to hwmon/thermal driver cooperation, but
> more to this at a later time as it's not the point here.
> 
> This old patch I proposed was kind of hacky and I didn't receive a lot
> of feedback, but I got some ideas to improve the code. I'm now in the
> process of reworking my patch to be more maintainable.
> 
I kind of recall that I wasn't sure about it ... it just "didn't look nice",
whatever that means. "kind of hacky" might be a good description.

> Along the way I noticed some mismatches between the API as presented by
> libsensors and the documentation in kernel. One example is the sysfs
> entry "fan_max" which is mentioned in the kernel documentation, but is
> not seen in the libsensors fan_matches subfeatures table. I think a
> hwmon-core with all the sysfs handling is a great place to stabilize the
> API between kernel and userspace and to do so I want to know who I
> should trust, kernel doc or libsensors code?
> 
My priorities would be

1) Documentation/hwmon/sysfs-interface
2) Code in drivers/hwmon

libsensors is just an application and not expected to implement everything,
even though it would be great if it did. But you can't argue that a system call
should go away just because glibc doesn't implement it.

If there are discrepancies, it would be good to have a table listing what
those are. sysfs-interface is not (necessarily) perfect, and there are lots
of non-standardized attributes used by various hwmon drivers. Also,
sysfs-interface is not cast in stone; just look at its history. If I catch
a non-standard attribute in a driver I am working on, and if I happen to have
some spare time, I look around and check if it is used by multiple drivers.
If it is, it may make sense to add it to the ABI, and update drivers to use
it if applicable. One example I remember is update_interval.

If an attribute is defined in sysfs-interface but not supported by libsensors,
and especially if it is supported by some drivers, I am sure Jean will be happy
to accept a patch for libsensors (if he finds the time to review it after digging
through all the patches I already sent to him ;). fanX_max is a really good
example here, since it is supported by several drivers.

Similar, if there happens to be an attribute supported by libsensors and by
some hwmon drivers which is _not_ defined in sysfs-interface, it would be useful
to know so we can address the issue (that would really be something that should
be fixed). On the other side, if there should be an attribute defined in
sysfs-interface but not implemented by any driver, it would be useful to know
to be able to decide if it should be kept or not.

In summary, again, nothing is cast in stone. First priority should be to do
what makes sense, not what is written in some specification.

> Could we please sort this out whether the kernel doc is incorrect and
> does not reflect actual interface or if sensors code needs some
> additions to reflect API exposed by the kernel? I would really like to
> help here and I'm willing to spend some time with stabilizing interface
> and documentation, but as a newcomer it is hard to make a sense out of
> all this conflicting information. I think a stable API both in and out
> of kernel is beneficial to all hwmon drivers and systems interfacing
> with them.
> 
> On a side note I could not find any API entries in libsensors for
> handling trip points which are mentioned in the kernel doc, but I think
> this is another issue as we really should handle trip point support as a
> cooperation of hwmon and a thermal driver.
> 
That is really a tricky and sensitive issue. So far the relationship is strictly
thermal -> hwmon, since hwmon by itself has a passive role. Fan handling with
its trip point settings are pretty much at the edge of things, and probably
acceptable because the actual _action_ associated with it is implemented in HW.
We have just started talking about if and how to handle interrupts, sysfs notifications,
and uevents.

One of the key question here is if the kernel should do anything besides notifying
applications. Seems to me the real action should be in userland, not in the kernel.
But then maybe I am missing some essential use cases, where the action would _have_
to be in the kernel. If so, we'll have to think really carefully about any to-be-defined
APIs.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] General API and documentation discussion

[Lucas Stach]

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

Hello Guenter,
Am Dienstag, den 13.09.2011, 22:33 -0700 schrieb Guenter Roeck:
[...]
> My priorities would be
> 
> 1) Documentation/hwmon/sysfs-interface
> 2) Code in drivers/hwmon
> 
> libsensors is just an application and not expected to implement everything,
> even though it would be great if it did. But you can't argue that a system call
> should go away just because glibc doesn't implement it.
> 
> If there are discrepancies, it would be good to have a table listing what
> those are. sysfs-interface is not (necessarily) perfect, and there are lots
> of non-standardized attributes used by various hwmon drivers. Also,
> sysfs-interface is not cast in stone; just look at its history. If I catch
> a non-standard attribute in a driver I am working on, and if I happen to have
> some spare time, I look around and check if it is used by multiple drivers.
> If it is, it may make sense to add it to the ABI, and update drivers to use
> it if applicable. One example I remember is update_interval.
> 
Ok, I did a bit digging and it seems only libsensors is missing some
sysfs attributes and not the other way around. I attached a list with a
side-by-side comparison (minus hardware trip point handling).

I think I will run with what is documented in
Documentation/hwmon/sysfs-interface for my first iteration of the core
interface, if something pops up in the process of porting drivers to the
new interface it can always be added later.

Only thing I noticed is "vid" and "vrm" are listed as voltage features,
though libsensors count them as an extra "cpu" feature. Should we change
the kernel doc accordingly, or should it stay under voltages?
[...]
> That is really a tricky and sensitive issue. So far the relationship is strictly
> thermal -> hwmon, since hwmon by itself has a passive role. Fan handling with
> its trip point settings are pretty much at the edge of things, and probably
> acceptable because the actual _action_ associated with it is implemented in HW.
> We have just started talking about if and how to handle interrupts, sysfs notifications,
> and uevents.
> 
I will leave this out of the core interface for the time being. I think
we should reiterate the issue, after doing a simple core interface.

> One of the key question here is if the kernel should do anything besides notifying
> applications. Seems to me the real action should be in userland, not in the kernel.
> But then maybe I am missing some essential use cases, where the action would _have_
> to be in the kernel. If so, we'll have to think really carefully about any to-be-defined
> APIs.
> 
I think many people want to have the thermal management in kernel, so
even if no userspace is present the right actions could be performed to
protect the hardware. This is the philosophy of thermal drivers, where
the userspace could specify which actions the kernel should perform in
case of rising temperature, like throttling the hardware or rising fan
speeds, but no actions are performed in userspace itself. So I think we
need some generic thermal driver to take action according to information
it gets from hwmon drivers, the core interface will be a great way to
allow such cooperation.

But this is only the third step after
1. specifying a basic core interface with split out of the sysfs
handling
2. knowing how to handle interrupts and hardware trip points and
extending the core interface to reflect such things

Would you be ok with this?
--Lucas

> Guenter
> 


[-- Attachment #2: hwmon-sysfs-vs-libsensors --]
[-- Type: text/plain, Size: 4089 bytes --]

sysfs kernel doc            |    libsensors code
                         general
name                        |
update_interval             |
                           cpu
vid (listed under voltages) |   vid
vrm	(listed under voltages) |
                         voltages
input                       |   input
min                         |   min
max                         |   max
lcrit                       |   lcrit
crit                        |   crit
average                     |
lowest                      |
highest                     |
reset_history               |
label                       |
alarm                       |   alarm
min_alarm                   |   min_alarm
max_alarm                   |   max_alarm
lcrit_alarm                 |   lcrit_alarm
crit_alarm                  |   crit_alarm
beep                        |   beep
                           fans
input                       |   input
min                         |   min
max                         |
div                         |   div
pulses                      |   pulses
target                      |
label                       |
(unclear)                   |   alarm
beep                        |   beep
fault                       |   fault
						   pwm
pwm                         |
enable                      |
mode                        |
freq                        |
auto_channels_tem           |
*various trip points*       |
                        temperatures
type                        |   type
input                       |   input
min                         |   min
max                         |   max
max_hyst                    |   max_hyst
lcrit                       |   lcrit
crit                        |   crit
crit_hyst                   |   crit_hyst
emergency                   |   emergency
emergency_hyst              |   emergency_hyst
offset                      |   offset
label                       |
lowest                      |
highest                     |
reset_history               |
alarm                       |   alarm
min_alarm                   |   min_alarm
max_alarm                   |   max_alarm
lcrit_alarm                 |   lcrit_alarm
crit_alarm                  |   crit_alarm
emergency_alarm             |   emergency_alarm
beep                        |   beep
fault                       |   fault
                        currents
input                       |   input
min                         |   min
max                         |   max
lcrit                       |   lcrit
crit                        |   crit
average                     |
lowest                      |
highest                     |
reset_history               |
alarm                       |   alarm
min_alarm                   |   min_alarm
max_alarm                   |   max_alarm
lcrit_alarm                 |   lcrit_alarm
crit_alarm                  |   crit_alarm
beep                        |   beep
                          power
input                       |   input
input_lowest                |   input_lowest
input_highest               |   input_highest
reset_history               |
accuracy                    |
average                     |   average
average_interval            |   average_interval
average_interval_min        |
average_interval_max        |
average_min                 |
average_max                 |
average_lowest              |   average_lowest
average_highest             |   average_highest
cap                         |   cap
cap_hyst                    |   cap_hyst
cap_min                     |
cap_max                     |
max                         |   max
crit                        |   crit
alarm                       |   alarm
cap_alarm                   |   cap_alarm
max_alarm                   |   max_alarm
crit_alarm                  |   crit_alarm
                          energy
input                       |   input
                         humidity
input                       |   input
                        intrusion
alarm                       |   alarm
beep                        |   beep

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] General API and documentation discussion

[Guenter Roeck]

Li Lucas,

On Thu, 2011-09-15 at 07:41 -0400, Lucas Stach wrote:
> Hello Guenter,
> Am Dienstag, den 13.09.2011, 22:33 -0700 schrieb Guenter Roeck:
> [...]
> > My priorities would be
> > 
> > 1) Documentation/hwmon/sysfs-interface
> > 2) Code in drivers/hwmon
> > 
> > libsensors is just an application and not expected to implement everything,
> > even though it would be great if it did. But you can't argue that a system call
> > should go away just because glibc doesn't implement it.
> > 
> > If there are discrepancies, it would be good to have a table listing what
> > those are. sysfs-interface is not (necessarily) perfect, and there are lots
> > of non-standardized attributes used by various hwmon drivers. Also,
> > sysfs-interface is not cast in stone; just look at its history. If I catch
> > a non-standard attribute in a driver I am working on, and if I happen to have
> > some spare time, I look around and check if it is used by multiple drivers.
> > If it is, it may make sense to add it to the ABI, and update drivers to use
> > it if applicable. One example I remember is update_interval.
> > 
> Ok, I did a bit digging and it seems only libsensors is missing some
> sysfs attributes and not the other way around. I attached a list with a
> side-by-side comparison (minus hardware trip point handling).
> 
> I think I will run with what is documented in
> Documentation/hwmon/sysfs-interface for my first iteration of the core
> interface, if something pops up in the process of porting drivers to the
> new interface it can always be added later.
> 
Have a look at this patch:

http://thread.gmane.org/gmane.linux.drivers.sensors/26880

It covers most of the missing attributes. Let's hope that Jean finds the
time to review it at some point.

> Only thing I noticed is "vid" and "vrm" are listed as voltage features,
> though libsensors count them as an extra "cpu" feature. Should we change
> the kernel doc accordingly, or should it stay under voltages?

Up to Jean to decide. VID and VRM are typically used by CPUs to select
voltages, but they _are_ voltage attributes and _may_ be used in other
contexts.

> [...]
> > That is really a tricky and sensitive issue. So far the relationship is strictly
> > thermal -> hwmon, since hwmon by itself has a passive role. Fan handling with
> > its trip point settings are pretty much at the edge of things, and probably
> > acceptable because the actual _action_ associated with it is implemented in HW.
> > We have just started talking about if and how to handle interrupts, sysfs notifications,
> > and uevents.
> > 
> I will leave this out of the core interface for the time being. I think
> we should reiterate the issue, after doing a simple core interface.
> 
> > One of the key question here is if the kernel should do anything besides notifying
> > applications. Seems to me the real action should be in userland, not in the kernel.
> > But then maybe I am missing some essential use cases, where the action would _have_
> > to be in the kernel. If so, we'll have to think really carefully about any to-be-defined
> > APIs.
> > 
> I think many people want to have the thermal management in kernel, so

Question is who is "many". There are many (for a given definition of
many, of course) others who believe that it should _not_ be handled in
the kernel. I am quite sure that Jean is one of the latter many. For
myself, I am not convinced that in-kernel handling provides sufficient
benefits to outweigh its disadvantages. There is really a lot of
philosophy involved here.

> even if no userspace is present the right actions could be performed to
> protect the hardware. This is the philosophy of thermal drivers, where
> the userspace could specify which actions the kernel should perform in
> case of rising temperature, like throttling the hardware or rising fan
> speeds, but no actions are performed in userspace itself. So I think we
> need some generic thermal driver to take action according to information
> it gets from hwmon drivers, the core interface will be a great way to
> allow such cooperation.
> 
> But this is only the third step after
> 1. specifying a basic core interface with split out of the sysfs
> handling
> 2. knowing how to handle interrupts and hardware trip points and
> extending the core interface to reflect such things
> 
> Would you be ok with this?

Sure, but most important is really how the code looks like ... anything
that reduces the total number of LOC while adding functionality is
almost per definition good, anything else has an uphill battle. Or, in
other words, KISS does apply (fortunately).

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors