[lm-sensors] powerX_alarm sysfs attribute

[lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

Hi all,

I am looking through libsensors and the hwmon sysfs ABI to identify and fix
inconsistencies.

One problem I noticed is powerX_alarm, which is defined as "system is drawing
more power than the cap allows".

powerX_cap is defined as " ... The *_cap files only appear if the cap is known
to be enforced by hardware".

Now there are conditions where power limits are defined and supported,
but the hardware does not enforce it. Similar, there are devices reporting power
alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
and the alarm is not associated with a maximum, thus a reported alarm doesn't
really reflect the ABI.

I see a number of options:

1) Introduce powerX_max as unenforced "max" attribute, in addition to powerX_cap.
   1a) Introduce powerX_cap_alarm to reflect alarms associated with powerX_cap,
       introduce powerX_max_alarm, and redefine the semantics of powerX_alarm 
       to include all possible alarms
   1b) Redefine semantics of powerX_alarm to include all possible reasons for
       power alarms.

2) Redefine the semantics of powerX_cap to include unenforced maximum power.
   Redefine powerX_alarm to include all possible reasons for alarms.

My preferred solution would be 1) with 1b), though 2) would be fine for me as well.
Any thoughts / comments ? 

Thanks,
Guenter

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Ira W. Snyder]

On Thu, Dec 09, 2010 at 08:58:58AM -0800, Guenter Roeck wrote:
> Hi all,
> 
> I am looking through libsensors and the hwmon sysfs ABI to identify and fix
> inconsistencies.
> 
> One problem I noticed is powerX_alarm, which is defined as "system is drawing
> more power than the cap allows".
> 
> powerX_cap is defined as " ... The *_cap files only appear if the cap is known
> to be enforced by hardware".
> 
> Now there are conditions where power limits are defined and supported,
> but the hardware does not enforce it. Similar, there are devices reporting power
> alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
> powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
> and the alarm is not associated with a maximum, thus a reported alarm doesn't
> really reflect the ABI.
> 

In the ltc4215, the power1_alarm occurs when the output voltage of the
chip is outside a certain range. This range is specified by external
resistors, specific to each application. They are not required to be a
specific value by the hardware.

I guess that the ltc4215 driver's use of powerX_alarm doesn't follow the
ABI document.

In essence, the power1_alarm is connected to the chip's power good
output (negated). Should the ABI have a powerX_good or powerX_fail
attribute?

Ira

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Thu, 2010-12-09 at 16:26 -0500, Ira W. Snyder wrote:
> On Thu, Dec 09, 2010 at 08:58:58AM -0800, Guenter Roeck wrote:
> > Hi all,
> > 
> > I am looking through libsensors and the hwmon sysfs ABI to identify and fix
> > inconsistencies.
> > 
> > One problem I noticed is powerX_alarm, which is defined as "system is drawing
> > more power than the cap allows".
> > 
> > powerX_cap is defined as " ... The *_cap files only appear if the cap is known
> > to be enforced by hardware".
> > 
> > Now there are conditions where power limits are defined and supported,
> > but the hardware does not enforce it. Similar, there are devices reporting power
> > alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
> > powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
> > and the alarm is not associated with a maximum, thus a reported alarm doesn't
> > really reflect the ABI.
> > 
> 
> In the ltc4215, the power1_alarm occurs when the output voltage of the
> chip is outside a certain range. This range is specified by external
> resistors, specific to each application. They are not required to be a
> specific value by the hardware.
> 
> I guess that the ltc4215 driver's use of powerX_alarm doesn't follow the
> ABI document.
> 
> In essence, the power1_alarm is connected to the chip's power good
> output (negated). Should the ABI have a powerX_good or powerX_fail
> attribute?
> 
I think powerX_alarm pretty well covers it. My concern is with the ABI
description/definition, which is not in line with other _alarm
attributes.

Question for the ltc4215 driver, though, is if power1_alarm is
appropriate in the first place. After reading the datasheet, I noticed
that it does not really report a power problem, but "output voltage
low". So I wonder it the attribute in the driver should be "in2_alarm"
or possibly "in2_min_alarm" instead of power1_alarm, and if the power
attributes should be dropped entirely.

We could possibly add an inX_fail alarm attribute to cover the condition
reported by the ltc4215. Not sure if that would be worth it, but it
might be worth a thought.

Thanks,
Guenter



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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Ira W. Snyder]

