Re: [lm-sensors] svn commit: r4959 -

Re: [lm-sensors] svn commit: r4959 -

[Jean Delvare]

Hi Hans,

>       Author: jwrdegoede
>         Date: Fri Oct 19 11:48:49 2007
> New Revision: 4959
>    Changeset: http://lm-sensors.org/changeset/4959
> 
> Modified:
>    lm-sensors/branches/lm-sensors-3.0.0/etc/sensors.conf.eg
> 
> Log:
> The new fschmd driver uses the standard scaling values from the datasheet, so no conversion should be done from sensors.conf

I am worried by this change, for two reasons. The first reason is that
the original comment was suggesting that the scaling resistors for the
Hermes were external and could change from one motherboard to the next,
while your new fschmd driver handles the scaling internally. If the
resistors are actually external, then your driver should not do the
scaling. Please clarify.

The second reason is that your change to sensors.conf.eg breaks the
original fscher driver. I understand that the two drivers are not
compatible in this respect (for now at least) so you can't have a
single default configuration file that works for both, but you really
should document the problem so that users have a chance to fix the
config file for use with the old fscher driver. In other words, instead
of deleting this section from the configuration file, you should have
commented it out and added a note that users of the old driver must
uncomment it again.

-- 
Jean Delvare

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

Re: [lm-sensors] svn commit: r4959 -

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
>>       Author: jwrdegoede
>>         Date: Fri Oct 19 11:48:49 2007
>> New Revision: 4959
>>    Changeset: http://lm-sensors.org/changeset/4959
>>
>> Modified:
>>    lm-sensors/branches/lm-sensors-3.0.0/etc/sensors.conf.eg
>>
>> Log:
>> The new fschmd driver uses the standard scaling values from the datasheet, so no conversion should be done from sensors.conf
> 
> I am worried by this change, for two reasons. The first reason is that
> the original comment was suggesting that the scaling resistors for the
> Hermes were external and could change from one motherboard to the next,
> while your new fschmd driver handles the scaling internally.

I don't know where the resistors are, as the "datasheet" which we have gives no 
details on this.

> If the
> resistors are actually external, then your driver should not do the
> scaling. Please clarify.
>

There are 3 reasons to do the scaling in the driver:
1) The used scale values are taken 1 on 1 from the fscher "datasheet"
2) They are all we have, the old driver directly exported register values
    which is not compliant with the sysfs interface
3) The new unified driver behaves identical for the fscpos and fscscy as the
    old drivers, the old fscher driver was the only fsc driver which didn't do
    scaling (no scaling at all, as said it directly exported register values
    which is not compliant with the sysfs interface)

> The second reason is that your change to sensors.conf.eg breaks the
> original fscher driver. I understand that the two drivers are not
> compatible in this respect (for now at least) so you can't have a
> single default configuration file that works for both, but you really
> should document the problem so that users have a chance to fix the
> config file for use with the old fscher driver. In other words, instead
> of deleting this section from the configuration file, you should have
> commented it out and added a note that users of the old driver must
> uncomment it again.
> 

Agreed, I'll put it back in commented for the 3.x.x branch, and a comment that 
the lines should be commented to the trunk sensors.conf

Regards,

Hans

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

Re: [lm-sensors] svn commit: r4959 -

[Jean Delvare]

Hi Hans,

On Wed, 24 Oct 2007 14:39:06 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > I am worried by this change, for two reasons. The first reason is that
> > the original comment was suggesting that the scaling resistors for the
> > Hermes were external and could change from one motherboard to the next,
> > while your new fschmd driver handles the scaling internally.
> 
> I don't know where the resistors are, as the "datasheet" which we have gives no 
> details on this.
> 
> > If the
> > resistors are actually external, then your driver should not do the
> > scaling. Please clarify.
> 
> There are 3 reasons to do the scaling in the driver:
> 1) The used scale values are taken 1 on 1 from the fscher "datasheet"

Do we really have the same datasheet? Mine states that the scaling
factor is computed from values present in the DMI table. Could you
please send me (in private) the output of dmidecode for your test
system? I'd like to take a look. The Poseidon datasheet OTOH indeed
lists hard-coded values.

