phosphor-hwmon refactoring proposal

phosphor-hwmon refactoring proposal

[Brad Bishop]

Hi everyone

I have a quick phosphor-hwmon change proposal I’d like to get feedback on.

One issue with phosphor-hwmon that has been brought up a couple times now  
is that it gets its configuration settings from a file with a path in the  
filesystem that mirrors the path to the hwmon device in the device tree.   
This is problematic because the device tree path is not required to be  
stable, so whenever things move around there, the config files all have to  
be moved.

Unfortunately there are > 200 config files like this scattered throughout  
the openbmc source tree.  So my proposal addresses the limitation in a way  
that allows users to move over to the new config file locations on their  
own schedule.

Presently phosphor-hwmon gets its configuration from the environment,  
provided by systemd’s EnvironmentFile option:

EnvironmentFile=/etc/default/obmc/hwmon/%I.conf
ExecStart=/usr/bin/phosphor-hwmon-readd -o %I

The proposal is simply to quit passing the configuration via the  
environment and instead look for a config file, the location specified via  
logic in the application, with a path like:

/usr/share/phosphor-hwmon/i2c/2-004c.conf

This is the path to the hwmon parent device relative to /sys/bus e.g.  
/sys/bus/i2c/devices/2-004c/.  All the logic to do this would be added to  
the application itself, the sole input being the /sys/devices/... path  
provided by udev.  libsystemd has an sd-device subsystem that could be used  
to do the /sys/devices/… -> /sys/bus/… traversal if that makes things any  
easier.

The new config file could keep the same format as today, or we could move  
it to json and parse it with nlohmann?  If json we could preserve the  
current flat configuration or have dictionaries?

While we are poking around in here, I’d also recommend swapping out the  
current helper script with a SYSTEMD_WANTS as systemd has been updated to  
handle templates in udev rules since this was all originally implemented.

Finally, to enabled a staged migration, the new unit file and udev rules  
would be installed with a new feature flag (e.g. meson option) in place of  
the old.

Please poke holes.  thanks!

-brad

Re: phosphor-hwmon refactoring proposal

[Jeremy Kerr]

Hi Brad,

> I have a quick phosphor-hwmon change proposal I’d like to get feedback
> on.
> 
> One issue with phosphor-hwmon that has been brought up a couple times
> now  is that it gets its configuration settings from a file with a
> path in the  filesystem that mirrors the path to the hwmon device in
> the device tree.   This is problematic because the device tree path is
> not required to be  stable, so whenever things move around there, the
> config files all have to  be moved.

Yep, sounds like we should find a better approach.

> The proposal is simply to quit passing the configuration via the  
> environment and instead look for a config file, the location specified
> via logic in the application, with a path like:
> 
> /usr/share/phosphor-hwmon/i2c/2-004c.conf
> 
> This is the path to the hwmon parent device relative to /sys/bus

We want to ensure that any of those paths use persistent kernel
naming/numbering though. In your example, we would need that i2c bus to
be guaranteed to be probed as i2c bus id 2.

There are facilities in place to ensure this happens, and as far as I
know we are currently using those, but we will need to ensure that stays
the case.

Would this be specific to each bus type? The example for i2c is good, as
we do have a mechanism for persistent device paths under /sys/bus/i2c/,
but the same is not true for other bus types (eg, /sys/bus/platform). To
handle that, we may want the paths to be of the form:

 /usr/share/phosphor-hwmon/<bus>/<bus-specific-id>.conf

Where <bus> is from a predefined set of supported bus types, and
<bus-specific-id> depends on that; it would typically be the name under
/sys/bus/<bus>/devices, but we may want to introduce custom mappings
perhaps?

Cheers,


Jeremy

Re: phosphor-hwmon refactoring proposal

[Brad Bishop]

at 6:26 AM, Jeremy Kerr <jk@ozlabs.org> wrote:

> Hi Brad,
>
>> I have a quick phosphor-hwmon change proposal I’d like to get feedback
>> on.
>>
>> One issue with phosphor-hwmon that has been brought up a couple times
>> now  is that it gets its configuration settings from a file with a
>> path in the  filesystem that mirrors the path to the hwmon device in
>> the device tree.   This is problematic because the device tree path is
>> not required to be  stable, so whenever things move around there, the
>> config files all have to  be moved.
>
> Yep, sounds like we should find a better approach.
>
>> The proposal is simply to quit passing the configuration via the
>> environment and instead look for a config file, the location specified
>> via logic in the application, with a path like:
>>
>> /usr/share/phosphor-hwmon/i2c/2-004c.conf
>>
>> This is the path to the hwmon parent device relative to /sys/bus
>
> We want to ensure that any of those paths use persistent kernel
> naming/numbering though. In your example, we would need that i2c bus to
> be guaranteed to be probed as i2c bus id 2.
>
> There are facilities in place to ensure this happens, and as far as I
> know we are currently using those, but we will need to ensure that stays
> the case.

Can you elaborate a tiny bit on what the facilities are?  I just want to  
convince myself we are in fact using those.

> Would this be specific to each bus type? The example for i2c is good, as
> we do have a mechanism for persistent device paths under /sys/bus/i2c/,

We’ll need FSI+sbefifo too for the occ hwmon devices.  Are there facilities  
for stable bus id probing for FSI+sbefifo?