On Thu, Dec 09, 2010 at 01:48:33PM -0800, Guenter Roeck wrote:
> On Thu, 2010-12-09 at 16:26 -0500, Ira W. Snyder wrote:
> > On Thu, Dec 09, 2010 at 08:58:58AM -0800, Guenter Roeck wrote:
> > > Hi all,
> > > 
> > > I am looking through libsensors and the hwmon sysfs ABI to identify and fix
> > > inconsistencies.
> > > 
> > > One problem I noticed is powerX_alarm, which is defined as "system is drawing
> > > more power than the cap allows".
> > > 
> > > powerX_cap is defined as " ... The *_cap files only appear if the cap is known
> > > to be enforced by hardware".
> > > 
> > > Now there are conditions where power limits are defined and supported,
> > > but the hardware does not enforce it. Similar, there are devices reporting power
> > > alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
> > > powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
> > > and the alarm is not associated with a maximum, thus a reported alarm doesn't
> > > really reflect the ABI.
> > > 
> > 
> > In the ltc4215, the power1_alarm occurs when the output voltage of the
> > chip is outside a certain range. This range is specified by external
> > resistors, specific to each application. They are not required to be a
> > specific value by the hardware.
> > 
> > I guess that the ltc4215 driver's use of powerX_alarm doesn't follow the
> > ABI document.
> > 
> > In essence, the power1_alarm is connected to the chip's power good
> > output (negated). Should the ABI have a powerX_good or powerX_fail
> > attribute?
> > 
> I think powerX_alarm pretty well covers it. My concern is with the ABI
> description/definition, which is not in line with other _alarm
> attributes.
> 
> Question for the ltc4215 driver, though, is if power1_alarm is
> appropriate in the first place. After reading the datasheet, I noticed
> that it does not really report a power problem, but "output voltage
> low". So I wonder it the attribute in the driver should be "in2_alarm"
> or possibly "in2_min_alarm" instead of power1_alarm, and if the power
> attributes should be dropped entirely.
> 

Reviewers (probably Jean) suggested the power1_input and power1_alarm
files when I submitted the driver. (I'm not placing blame, just
explaining where they came from.)

In both the ltc4215 and ltc4245 drivers, the power outputs are
calculated purely in software. This is very convenient for users of the
sensors utility.

I would like to keep the power1_input sysfs file. I do not have any
objections to changing power1_alarm to in2_alarm or in2_min_alarm.

Ira

> We could possibly add an inX_fail alarm attribute to cover the condition
> reported by the ltc4215. Not sure if that would be worth it, but it
> might be worth a thought.
> 
> Thanks,
> Guenter
> 
> 

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Jean Delvare]

Hi Ira, Guenter,

On Thu, 9 Dec 2010 16:07:08 -0800, Ira W. Snyder wrote:
> On Thu, Dec 09, 2010 at 01:48:33PM -0800, Guenter Roeck wrote:
> > Question for the ltc4215 driver, though, is if power1_alarm is
> > appropriate in the first place. After reading the datasheet, I noticed
> > that it does not really report a power problem, but "output voltage
> > low". So I wonder it the attribute in the driver should be "in2_alarm"
> > or possibly "in2_min_alarm" instead of power1_alarm, and if the power
> > attributes should be dropped entirely.
> 
> Reviewers (probably Jean) suggested the power1_input and power1_alarm
> files when I submitted the driver. (I'm not placing blame, just
> explaining where they came from.)

Probably not, the ltc4215 driver went upstream through Andrew Morton
because I lacked the time to do a proper review.

> In both the ltc4215 and ltc4245 drivers, the power outputs are
> calculated purely in software. This is very convenient for users of the
> sensors utility.
> 
> I would like to keep the power1_input sysfs file.

I have no problem with this, at least as long as libsensors doesn't
offer a way to bind current sensors to voltage sensors.

> I do not have any
> objections to changing power1_alarm to in2_alarm or in2_min_alarm.

in2_* doesn't seem right for a voltage output alarm. I'd say such a
feature doesn't belong to the hwmon ABI in the first place.

-- 
Jean Delvare

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Jean Delvare]

On Thu, 9 Dec 2010 13:26:13 -0800, Ira W. Snyder wrote:
> On Thu, Dec 09, 2010 at 08:58:58AM -0800, Guenter Roeck wrote:
> > Hi all,
> > 
> > I am looking through libsensors and the hwmon sysfs ABI to identify and fix
> > inconsistencies.
> > 
> > One problem I noticed is powerX_alarm, which is defined as "system is drawing
> > more power than the cap allows".
> > 
> > powerX_cap is defined as " ... The *_cap files only appear if the cap is known
> > to be enforced by hardware".
> > 
> > Now there are conditions where power limits are defined and supported,
> > but the hardware does not enforce it. Similar, there are devices reporting power
> > alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
> > powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
> > and the alarm is not associated with a maximum, thus a reported alarm doesn't
> > really reflect the ABI.
> 
> In the ltc4215, the power1_alarm occurs when the output voltage of the
> chip is outside a certain range. This range is specified by external
> resistors, specific to each application. They are not required to be a
> specific value by the hardware.

I am confused. Did you just write _output_ voltage? 

> I guess that the ltc4215 driver's use of powerX_alarm doesn't follow the
> ABI document.

I can confirm that. For one thing, if this has to do with voltage, it's
not a power limit. For another, if it is a voltage _output_ limit, it
has nothing to do with hardware monitoring.