What worries me a little is that your scaling actually differs from what
sensors.conf had for +12V for the fscher driver. You driver does *
14200 / 255, while the formula in sensors.conf does * 16170 / 255. This
suggests that different motherboards actually do need different scaling
factors. OTOH I admit I find it strange because the input measures the
same voltage (+12V) so I fail to see the point of using a slightly
different scaling factor from one board to the next, especially when
all boards are from the same manufacturer.

> 2) They are all we have, the old driver directly exported register values
>     which is not compliant with the sysfs interface

I agree that what the fscher driver was doing originally was probably
not correct: it would be surprising if the FSC chips' ADC had exactly a
10 mV resolution. OTOH, if we don't know the actual ADC resolution and
still want to do the scaling in user-space, that was about the only way.

> 3) The new unified driver behaves identical for the fscpos and fscscy as the
>     old drivers, the old fscher driver was the only fsc driver which didn't do
>     scaling (no scaling at all, as said it directly exported register values
>     which is not compliant with the sysfs interface)

The majority isn't always right ;)

The fscher wasn't exporting raw register values, rather it was
arbitrarily assuming a 10 mV ADC. But OK, that's about the same.

> > The second reason is that your change to sensors.conf.eg breaks the
> > original fscher driver. I understand that the two drivers are not
> > compatible in this respect (for now at least) so you can't have a
> > single default configuration file that works for both, but you really
> > should document the problem so that users have a chance to fix the
> > config file for use with the old fscher driver. In other words, instead
> > of deleting this section from the configuration file, you should have
> > commented it out and added a note that users of the old driver must
> > uncomment it again.
> 
> Agreed, I'll put it back in commented for the 3.x.x branch, and a comment that 
> the lines should be commented to the trunk sensors.conf

Great, thanks.

-- 
Jean Delvare

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

Re: [lm-sensors] svn commit: r4959 -

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Wed, 24 Oct 2007 14:39:06 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> I am worried by this change, for two reasons. The first reason is that
>>> the original comment was suggesting that the scaling resistors for the
>>> Hermes were external and could change from one motherboard to the next,
>>> while your new fschmd driver handles the scaling internally.
>> I don't know where the resistors are, as the "datasheet" which we have gives no 
>> details on this.
>>
>>> If the
>>> resistors are actually external, then your driver should not do the
>>> scaling. Please clarify.
>> There are 3 reasons to do the scaling in the driver:
>> 1) The used scale values are taken 1 on 1 from the fscher "datasheet"
> 
> Do we really have the same datasheet? Mine states that the scaling
> factor is computed from values present in the DMI table. Could you
> please send me (in private) the output of dmidecode for your test
> system? I'd like to take a look. The Poseidon datasheet OTOH indeed
> lists hard-coded values.
> 

Jean, you are amazing (in a positive way), you are completely right. I've 
totally overlooked this when comparing the datasheets for creating the unified 
drivers (oops).

I don't have access to my FSC (her) machine until monday, then I'll take a look 
at this. I'll also mail my FSC contact to ask if the DMI based scaling should 
be used for other models and for which models.


My plan is to implement DMI value based scaling for the relevant models for DMI 
enabled kernels, and fallback to the current constants for non DMI enabled kernels.

I feel a big discussion coming up here, like with the uguru3 on doing this in 
the kernel versus userspace, I hope we can come to an agreement a bit quicker / 
easier this time :)

Arguments to do it in the kernel:
1) With lm_sensors-3.x.x we've made userspace completely free of chip specific
    knowledge, I would like to keep it this way
2) The kernel already includes a DMI parser, so it is not going to add a whole
    lot of code
3) We could avoid 1) by creating an FSC specific userspace tool to generate a
    sensors.conf, but then this tool still needs to be run, so either we need to
    hack this into our initscripts somehow, or we need to document this and
    users must run it manualy. Having this in the driver will make things "just
    work (tm)".
4) Its device specific knowledge and device specific knowledge belongs in the
    driver.


Regards,

Hans

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

Re: [lm-sensors] svn commit: r4959 -

[Jean Delvare]

Hi Hans,