> but the same is not true for other bus types (eg, /sys/bus/platform). To
> handle that, we may want the paths to be of the form:
>
>  /usr/share/phosphor-hwmon/<bus>/<bus-specific-id>.conf

Slightly confused...I thought this is what I was trying to say :-)

The application would be passed the /sys/devices/… path on the command  
line, then find that device under /sys/bus/.  Once it has done that, it can  
extract <bus> and <bus-specific-id> from the discovered /sys/bus/… path.   
It then uses <bus> and <bus-specific-id> to construct the path to the  
config file, as you’ve spec’ed out.

> Where <bus> is from a predefined set of supported bus types, and

Why must they be pre-defined?  Is it because <bus-specific-id> may not be  
guaranteed to be stable for a given <bus> and we therefore need to ensure  
a-priori that they are stable before allowing users to use the program with  
hwmon devices on those bus types?

> <bus-specific-id> depends on that; it would typically be the name under
> /sys/bus/<bus>/devices, but we may want to introduce custom mappings
> perhaps?

I can’t think of any use case for custom mappings right now.  I did not  
think very hard about it though.

Are we desiging anything that would prevent us from adding the custom  
mappings later when we discover a need for them?

Thanks!

-brad

Re: phosphor-hwmon refactoring proposal

[Patrick Venture]

On Thu, Oct 17, 2019 at 6:16 AM Brad Bishop <bradleyb@fuzziesquirrel.com> wrote:
>
> Hi everyone
>
> I have a quick phosphor-hwmon change proposal I’d like to get feedback on.
>
> One issue with phosphor-hwmon that has been brought up a couple times now
> is that it gets its configuration settings from a file with a path in the
> filesystem that mirrors the path to the hwmon device in the device tree.
> This is problematic because the device tree path is not required to be
> stable, so whenever things move around there, the config files all have to
> be moved.
>
> Unfortunately there are > 200 config files like this scattered throughout
> the openbmc source tree.  So my proposal addresses the limitation in a way
> that allows users to move over to the new config file locations on their
> own schedule.
>
> Presently phosphor-hwmon gets its configuration from the environment,
> provided by systemd’s EnvironmentFile option:
>
> EnvironmentFile=/etc/default/obmc/hwmon/%I.conf
> ExecStart=/usr/bin/phosphor-hwmon-readd -o %I
>
> The proposal is simply to quit passing the configuration via the
> environment and instead look for a config file, the location specified via
> logic in the application, with a path like:
>
> /usr/share/phosphor-hwmon/i2c/2-004c.conf
>
> This is the path to the hwmon parent device relative to /sys/bus e.g.
> /sys/bus/i2c/devices/2-004c/.  All the logic to do this would be added to
> the application itself, the sole input being the /sys/devices/... path
> provided by udev.  libsystemd has an sd-device subsystem that could be used
> to do the /sys/devices/… -> /sys/bus/… traversal if that makes things any
> easier.
>
> The new config file could keep the same format as today, or we could move
> it to json and parse it with nlohmann?  If json we could preserve the
> current flat configuration or have dictionaries?
>
> While we are poking around in here, I’d also recommend swapping out the
> current helper script with a SYSTEMD_WANTS as systemd has been updated to
> handle templates in udev rules since this was all originally implemented.
>
> Finally, to enabled a staged migration, the new unit file and udev rules
> would be installed with a new feature flag (e.g. meson option) in place of
> the old.
>
> Please poke holes.  thanks!

I'll let others focus on the hole poking.  I just want to say that
testing this userspace code will be easier if we transition away from
environment-based configuration.  So I'm all for that.

>
> -brad

Re: phosphor-hwmon refactoring proposal

[Andrew Jeffery]

On Fri, 18 Oct 2019, at 22:44, Brad Bishop wrote:
> at 6:26 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
> 
> > Hi Brad,
> >
> >> I have a quick phosphor-hwmon change proposal I’d like to get feedback
> >> on.
> >>
> >> One issue with phosphor-hwmon that has been brought up a couple times
> >> now  is that it gets its configuration settings from a file with a
> >> path in the  filesystem that mirrors the path to the hwmon device in
> >> the device tree.   This is problematic because the device tree path is
> >> not required to be  stable, so whenever things move around there, the
> >> config files all have to  be moved.
> >
> > Yep, sounds like we should find a better approach.
> >
> >> The proposal is simply to quit passing the configuration via the
> >> environment and instead look for a config file, the location specified
> >> via logic in the application, with a path like:
> >>
> >> /usr/share/phosphor-hwmon/i2c/2-004c.conf
> >>
> >> This is the path to the hwmon parent device relative to /sys/bus
> >
> > We want to ensure that any of those paths use persistent kernel
> > naming/numbering though. In your example, we would need that i2c bus to
> > be guaranteed to be probed as i2c bus id 2.
> >
> > There are facilities in place to ensure this happens, and as far as I
> > know we are currently using those, but we will need to ensure that stays
> > the case.
> 
> Can you elaborate a tiny bit on what the facilities are?  I just want to  
> convince myself we are in fact using those.

One is aliases in the devicetrees, e.g: 

https://github.com/openbmc/linux/blob/dev-5.3/arch/arm/boot/dts/aspeed-g5.dtsi#L11