> In essence, the power1_alarm is connected to the chip's power good
> output (negated). Should the ABI have a powerX_good or powerX_fail
> attribute?

-- 
Jean Delvare

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Fri, Dec 10, 2010 at 09:36:46AM -0500, Jean Delvare wrote:
> Hi Ira, Guenter,
> 
> On Thu, 9 Dec 2010 16:07:08 -0800, Ira W. Snyder wrote:
> > On Thu, Dec 09, 2010 at 01:48:33PM -0800, Guenter Roeck wrote:
> > > Question for the ltc4215 driver, though, is if power1_alarm is
> > > appropriate in the first place. After reading the datasheet, I noticed
> > > that it does not really report a power problem, but "output voltage
> > > low". So I wonder it the attribute in the driver should be "in2_alarm"
> > > or possibly "in2_min_alarm" instead of power1_alarm, and if the power
> > > attributes should be dropped entirely.
> > 
> > Reviewers (probably Jean) suggested the power1_input and power1_alarm
> > files when I submitted the driver. (I'm not placing blame, just
> > explaining where they came from.)
> 
> Probably not, the ltc4215 driver went upstream through Andrew Morton
> because I lacked the time to do a proper review.
> 
> > In both the ltc4215 and ltc4245 drivers, the power outputs are
> > calculated purely in software. This is very convenient for users of the
> > sensors utility.
> > 
> > I would like to keep the power1_input sysfs file.
> 
> I have no problem with this, at least as long as libsensors doesn't
> offer a way to bind current sensors to voltage sensors.
> 
> > I do not have any
> > objections to changing power1_alarm to in2_alarm or in2_min_alarm.
> 
> in2_* doesn't seem right for a voltage output alarm. I'd say such a
> feature doesn't belong to the hwmon ABI in the first place.
> 
Why not ? You lost me there. It optionally monitors the voltage it controls,
and provides the monitored value(s) to the user. Many of the other recent chips
do the same.  ltc4215, ltc4245, ltc4261, smm655, and pretty much all PMBus devices.
The ltc42xx devices act as voltage switch (on/off) and don't otherwise affect
the controlled voltage.
For ltc4215, in2_input is still connected to a a sensor input (AD2IN or so), as is
the FB pin (which causes the alarm if the voltage connected to it drops below a
certain level). It doesn't make sense to state that the monitored voltage can not
be reported through hwmon just because it may be a voltage it controls.

It gets even more tricky with devices which can be used as voltage controller
or voltage monitor or both. For example, the smm665 and ltc2978 are both a power
supply monitor and controller. I have seen them used for both purposes. But there isn't
a reason to deny hwmon support for such devices just because they also may be used for
voltage control.

Another question (for which I still don't have a clear answer) is if voltage controllers/monitors
such as the smm665 and at least some pmbus devices should also have a matching power_supply
component, and thus if they should be considered mfd devices with hwmon and power_supply
components. I have that on my backburner list; I'll probably get there if someone
actually wants to be able to set a voltage on one of the devices.

Guenter


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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Fri, Dec 10, 2010 at 09:37:59AM -0500, Jean Delvare wrote:
> On Thu, 9 Dec 2010 13:26:13 -0800, Ira W. Snyder wrote:
> > On Thu, Dec 09, 2010 at 08:58:58AM -0800, Guenter Roeck wrote:
> > > Hi all,
> > > 
> > > I am looking through libsensors and the hwmon sysfs ABI to identify and fix
> > > inconsistencies.
> > > 
> > > One problem I noticed is powerX_alarm, which is defined as "system is drawing
> > > more power than the cap allows".
> > > 
> > > powerX_cap is defined as " ... The *_cap files only appear if the cap is known
> > > to be enforced by hardware".
> > > 
> > > Now there are conditions where power limits are defined and supported,
> > > but the hardware does not enforce it. Similar, there are devices reporting power
> > > alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
> > > powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
> > > and the alarm is not associated with a maximum, thus a reported alarm doesn't
> > > really reflect the ABI.
> > 
> > In the ltc4215, the power1_alarm occurs when the output voltage of the
> > chip is outside a certain range. This range is specified by external
> > resistors, specific to each application. They are not required to be a
> > specific value by the hardware.
> 
> I am confused. Did you just write _output_ voltage? 
> 
output is probably an inexact statement here. The chip controls the voltage
to a board using an external transistor, and it can monitor the output voltage 
of that transistor.

> > I guess that the ltc4215 driver's use of powerX_alarm doesn't follow the
> > ABI document.
> 
> I can confirm that. For one thing, if this has to do with voltage, it's
> not a power limit. For another, if it is a voltage _output_ limit, it
> has nothing to do with hardware monitoring.
> 
I disagree; please also see my other reply.

Thanks,
Guenter

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Jean Delvare]

