Starting the phosphor-hwmon service from udev rules

Starting the phosphor-hwmon service from udev rules

[Matt Spinler]

I am working on a story this sprint to start the phosphor-hwmon service 
from udev rules.  In addition to just starting the service based on a 
device path, I need to be able to communicate to it some threshold 
values, like critical high temps for example.

I have a proposal for the rules at 
https://gerrit.openbmc-project.xyz/#/c/1243/.

It has lines like:

SUBSYSTEM=="hwmon", DEVPATH=="*i2c-3/3-0077/*", ACTION == "add", 
TAG+="systemd", 
ENV{SYSTEMD_WANTS}+="phosphor-hwmon@/sys/class/hwmon/%k.service", 
ENV{ENVFILE}+="/etc/hwmon.d/hwmon-bmp280-3-77.conf"

where the .conf file will contain any threshold values, maybe like 
CRIT=50.  The command line in the unit file would then somehow make a 
udevadm call to pull out the ENVFILE so it can get passed into the 
executable along with the device path.

Is this the recommended way to go about doing this?  Or instead of 
having a separate udev rule for each device, but 1 service file, is it 
better to have 1 udev rule but separate service files?

Thanks

Re: Starting the phosphor-hwmon service from udev rules

[Joel Stanley]

On Tue, Nov 29, 2016 at 3:01 AM, Matt Spinler
<mspinler@linux.vnet.ibm.com> wrote:
> I am working on a story this sprint to start the phosphor-hwmon service from
> udev rules.  In addition to just starting the service based on a device
> path, I need to be able to communicate to it some threshold values, like
> critical high temps for example.

What are the functions of the 'phosphor-hwmon' service?

I would consider naming it phosphor-hardware-monitor or similar, in
case you decide it should monitor things other than Linux 'hwmon'
devices.

>
> I have a proposal for the rules at
> https://gerrit.openbmc-project.xyz/#/c/1243/.
>
> It has lines like:
>
> SUBSYSTEM=="hwmon", DEVPATH=="*i2c-3/3-0077/*", ACTION == "add",
> TAG+="systemd",
> ENV{SYSTEMD_WANTS}+="phosphor-hwmon@/sys/class/hwmon/%k.service",
> ENV{ENVFILE}+="/etc/hwmon.d/hwmon-bmp280-3-77.conf"

Do we need to match only on the path? Can it match on the device driver instead?

>
> where the .conf file will contain any threshold values, maybe like CRIT=50.
> The command line in the unit file would then somehow make a udevadm call to
> pull out the ENVFILE so it can get passed into the executable along with the
> device path.
>
> Is this the recommended way to go about doing this?  Or instead of having a
> separate udev rule for each device, but 1 service file, is it better to have
> 1 udev rule but separate service files?

It appears you plan on re-implementing the functionality of the
lm-sensors package. While the upstream project has dissappeared from
the net, the source code and documentation is still available. I would
consider firstly trying to re-use existing daemons and configuration
that we already have access to. If that does not work, then reading
and re-using the source code to adopt the same design would save you
time and effort.

https://web.archive.org/web/20150905145034/http://www.lm-sensors.org/

Cheers,

Joel

Re: Starting the phosphor-hwmon service from udev rules

[Patrick Williams]

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

On Tue, Nov 29, 2016 at 11:58:32AM +1030, Joel Stanley wrote:

> What are the functions of the 'phosphor-hwmon' service?

phosphor-hwmon is an implementation of these dbus interfaces, which
sources its information from the Linux hwmon interfaces:

https://github.com/openbmc/phosphor-dbus-interfaces/tree/master/xyz/openbmc_project/Sensor

> I would consider naming it phosphor-hardware-monitor or similar, in
> case you decide it should monitor things other than Linux 'hwmon'
> devices.

An alternative source of data should be in a separate application /
repository.  The non-hwmon-specific should be mostly contained in the
generated dbus bindings, so there is no or very little overlap between
different implementations.

> > It has lines like:
> >
> > SUBSYSTEM=="hwmon", DEVPATH=="*i2c-3/3-0077/*", ACTION == "add",
> > TAG+="systemd",
> > ENV{SYSTEMD_WANTS}+="phosphor-hwmon@/sys/class/hwmon/%k.service",
> > ENV{ENVFILE}+="/etc/hwmon.d/hwmon-bmp280-3-77.conf"
> 
> Do we need to match only on the path? Can it match on the device driver instead?

The driver name is insufficient to differentiate between two different
sensors of the same device type.  For instance, the front and rear
ambient may both be LMxx-class devices, but have different dbus paths
and potentially different thresholds.

> > where the .conf file will contain any threshold values, maybe like CRIT=50.
> > The command line in the unit file would then somehow make a udevadm call to
> > pull out the ENVFILE so it can get passed into the executable along with the
> > device path.
> >
> > Is this the recommended way to go about doing this?  Or instead of having a
> > separate udev rule for each device, but 1 service file, is it better to have
> > 1 udev rule but separate service files?
> 
> It appears you plan on re-implementing the functionality of the
> lm-sensors package. While the upstream project has dissappeared from
> the net, the source code and documentation is still available. I would
> consider firstly trying to re-use existing daemons and configuration
> that we already have access to. If that does not work, then reading
> and re-using the source code to adopt the same design would save you
> time and effort.