On Thu, 25 Oct 2007 12:01:59 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > On Wed, 24 Oct 2007 14:39:06 +0200, Hans de Goede wrote:
> >> There are 3 reasons to do the scaling in the driver:
> >> 1) The used scale values are taken 1 on 1 from the fscher "datasheet"
> > 
> > Do we really have the same datasheet? Mine states that the scaling
> > factor is computed from values present in the DMI table. Could you
> > please send me (in private) the output of dmidecode for your test
> > system? I'd like to take a look. The Poseidon datasheet OTOH indeed
> > lists hard-coded values.
> 
> Jean, you are amazing (in a positive way), you are completely right. I've 
> totally overlooked this when comparing the datasheets for creating the unified 
> drivers (oops).

Once in a while, there has to be some positive effects to me bugging
everyone all the time with silly questions and remarks ;)

> I don't have access to my FSC (her) machine until monday, then I'll take a look 
> at this. I'll also mail my FSC contact to ask if the DMI based scaling should 
> be used for other models and for which models.
> 
> 
> My plan is to implement DMI value based scaling for the relevant models for DMI 
> enabled kernels, and fallback to the current constants for non DMI enabled kernels.
> 
> I feel a big discussion coming up here, like with the uguru3 on doing this in 
> the kernel versus userspace, I hope we can come to an agreement a bit quicker / 
> easier this time :)
> 
> Arguments to do it in the kernel:
> 1) With lm_sensors-3.x.x we've made userspace completely free of chip specific
>     knowledge, I would like to keep it this way

Not exactly. The libsensors code and applications using libsensors are
free of chip-specific (or, for that matter, motherboard-specific)
knowledge. sensors.conf still holds motherboard-specific knowledge, and
there's nothing wrong with this.

> 2) The kernel already includes a DMI parser, so it is not going to add a whole
>     lot of code

I'm not sure what exactly is parsed in the kernel. It seems that it's
only extracting a couple identification strings and specific data and
doesn't give you access to arbitrary record data. So you'll probably
have to add your own FSC-specific handler, and to find a way to export
the data. Not sure if that will be accepted upstream.

> 3) We could avoid 1) by creating an FSC specific userspace tool to generate a
>     sensors.conf, but then this tool still needs to be run, so either we need to
>     hack this into our initscripts somehow, or we need to document this and
>     users must run it manualy. Having this in the driver will make things "just
>     work (tm)".

Most users will need to tweak their configuration file manually, at
least until we have our config file database in place. I don't think
it's a big issue if FSC chip users have to do the same as all other
users in this respect.

Writing a user-space tool on top of dmidecode to generate compute
statements automatically for FSC motherboards would probably not be
difficult. We'll soon depend on dmidecode anyway.

> 4) Its device specific knowledge and device specific knowledge belongs in the
>     driver.

It's actually motherboard-specific knowledge we're discussing here.

If you can hook up the DMI table parser in the kernel in a clean way,
that's completely fine with me. I'm just not sure if you'll be able to
do that. If you aren't, doing it in userspace still sounds doable.

As a side note, I would _love_ if other manufacturers could follow
FSC's track here and include voltage scaling information in their DMI
tables in a documented way. Even if each vendor has its own encoding
for that, it would still be very helpful when setting up the initial
sensors.conf file for a new motherboard.

-- 
Jean Delvare

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

Re: [lm-sensors] svn commit: r4959 -

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Thu, 25 Oct 2007 12:01:59 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> On Wed, 24 Oct 2007 14:39:06 +0200, Hans de Goede wrote:
>>>> There are 3 reasons to do the scaling in the driver:
>>>> 1) The used scale values are taken 1 on 1 from the fscher "datasheet"
>>> Do we really have the same datasheet? Mine states that the scaling
>>> factor is computed from values present in the DMI table. Could you
>>> please send me (in private) the output of dmidecode for your test
>>> system? I'd like to take a look. The Poseidon datasheet OTOH indeed
>>> lists hard-coded values.
>> Jean, you are amazing (in a positive way), you are completely right. I've 
>> totally overlooked this when comparing the datasheets for creating the unified 
>> drivers (oops).
> 
> Once in a while, there has to be some positive effects to me bugging
> everyone all the time with silly questions and remarks ;)
> 
>> I don't have access to my FSC (her) machine until monday, then I'll take a look 
>> at this. I'll also mail my FSC contact to ask if the DMI based scaling should 
>> be used for other models and for which models.
>>
>>
>> My plan is to implement DMI value based scaling for the relevant models for DMI 
>> enabled kernels, and fallback to the current constants for non DMI enabled kernels.
>>
>> I feel a big discussion coming up here, like with the uguru3 on doing this in 
>> the kernel versus userspace, I hope we can come to an agreement a bit quicker / 
>> easier this time :)
>>
>> Arguments to do it in the kernel:
>> 1) With lm_sensors-3.x.x we've made userspace completely free of chip specific
>>     knowledge, I would like to keep it this way
> 
> Not exactly. The libsensors code and applications using libsensors are
> free of chip-specific (or, for that matter, motherboard-specific)
> knowledge. sensors.conf still holds motherboard-specific knowledge, and
> there's nothing wrong with this.
> 
>> 2) The kernel already includes a DMI parser, so it is not going to add a whole
>>     lot of code
> 
> I'm not sure what exactly is parsed in the kernel. It seems that it's
> only extracting a couple identification strings and specific data and
> doesn't give you access to arbitrary record data. So you'll probably
> have to add your own FSC-specific handler, and to find a way to export
> the data. Not sure if that will be accepted upstream.
> 