On Fri, 10 Dec 2010 07:12:09 -0800, Guenter Roeck wrote:
> On Fri, Dec 10, 2010 at 09:36:46AM -0500, Jean Delvare wrote:
> > On Thu, 9 Dec 2010 16:07:08 -0800, Ira W. Snyder wrote:
> > > I would like to keep the power1_input sysfs file.
> > 
> > I have no problem with this, at least as long as libsensors doesn't
> > offer a way to bind current sensors to voltage sensors.
> > 
> > > I do not have any
> > > objections to changing power1_alarm to in2_alarm or in2_min_alarm.
> > 
> > in2_* doesn't seem right for a voltage output alarm. I'd say such a
> > feature doesn't belong to the hwmon ABI in the first place.
> 
> Why not ? You lost me there. It optionally monitors the voltage it controls,
> and provides the monitored value(s) to the user. Many of the other recent chips
> do the same.  ltc4215, ltc4245, ltc4261, smm655, and pretty much all PMBus devices.
> The ltc42xx devices act as voltage switch (on/off) and don't otherwise affect
> the controlled voltage.
> For ltc4215, in2_input is still connected to a a sensor input (AD2IN or so), as is
> the FB pin (which causes the alarm if the voltage connected to it drops below a
> certain level). It doesn't make sense to state that the monitored voltage can not
> be reported through hwmon just because it may be a voltage it controls.

Thanks for clarifying. I didn't read the datasheet, and what Ira wrote
confused me. If the alarm is related to the value of in2_input, then of
course in2_alarm, in2_min_alarm (assuming in2_min is present) or
in2_max_alarm (assuming in2_max is present) makes sense.

BTW, powerX_alarm makes no sense at all if powerX_input is computed, as
is the case for the ltc4215 according to Ira.

-- 
Jean Delvare

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Fri, Dec 10, 2010 at 11:00:23AM -0500, Jean Delvare wrote:
> On Fri, 10 Dec 2010 07:12:09 -0800, Guenter Roeck wrote:
> > On Fri, Dec 10, 2010 at 09:36:46AM -0500, Jean Delvare wrote:
> > > On Thu, 9 Dec 2010 16:07:08 -0800, Ira W. Snyder wrote:
> > > > I would like to keep the power1_input sysfs file.
> > > 
> > > I have no problem with this, at least as long as libsensors doesn't
> > > offer a way to bind current sensors to voltage sensors.
> > > 
> > > > I do not have any
> > > > objections to changing power1_alarm to in2_alarm or in2_min_alarm.
> > > 
> > > in2_* doesn't seem right for a voltage output alarm. I'd say such a
> > > feature doesn't belong to the hwmon ABI in the first place.
> > 
> > Why not ? You lost me there. It optionally monitors the voltage it controls,
> > and provides the monitored value(s) to the user. Many of the other recent chips
> > do the same.  ltc4215, ltc4245, ltc4261, smm655, and pretty much all PMBus devices.
> > The ltc42xx devices act as voltage switch (on/off) and don't otherwise affect
> > the controlled voltage.
> > For ltc4215, in2_input is still connected to a a sensor input (AD2IN or so), as is
> > the FB pin (which causes the alarm if the voltage connected to it drops below a
> > certain level). It doesn't make sense to state that the monitored voltage can not
> > be reported through hwmon just because it may be a voltage it controls.
> 
> Thanks for clarifying. I didn't read the datasheet, and what Ira wrote
> confused me. If the alarm is related to the value of in2_input, then of
> course in2_alarm, in2_min_alarm (assuming in2_min is present) or
> in2_max_alarm (assuming in2_max is present) makes sense.
> 
Is that an implied requirement for supporting min_alarm ? Reason for asking is that many chips 
have limit alarms but no matching (readable/settable) limit values. ltc4215, ltc4245, and ltc4261
are examples. Alarm status is determined by a voltage on a pin exceeding a hardcoded limit,
and the actual voltage on a pin is determined by external resistor arrays.
The ltc4245 and ltc4261 drivers already provide min_alarm and/or max_alarm but not min and max.

> BTW, powerX_alarm makes no sense at all if powerX_input is computed, as
> is the case for the ltc4215 according to Ira.
> 
Agreed. It needs to be changed to in2_alarm or in2_min_alarm.

Guenter

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Jean Delvare]

On Fri, 10 Dec 2010 08:18:30 -0800, Guenter Roeck wrote:
> On Fri, Dec 10, 2010 at 11:00:23AM -0500, Jean Delvare wrote:
> > Thanks for clarifying. I didn't read the datasheet, and what Ira wrote
> > confused me. If the alarm is related to the value of in2_input, then of
> > course in2_alarm, in2_min_alarm (assuming in2_min is present) or
> > in2_max_alarm (assuming in2_max is present) makes sense.
>
> Is that an implied requirement for supporting min_alarm ? Reason for asking is that many chips 
> have limit alarms but no matching (readable/settable) limit values. ltc4215, ltc4245, and ltc4261
> are examples. Alarm status is determined by a voltage on a pin exceeding a hardcoded limit,
> and the actual voltage on a pin is determined by external resistor arrays.
> The ltc4245 and ltc4261 drivers already provide min_alarm and/or max_alarm but not min and max.