Can you give more details on which aspects you feel there is overlap?

In any case, your questions and critiques should all be directed at Brad
who is implementing phosphor-hwmon and not Matt who asked specifically
about how to launch the application from MRW-based information?

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Starting the phosphor-hwmon service from udev rules

[Patrick Williams]

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

On Mon, Nov 28, 2016 at 10:31:20AM -0600, Matt Spinler wrote:
> The command line in the unit file would then somehow make a 
> udevadm call to pull out the ENVFILE so it can get passed into the 
> executable along with the device path.

This was a detail I missed before.  systemd service files natively have
the variable 'ENVFILE'.  If they don't automatically take the ENVFILE
from the environment (via udev invocation) then I would rather we use
unique systemd service files instead of unique udev rules.  Having the
systemd service file launch some kind of piped shell processing is
problematic and certainly not preferred.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Starting the phosphor-hwmon service from udev rules

[Matt Spinler]

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



On 11/28/2016 8:42 PM, Patrick Williams wrote:
> On Mon, Nov 28, 2016 at 10:31:20AM -0600, Matt Spinler wrote:
>> The command line in the unit file would then somehow make a
>> udevadm call to pull out the ENVFILE so it can get passed into the
>> executable along with the device path.
> This was a detail I missed before.  systemd service files natively have
> the variable 'ENVFILE'.  If they don't automatically take the ENVFILE
> from the environment (via udev invocation) then I would rather we use
> unique systemd service files instead of unique udev rules.  Having the
> systemd service file launch some kind of piped shell processing is
> problematic and certainly not preferred.
>

OK. The only way for the udev rule to send something directly into the 
service file is with the template parameter, and that needs to take the 
device path, since that isn't known ahead of time.  (Unless the service 
wants to parse its %i into multiple values?)

So now I think the rule would look something like:
SUBSYSTEM=="hwmon", DEVPATH=="*i2c-3/3-0077/*", ACTION == "add", 
TAG+="systemd", 
ENV{SYSTEMD_WANTS}+="phosphor-hwmon-3-0077@/sys/class/hwmon/%k.service"

and could use the Environment or EnvironmentFile directive in the 
generated device specific unit file to pick up threshold vars.

Another issue I have is if the MRW should have the knowledge of if a 
device has a hwmon driver or not?  While it would make things a lot more 
simple if it did, I'm not sure if it's in its domain. If it doesn't, 
then it would either still generate rules and unit files for every I2C 
device and some just wouldn't get used, or we'd have to keep that 
knowledge in our tree somewhere.
Thoughts on that?