You are right, so of to userspace we go.

This brings us to 2 questions:
1) What to use as conversion / divider in the driver? I see 2 options:
  a) Use the fscpos datasheet divider everywhere
  b) Use the fscpos datasheet divider for the fscpos and others which lack
     the DMI info, and use the old fscher divider / multiplier of 10 mv / step
     for those with DMI data.

  I must say I do not have much preference a) leads to a somewhat simpler
  driver, b) means that the old configs will stay working even when switching to
  the fschmd driver. So which one will it be>

2) How? I can write a specific utility to spit out the conversion formulas (I
  guess that is where I'll start). But at the end we may want to include this in
  sensors-detect. However currently sensors-detect does not touch sensors.conf,

  I guess its best to for now write a userspace utility which generates the
  conversion formulas for users, and integrate this into sensors-detect when we
  integrate dmi-based autoconfig into it.

Regards,

Hans

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

Re: [lm-sensors] svn commit: r4959 -

[Jean Delvare]

Hi Hans,

On Sun, 28 Oct 2007 14:16:11 +0100, Hans de Goede wrote:
> Jean Delvare wrote:
> > I'm not sure what exactly is parsed in the kernel. It seems that it's
> > only extracting a couple identification strings and specific data and
> > doesn't give you access to arbitrary record data. So you'll probably
> > have to add your own FSC-specific handler, and to find a way to export
> > the data. Not sure if that will be accepted upstream.
> 
> You are right, so of to userspace we go.
> 
> This brings us to 2 questions:
> 1) What to use as conversion / divider in the driver? I see 2 options:
>   a) Use the fscpos datasheet divider everywhere
>   b) Use the fscpos datasheet divider for the fscpos and others which lack
>      the DMI info, and use the old fscher divider / multiplier of 10 mv / step
>      for those with DMI data.
> 
>   I must say I do not have much preference a) leads to a somewhat simpler
>   driver, b) means that the old configs will stay working even when switching to
>   the fschmd driver. So which one will it be>

Option b) gets my favor. First reason is that it preserves compatibility
with the old drivers. Second reason is that you will otherwise have to
undo the scaling in user-space for the Hermes chip, before you apply
the one from the DMI table, which is a bit confusing.

One problem with this approach is that we need to know what to do for
the Heimdall and Heracles chips right now. Changing teh way we handle
them at a later point would only add to confusion.

I agree that it's not so nice. Really, too bad that you can't (easily)
get the DMI data from the kernel, that would have made things much
nicer. I'm wondering if it wouldn't be worth giving it a try anyway. If
dmi_scan_machine() was not __init and could take a record handler as a
parameter (as dmi_table does) then it would be possible to access and
process arbitrary DMI records from various kernel drivers even after
boot. I'm curious if other drivers would benefit from that or if your
fschmd driver is an isolated case.

> 2) How? I can write a specific utility to spit out the conversion formulas (I
>   guess that is where I'll start). But at the end we may want to include this in
>   sensors-detect. However currently sensors-detect does not touch sensors.conf,
> 
>   I guess its best to for now write a userspace utility which generates the
>   conversion formulas for users, and integrate this into sensors-detect when we
>   integrate dmi-based autoconfig into it.

Agreed. For now you can simply add a note in the default sensors.conf
file, pointing the user to your user-space tool for chips which need it.

-- 
Jean Delvare

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