I had our recent discussion about the "sensors" code in mind. I seem to
recall that the code currently ignores limit-specific alarms if there
is no matching limit.

That being said, if some chip drivers really have to do that, then
probably we'll have to adjust the "sensors" code, rather than the other
way around.

-- 
Jean Delvare

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Fri, Dec 10, 2010 at 11:30:25AM -0500, Jean Delvare wrote:
> On Fri, 10 Dec 2010 08:18:30 -0800, Guenter Roeck wrote:
> > On Fri, Dec 10, 2010 at 11:00:23AM -0500, Jean Delvare wrote:
> > > Thanks for clarifying. I didn't read the datasheet, and what Ira wrote
> > > confused me. If the alarm is related to the value of in2_input, then of
> > > course in2_alarm, in2_min_alarm (assuming in2_min is present) or
> > > in2_max_alarm (assuming in2_max is present) makes sense.
> >
> > Is that an implied requirement for supporting min_alarm ? Reason for asking is that many chips 
> > have limit alarms but no matching (readable/settable) limit values. ltc4215, ltc4245, and ltc4261
> > are examples. Alarm status is determined by a voltage on a pin exceeding a hardcoded limit,
> > and the actual voltage on a pin is determined by external resistor arrays.
> > The ltc4245 and ltc4261 drivers already provide min_alarm and/or max_alarm but not min and max.
> 
> I had our recent discussion about the "sensors" code in mind. I seem to
> recall that the code currently ignores limit-specific alarms if there
> is no matching limit.
> 
> That being said, if some chip drivers really have to do that, then
> probably we'll have to adjust the "sensors" code, rather than the other
> way around.
> 
Makes sense. I'll look into it.

Guenter

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Fri, 2010-12-10 at 11:18 -0500, Guenter Roeck wrote:
> On Fri, Dec 10, 2010 at 11:00:23AM -0500, Jean Delvare wrote:
> > On Fri, 10 Dec 2010 07:12:09 -0800, Guenter Roeck wrote:
> > > On Fri, Dec 10, 2010 at 09:36:46AM -0500, Jean Delvare wrote:
> > > > On Thu, 9 Dec 2010 16:07:08 -0800, Ira W. Snyder wrote:
> > > > > I would like to keep the power1_input sysfs file.
> > > > 
> > > > I have no problem with this, at least as long as libsensors doesn't
> > > > offer a way to bind current sensors to voltage sensors.
> > > > 
> > > > > I do not have any
> > > > > objections to changing power1_alarm to in2_alarm or in2_min_alarm.
> > > > 
> > > > in2_* doesn't seem right for a voltage output alarm. I'd say such a
> > > > feature doesn't belong to the hwmon ABI in the first place.
> > > 
> > > Why not ? You lost me there. It optionally monitors the voltage it controls,
> > > and provides the monitored value(s) to the user. Many of the other recent chips
> > > do the same.  ltc4215, ltc4245, ltc4261, smm655, and pretty much all PMBus devices.
> > > The ltc42xx devices act as voltage switch (on/off) and don't otherwise affect
> > > the controlled voltage.
> > > For ltc4215, in2_input is still connected to a a sensor input (AD2IN or so), as is
> > > the FB pin (which causes the alarm if the voltage connected to it drops below a
> > > certain level). It doesn't make sense to state that the monitored voltage can not
> > > be reported through hwmon just because it may be a voltage it controls.
> > 
> > Thanks for clarifying. I didn't read the datasheet, and what Ira wrote
> > confused me. If the alarm is related to the value of in2_input, then of
> > course in2_alarm, in2_min_alarm (assuming in2_min is present) or
> > in2_max_alarm (assuming in2_max is present) makes sense.
> > 
> Is that an implied requirement for supporting min_alarm ? Reason for asking is that many chips 
> have limit alarms but no matching (readable/settable) limit values. ltc4215, ltc4245, and ltc4261
> are examples. Alarm status is determined by a voltage on a pin exceeding a hardcoded limit,
> and the actual voltage on a pin is determined by external resistor arrays.
> The ltc4245 and ltc4261 drivers already provide min_alarm and/or max_alarm but not min and max.
> 
> > BTW, powerX_alarm makes no sense at all if powerX_input is computed, as
> > is the case for the ltc4215 according to Ira.
> > 
> Agreed. It needs to be changed to in2_alarm or in2_min_alarm.

Hi Ira,

can you make that change, or do you want me to do it ?

Thanks,
Guenter



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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Jean Delvare]

Hi Guenter,

On Thu, 9 Dec 2010 08:58:58 -0800, Guenter Roeck wrote:
> I am looking through libsensors and the hwmon sysfs ABI to identify and fix
> inconsistencies.
> 
> One problem I noticed is powerX_alarm, which is defined as "system is drawing
> more power than the cap allows".
> 
> powerX_cap is defined as " ... The *_cap files only appear if the cap is known
> to be enforced by hardware".