[-- Attachment #2: Type: text/html, Size: 2383 bytes --]

Re: Starting the phosphor-hwmon service from udev rules

[Andrew Jeffery]

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

On Tue, 2016-11-29 at 10:30 -0600, Matt Spinler wrote:
> Another issue I have is if the MRW should have the knowledge of if a
> device has a hwmon driver or not?  While it would make things a lot
> more simple if it did, I'm not sure if it's in its domain. If it
> doesn't, then it would either still generate rules and unit files for
> every I2C device and some just wouldn't get used

This seems reasonable to me. That way as hwmon drivers are added to the
kernel we have no extra work to do to enable the devices, and we don't
have work to do to keep a list of disabled devices.

The "extra" cruft is a problem if we're running out of space, but I
doubt udev rules and systemd unit files are going to be our first
concern there.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Starting the phosphor-hwmon service from udev rules

[Patrick Williams]

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

Matt,

We had a phone discussion last night with Joel, Andrew J, and Brad and I
was suppose to catch up with you today and never found time to.  (It was
half-impromptu and half-miscommunication on the desired topic.)

The net is that we came up with a third option for specifying these:

   1. Unique udev rules per device with corresponding environment file, and
      single systemd service, 
   2. Single udev rule per driver, unique systemd service file (with
      corresponding environment file?) per device.

   3. Single udev rule per driver, single template systemd service file per
      application (ex. phosphor-hwmon@.service), unique environment file
      per device.

On Tue, Nov 29, 2016 at 10:30:12AM -0600, Matt Spinler wrote:

> So now I think the rule would look something like:
> SUBSYSTEM=="hwmon", DEVPATH=="*i2c-3/3-0077/*", ACTION == "add", 
> TAG+="systemd", 
> ENV{SYSTEMD_WANTS}+="phosphor-hwmon-3-0077@/sys/class/hwmon/%k.service"

The 'SYSTEMD_WANTS' is useful if you want to trigger a specific
service.  If you leave that out, by default systemd triggers a @.device,
which other services can depend on.  sys-class-i2c-dev-i2c-3-0077.device
maybe in this case?

> and could use the Environment or EnvironmentFile directive in the 
> generated device specific unit file to pick up threshold vars.

In obmc-console@.service we have an example of using 'BindsTo'.  I think
we could write the phosphor-hwmon@.service to 'BindTo=sys-%i' and also
ConditionPathExists=/etc/conf.d/phosphor-hwmon-%i ,
EnvironmentFile=/etc/conf.d/phosphor-hwmon-%i ?

> Another issue I have is if the MRW should have the knowledge of if a 
> device has a hwmon driver or not?  While it would make things a lot more 
> simple if it did, I'm not sure if it's in its domain. If it doesn't, 
> then it would either still generate rules and unit files for every I2C 
> device and some just wouldn't get used, or we'd have to keep that 
> knowledge in our tree somewhere.
> Thoughts on that?

Doesn't it have to?  It isn't just that there is an lm77 sensor out on
an i2c bus and therefore we create the environment file.  The
environment file needs to have a decent amount of information, such as
what is the "NiceName" of the sensor ("FrontAmbient" maybe).  When you
talk about one of the MAX fan controller chips we potentially have
multiple fans on that chip so it more than one "sensor" worth of
configuration as well.  I can't imagine how the MRW data wouldn't
effectively encode that we expect the device to have an hwmon driver
(not directly, but indirectly by the presence of this information).

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Starting the phosphor-hwmon service from udev rules

[Matt Spinler]

On 11/29/2016 8:19 PM, Patrick Williams wrote:
> Matt,
>
> We had a phone discussion last night with Joel, Andrew J, and Brad and I
> was suppose to catch up with you today and never found time to.  (It was
> half-impromptu and half-miscommunication on the desired topic.)
>
> The net is that we came up with a third option for specifying these:
>
>     1. Unique udev rules per device with corresponding environment file, and
>        single systemd service,
>     2. Single udev rule per driver, unique systemd service file (with
>        corresponding environment file?) per device.
>
>     3. Single udev rule per driver, single template systemd service file per
>        application (ex. phosphor-hwmon@.service), unique environment file
>        per device.
>
> On Tue, Nov 29, 2016 at 10:30:12AM -0600, Matt Spinler wrote:
>
>> So now I think the rule would look something like:
>> SUBSYSTEM=="hwmon", DEVPATH=="*i2c-3/3-0077/*", ACTION == "add",
>> TAG+="systemd",
>> ENV{SYSTEMD_WANTS}+="phosphor-hwmon-3-0077@/sys/class/hwmon/%k.service"
> The 'SYSTEMD_WANTS' is useful if you want to trigger a specific
> service.  If you leave that out, by default systemd triggers a @.device,
> which other services can depend on.  sys-class-i2c-dev-i2c-3-0077.device
> maybe in this case?
>
>> and could use the Environment or EnvironmentFile directive in the
>> generated device specific unit file to pick up threshold vars.
> In obmc-console@.service we have an example of using 'BindsTo'.  I think
> we could write the phosphor-hwmon@.service to 'BindTo=sys-%i' and also
> ConditionPathExists=/etc/conf.d/phosphor-hwmon-%i ,
> EnvironmentFile=/etc/conf.d/phosphor-hwmon-%i ?
The issue with this is the %i is the device path of the hwmon driver, 
which looks like
/devices/platform/ahb/ahb:apb/1e78a000.i2c/i2c-0/i2c-0/0-0068/hwmon/hwmon0,
and the hwmon<num> can't be known ahead of time by the MRW so it can't make
conf files named that way.

This means the the environment file has to be explicitly set in the 
service file,  so I'd
need to generate unique service files.  (Or phosphor-hwmon just
builds its own env file names based on segments from its device path.)


>> Another issue I have is if the MRW should have the knowledge of if a
>> device has a hwmon driver or not?  While it would make things a lot more
>> simple if it did, I'm not sure if it's in its domain. If it doesn't,
>> then it would either still generate rules and unit files for every I2C
>> device and some just wouldn't get used, or we'd have to keep that
>> knowledge in our tree somewhere.
>> Thoughts on that?
> Doesn't it have to?  It isn't just that there is an lm77 sensor out on
> an i2c bus and therefore we create the environment file.  The
> environment file needs to have a decent amount of information, such as
> what is the "NiceName" of the sensor ("FrontAmbient" maybe).  When you
> talk about one of the MAX fan controller chips we potentially have
> multiple fans on that chip so it more than one "sensor" worth of
> configuration as well.  I can't imagine how the MRW data wouldn't
> effectively encode that we expect the device to have an hwmon driver
> (not directly, but indirectly by the presence of this information).
>

Re: Starting the phosphor-hwmon service from udev rules

[Patrick Williams]

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

On Wed, Dec 07, 2016 at 01:44:52PM -0600, Matt Spinler wrote:
> The issue with this is the %i is the device path of the hwmon driver, 
> which looks like
> /devices/platform/ahb/ahb:apb/1e78a000.i2c/i2c-0/i2c-0/0-0068/hwmon/hwmon0,
> and the hwmon<num> can't be known ahead of time by the MRW so it can't make
> conf files named that way.

Isn't this %i vs %I?  %I would be the unescaped path.  %i would be
escaped, so it would be devices-platform-ahb-ahb:apb-  (slash changed to
dash in general)

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]