This is almost contradictory. If the cap is enforced, then it can't be
exceeded, which means the alarm will never trigger.m

> Now there are conditions where power limits are defined and supported,
> but the hardware does not enforce it. Similar, there are devices reporting power
> alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
> powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
> and the alarm is not associated with a maximum, thus a reported alarm doesn't
> really reflect the ABI.
> 
> I see a number of options:
> 
> 1) Introduce powerX_max as unenforced "max" attribute, in addition to powerX_cap.
>    1a) Introduce powerX_cap_alarm to reflect alarms associated with powerX_cap,
>        introduce powerX_max_alarm, and redefine the semantics of powerX_alarm 
>        to include all possible alarms
>    1b) Redefine semantics of powerX_alarm to include all possible reasons for
>        power alarms.
> 
> 2) Redefine the semantics of powerX_cap to include unenforced maximum power.
>    Redefine powerX_alarm to include all possible reasons for alarms.
> 
> My preferred solution would be 1) with 1b), though 2) would be fine for me as well.
> Any thoughts / comments ? 

1) (both a and b) is fine with me. 2) not so, as the _cap name gets
confusing then.

I don't think that _cap should have existed in the first place. Just
because something happens when a limit is exceeded is no good reason to
rename that limit to something else. But now that it's there, I guess
it will be difficult to get rid of it.

-- 
Jean Delvare

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Sun, Dec 12, 2010 at 12:10:16PM -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Thu, 9 Dec 2010 08:58:58 -0800, Guenter Roeck wrote:
> > I am looking through libsensors and the hwmon sysfs ABI to identify and fix
> > inconsistencies.
> > 
> > One problem I noticed is powerX_alarm, which is defined as "system is drawing
> > more power than the cap allows".
> > 
> > powerX_cap is defined as " ... The *_cap files only appear if the cap is known
> > to be enforced by hardware".
> 
> This is almost contradictory. If the cap is enforced, then it can't be
> exceeded, which means the alarm will never trigger.m
> 
Looks like it. I think we should rephrase it, maybe to something like
"system limits power output to enforce powerX_cap".

> > Now there are conditions where power limits are defined and supported,
> > but the hardware does not enforce it. Similar, there are devices reporting power
> > alarms not associated with cap enforcement. Examples are ltc4215 and PMBus devices.
> > powerX_alarm is supported by the ltc4215 driver, but there is no _cap attribute,
> > and the alarm is not associated with a maximum, thus a reported alarm doesn't
> > really reflect the ABI.
> > 
> > I see a number of options:
> > 
> > 1) Introduce powerX_max as unenforced "max" attribute, in addition to powerX_cap.
> >    1a) Introduce powerX_cap_alarm to reflect alarms associated with powerX_cap,
> >        introduce powerX_max_alarm, and redefine the semantics of powerX_alarm 
> >        to include all possible alarms
> >    1b) Redefine semantics of powerX_alarm to include all possible reasons for
> >        power alarms.
> > 
> > 2) Redefine the semantics of powerX_cap to include unenforced maximum power.
> >    Redefine powerX_alarm to include all possible reasons for alarms.
> > 
> > My preferred solution would be 1) with 1b), though 2) would be fine for me as well.
> > Any thoughts / comments ? 
> 
> 1) (both a and b) is fine with me. 2) not so, as the _cap name gets
> confusing then.
> 
> I don't think that _cap should have existed in the first place. Just
> because something happens when a limit is exceeded is no good reason to
> rename that limit to something else. But now that it's there, I guess
> it will be difficult to get rid of it.
> 
The functionality is supported by some devices, though. The PMBus spec has a register
to set the power limit, and PMBus devices supporting it are expected to limit power
output if the cap is reached. This is in addition to a separate register, which sets the 
power max alarm limit (and creates an alarm if that limit is exceeded). So I'll
have the need for both _cap and _max.

The RFC patch I sent out earlier uses approach 1b), though 1a) would probably be
more consistent with other alarms. So maybe I should update the patch to use 1a) instead.

Guenter

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Jean Delvare]

On Sun, 12 Dec 2010 11:49:40 -0800, Guenter Roeck wrote:
> On Sun, Dec 12, 2010 at 12:10:16PM -0500, Jean Delvare wrote:
> > On Thu, 9 Dec 2010 08:58:58 -0800, Guenter Roeck wrote:
> > > I see a number of options:
> > > 
> > > 1) Introduce powerX_max as unenforced "max" attribute, in addition to powerX_cap.
> > >    1a) Introduce powerX_cap_alarm to reflect alarms associated with powerX_cap,
> > >        introduce powerX_max_alarm, and redefine the semantics of powerX_alarm 
> > >        to include all possible alarms
> > >    1b) Redefine semantics of powerX_alarm to include all possible reasons for
> > >        power alarms.
> > > 
> > > 2) Redefine the semantics of powerX_cap to include unenforced maximum power.
> > >    Redefine powerX_alarm to include all possible reasons for alarms.
> > > 
> > > My preferred solution would be 1) with 1b), though 2) would be fine for me as well.
> > > Any thoughts / comments ? 
> > 
> > 1) (both a and b) is fine with me. 2) not so, as the _cap name gets
> > confusing then.
> > 
> > I don't think that _cap should have existed in the first place. Just
> > because something happens when a limit is exceeded is no good reason to
> > rename that limit to something else. But now that it's there, I guess
> > it will be difficult to get rid of it.
> > 
> The functionality is supported by some devices, though. The PMBus spec has a register
> to set the power limit, and PMBus devices supporting it are expected to limit power
> output if the cap is reached. This is in addition to a separate register, which sets the 
> power max alarm limit (and creates an alarm if that limit is exceeded). So I'll
> have the need for both _cap and _max.

Which basically means that _cap should have been named _crit. For
example, it is fairly common for temperature sensors to take action
(e.g. speeding up fans or throttling the CPU) when the critical limit
is exceeded. But again, now that it's there, I guess we have to live
with it :(

> The RFC patch I sent out earlier uses approach 1b), though 1a) would probably be
> more consistent with other alarms. So maybe I should update the patch to use 1a) instead.

As I read it, 1b) is a subset of 1a). 1b) is enough to fix the current
wording, and 1a) can be done if some chips need it.

-- 
Jean Delvare

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Sun, Dec 12, 2010 at 03:17:20PM -0500, Jean Delvare wrote:
> On Sun, 12 Dec 2010 11:49:40 -0800, Guenter Roeck wrote:
> > On Sun, Dec 12, 2010 at 12:10:16PM -0500, Jean Delvare wrote:
> > > On Thu, 9 Dec 2010 08:58:58 -0800, Guenter Roeck wrote:
> > > > I see a number of options:
> > > > 
> > > > 1) Introduce powerX_max as unenforced "max" attribute, in addition to powerX_cap.
> > > >    1a) Introduce powerX_cap_alarm to reflect alarms associated with powerX_cap,
> > > >        introduce powerX_max_alarm, and redefine the semantics of powerX_alarm 
> > > >        to include all possible alarms
> > > >    1b) Redefine semantics of powerX_alarm to include all possible reasons for
> > > >        power alarms.
> > > > 
> > > > 2) Redefine the semantics of powerX_cap to include unenforced maximum power.
> > > >    Redefine powerX_alarm to include all possible reasons for alarms.
> > > > 
> > > > My preferred solution would be 1) with 1b), though 2) would be fine for me as well.
> > > > Any thoughts / comments ? 
> > > 
> > > 1) (both a and b) is fine with me. 2) not so, as the _cap name gets
> > > confusing then.
> > > 
> > > I don't think that _cap should have existed in the first place. Just
> > > because something happens when a limit is exceeded is no good reason to
> > > rename that limit to something else. But now that it's there, I guess
> > > it will be difficult to get rid of it.
> > > 
> > The functionality is supported by some devices, though. The PMBus spec has a register
> > to set the power limit, and PMBus devices supporting it are expected to limit power
> > output if the cap is reached. This is in addition to a separate register, which sets the 
> > power max alarm limit (and creates an alarm if that limit is exceeded). So I'll
> > have the need for both _cap and _max.
> 
> Which basically means that _cap should have been named _crit. For
> example, it is fairly common for temperature sensors to take action
> (e.g. speeding up fans or throttling the CPU) when the critical limit
> is exceeded. But again, now that it's there, I guess we have to live
> with it :(
> 
For PMBus, I have the following registers

Register		Attribute	Meaning
POWER_MAX		powerX_cap	Set enforced maximum power
POUT_OP_WARN_LIMIT	powerX_warn	Causes warning that output power is high
POUT_OP_FAULT_LIMIT	powerX_crit	Causes output overpower fault
					Response is configurable (none, shutdown, retry, disable)

At least in this respect, the meaning of "cap" is different to the meaning of "crit".

> As I read it, 1b) is a subset of 1a). 1b) is enough to fix the current
> wording, and 1a) can be done if some chips need it.
> 
Agreed. PMBus devices have separate flags for all three conditions, so I think
I'll introduce powerX_cap_alarm, powerX_max_alarm, and powerX_crit_alarm.

Which reminds me - would it make sense to introduce attributes to be able to configure
a reaction to a critical condition ? That would be useful to have for PMBus devices,
and some other chips as well - for example, the smm665 also has a configurable reaction
to such conditions.

Guenter

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Jean Delvare]

On Sun, 12 Dec 2010 13:04:54 -0800, Guenter Roeck wrote:
> On Sun, Dec 12, 2010 at 03:17:20PM -0500, Jean Delvare wrote:
> > Which basically means that _cap should have been named _crit. For
> > example, it is fairly common for temperature sensors to take action
> > (e.g. speeding up fans or throttling the CPU) when the critical limit
> > is exceeded. But again, now that it's there, I guess we have to live
> > with it :(
> > 
> For PMBus, I have the following registers
> 
> Register		Attribute	Meaning
> POWER_MAX		powerX_cap	Set enforced maximum power
> POUT_OP_WARN_LIMIT	powerX_warn	Causes warning that output power is high

powerX_warn doesn't exist in sysfs-interface. If anything, it should be
powerX_max, to match the naming of other channel types. I'm not
claiming these names were the best possible, but consistency is a must
for that kind of thing.

> POUT_OP_FAULT_LIMIT	powerX_crit	Causes output overpower fault
> 					Response is configurable (none, shutdown, retry, disable)
> 
> At least in this respect, the meaning of "cap" is different to the meaning of "crit".

OK. But maybe your powerX_cap should have been powerX_crit and your
powerX_crit should have been powerX_emergency. What really worries me
here is that we use different attribute names for power attributes when
I see no good reason for that.

> > As I read it, 1b) is a subset of 1a). 1b) is enough to fix the current
> > wording, and 1a) can be done if some chips need it.
>
> Agreed. PMBus devices have separate flags for all three conditions, so I think
> I'll introduce powerX_cap_alarm, powerX_max_alarm, and powerX_crit_alarm.
> 
> Which reminds me - would it make sense to introduce attributes to be able to configure
> a reaction to a critical condition ? That would be useful to have for PMBus devices,
> and some other chips as well - for example, the smm665 also has a configurable reaction
> to such conditions.

It would certainly make sense and be useful to at least have read-only
attributes for this, yes. Letting the user change the behavior seems
dangerous, but maybe it would be good to have too. But then we have to
come up with a proper standard for possible actions.

Also note that in many cases, the actual action is board-specific and
there is no way to figure it out. Many thermal sensors have logical
outputs going low (or high) when a limit is crossed. But there is no
way to know what, if anything, is connected to the output in question,
and what this something does. That's the reason why we don't currently
have any attribute for this.

-- 
Jean Delvare

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

Re: [lm-sensors] powerX_alarm sysfs attribute

[Guenter Roeck]

On Sun, Dec 12, 2010 at 04:20:29PM -0500, Jean Delvare wrote:
> On Sun, 12 Dec 2010 13:04:54 -0800, Guenter Roeck wrote:
> > On Sun, Dec 12, 2010 at 03:17:20PM -0500, Jean Delvare wrote:
> > > Which basically means that _cap should have been named _crit. For
> > > example, it is fairly common for temperature sensors to take action
> > > (e.g. speeding up fans or throttling the CPU) when the critical limit
> > > is exceeded. But again, now that it's there, I guess we have to live
> > > with it :(
> > > 
> > For PMBus, I have the following registers
> > 
> > Register		Attribute	Meaning
> > POWER_MAX		powerX_cap	Set enforced maximum power
> > POUT_OP_WARN_LIMIT	powerX_warn	Causes warning that output power is high
> 
> powerX_warn doesn't exist in sysfs-interface. If anything, it should be
> powerX_max, to match the naming of other channel types. I'm not
> claiming these names were the best possible, but consistency is a must
> for that kind of thing.
> 
Sorry, that is what I meant. Fat fingers, and my brain was elsewhere ;).

> > POUT_OP_FAULT_LIMIT	powerX_crit	Causes output overpower fault
> > 					Response is configurable (none, shutdown, retry, disable)
> > 
> > At least in this respect, the meaning of "cap" is different to the meaning of "crit".
> 
> OK. But maybe your powerX_cap should have been powerX_crit and your
> powerX_crit should have been powerX_emergency. What really worries me
> here is that we use different attribute names for power attributes when
> I see no good reason for that.
> 
But there is a difference in meaning, isn't it ? _max, _crit, and _emergency
don't result in changing the output power mode to "limiting", while _cap does.

I see that as essential difference - while _max_, _crit_, and _emerg may cause the 
OS or the chip to shut down, that is still different to a limiting mode, where the
output voltage would be reduced dynamically such that I*V does not exceed the cap. 

> > > As I read it, 1b) is a subset of 1a). 1b) is enough to fix the current
> > > wording, and 1a) can be done if some chips need it.
> >
> > Agreed. PMBus devices have separate flags for all three conditions, so I think
> > I'll introduce powerX_cap_alarm, powerX_max_alarm, and powerX_crit_alarm.
> > 
> > Which reminds me - would it make sense to introduce attributes to be able to configure
> > a reaction to a critical condition ? That would be useful to have for PMBus devices,
> > and some other chips as well - for example, the smm665 also has a configurable reaction
> > to such conditions.
> 
> It would certainly make sense and be useful to at least have read-only
> attributes for this, yes. Letting the user change the behavior seems
> dangerous, but maybe it would be good to have too. But then we have to
> come up with a proper standard for possible actions.
> 
You are right; I might introduce the attributes as read-only at least
for the time being and see if there is any feedback.

Guenter

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