entity-manager: SBTSI and hwmontempsensor

entity-manager: SBTSI and hwmontempsensor

[Oskar Senft]

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

Hi Zev

In
https://github.com/openbmc/entity-manager/commit/e22143df37faa0b0f5e2918d2f505b9f64e74b0f
you "removed devices now managed by hwmontempsensor".

I'm trying to figure out how to add SBTSI support for the TYAN S8036 board
(AMD Milan). Do I just add the device to the DTS and then reference it in
EntityManager board configuration via its bus and address?

Is there still a way to dynamically get Entity Manager (or now
dbus-sensors) to bind the driver or does it HAVE to happen in the DTS now?

Thanks!

Oskar.

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

Re: entity-manager: SBTSI and hwmontempsensor

[Zev Weiss]

On Thu, May 04, 2023 at 01:00:47PM PDT, Oskar Senft wrote:
>Hi Zev
>
>In
>https://github.com/openbmc/entity-manager/commit/e22143df37faa0b0f5e2918d2f505b9f64e74b0f
>you "removed devices now managed by hwmontempsensor".
>
>I'm trying to figure out how to add SBTSI support for the TYAN S8036 board
>(AMD Milan). Do I just add the device to the DTS and then reference it in
>EntityManager board configuration via its bus and address?
>
>Is there still a way to dynamically get Entity Manager (or now
>dbus-sensors) to bind the driver or does it HAVE to happen in the DTS now?
>
>Thanks!
>
>Oskar.

Hi Oskar,

Assuming you use a corresponding, recent enough version of dbus-sensors 
(i.e. including commit a1456c4aba, though hopefully also with at least 
commit 7627c860fa, which was a follow-up bugfix), it *should* work the 
same way it did before, with the device described in your E-M config.

It shouldn't be in your DTS, because then it'd be statically defined and 
hwmontempsensor wouldn't be able to remove it when the host is powered 
off (which I assume you'd want).

In terms of userspace/kernel separation, it's overall pretty similar to 
how it had been previously, just with the management of device lifetimes 
(instantiation & removal) moved to hwmontempsensor instead of 
entity-manager.

Hopefully that's clear?  Let me know if you're having trouble getting 
things working though.


Zev

Hot-plugging non-sensor devices on non-PnP buses (was: Re: entity-manager: SBTSI and hwmontempsensor)

[Paul Fertser]

Hi Zev,

I do not mean to hi-jack the topic as the issue I have a question about
is closely related, please see inline.

On Thu, May 04, 2023 at 02:10:15PM -0700, Zev Weiss wrote:
> It shouldn't be in your DTS, because then it'd be statically defined and
> hwmontempsensor wouldn't be able to remove it when the host is powered off
> (which I assume you'd want).
> 
> In terms of userspace/kernel separation, it's overall pretty similar to how
> it had been previously, just with the management of device lifetimes
> (instantiation & removal) moved to hwmontempsensor instead of
> entity-manager.

I see similar mechanism is also implemented in omnisensor, so this can
be an option (probably preferred since it's a much cleaner code) too.

But what if the device in consideration isn't a sensor at all? We're
trying to use PCA9551 which is a LED driver controllable over
I2C. Since it's meant to show statuses of the host storage devices
it's powered only when the host is powered. The kernel driver
leds-pca955x handles it nicely but the probe() needs to be run with
the device powered.

I initially thought entity-manager should be the right place in the
architecture to handle cases like this, but now that you say OpenBMC
is moving towards implementing dynamic driver assigning functionality
in the sensors daemons instead I wonder what the solution for the
non-sensor devices should be (as duplicating the relevant code in
phosphor-led-manager seems to be obviously wrong). I can also imagine
e.g. SPI devices needing similar treatment, so it's neither sensors
nor I2C specific in the big picture.

What further complicates situation with leds-pca955x specifically is
that it /needs/ DT or platform data to work, and that makes it try
binding automatically on startup, and probe() fails while the host
system is off, and "new_device" sysfs node can't be used to retry (as
the device is already defined), so either the driver needs to be
modular and reloaded with essentially rmmod/insmod sequence or the
userspace can use sysfs "bind" node to call probe() again (this is
also problematic with entity-manager as $Address template argument
isn't suitable for a string like 5-0019, where 19 is in hex).

I would be happy to hear your and the other OpenBMC community member's
thoughts on this matter.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: Hot-plugging non-sensor devices on non-PnP buses (was: Re: entity-manager: SBTSI and hwmontempsensor)

[Zev Weiss]

On Fri, May 05, 2023 at 02:23:06AM PDT, Paul Fertser wrote:
>Hi Zev,
>
>I do not mean to hi-jack the topic as the issue I have a question about
>is closely related, please see inline.
>
>On Thu, May 04, 2023 at 02:10:15PM -0700, Zev Weiss wrote:
>> It shouldn't be in your DTS, because then it'd be statically defined and
>> hwmontempsensor wouldn't be able to remove it when the host is powered off
>> (which I assume you'd want).
>>
>> In terms of userspace/kernel separation, it's overall pretty similar to how
>> it had been previously, just with the management of device lifetimes
>> (instantiation & removal) moved to hwmontempsensor instead of
>> entity-manager.
>
>I see similar mechanism is also implemented in omnisensor, so this can
>be an option (probably preferred since it's a much cleaner code) too.
>

Yeah, omnisensor works the same way, at least in part out of necessity 
due to aiming to be drop-in compatible with dbus-sensors.

>But what if the device in consideration isn't a sensor at all? We're
>trying to use PCA9551 which is a LED driver controllable over
>I2C. Since it's meant to show statuses of the host storage devices
>it's powered only when the host is powered. The kernel driver
>leds-pca955x handles it nicely but the probe() needs to be run with
>the device powered.
>

Okay, that sounds pretty analogous to the scenario I had that initially 
motivated moving device management to hwmontempsensor (a sensor driver 
that needed to be bound when the host was powered on and unbound when it 
shut down), so trying to reuse the same solution seems sensible.

>I initially thought entity-manager should be the right place in the
>architecture to handle cases like this, but now that you say OpenBMC
>is moving towards implementing dynamic driver assigning functionality
>in the sensors daemons instead I wonder what the solution for the
>non-sensor devices should be (as duplicating the relevant code in
>phosphor-led-manager seems to be obviously wrong). I can also imagine
>e.g. SPI devices needing similar treatment, so it's neither sensors
>nor I2C specific in the big picture.
>

Yeah, the existing implementation is certainly a fairly small slice of 
the full generality we'd ideally have.  I'm afraid I don't have any 
particularly great thoughts offhand about the best way to generalize the 
approach though.  Code duplication is of course not ideal, though I 
think the amount of code involved isn't too huge; factoring it out into 
a library or something to be shared between dbus-sensors and 
phosphor-led-manager (do we know of any other potential users at the 
moment?) seems like a fair amount of added complexity to avoid 
duplicating it -- but then again, duplication of that sort of common 
infrastructure code is a large part of what drives me nuts about the 
dbus-sensors codebase, so I'd certainly sympathize with not wanting to 
head down that path...

>What further complicates situation with leds-pca955x specifically is
>that it /needs/ DT or platform data to work, and that makes it try
>binding automatically on startup, and probe() fails while the host
>system is off, and "new_device" sysfs node can't be used to retry (as
>the device is already defined), so either the driver needs to be
>modular and reloaded with essentially rmmod/insmod sequence or the
>userspace can use sysfs "bind" node to call probe() again (this is
>also problematic with entity-manager as $Address template argument
>isn't suitable for a string like 5-0019, where 19 is in hex).
>

This seems like the trickier part to me.  AFAIK the kernel as it stands 
doesn't really offer any way of specifying any of the additional 
parameters that DT properties and such can provide when dynamically 
instantiating devices, so if you need any non-default configuration your 
only option is a statically-defined device (via a DT node), and if 
that's not an option because you need dynamic instantiation then you're 
kind of out of luck unfortunately.

DT overlays should solve this problem, but they've never made it 
upstream into the mainline kernel, and the last time I looked into it 
the impression I got was that getting them there would probably be a 
years-long effort, and one with no guarantee of success at that.  :/

It's not a great situation.

>I would be happy to hear your and the other OpenBMC community member's
>thoughts on this matter.
>

I'd also be curious if others have thoughts -- Ed or Andrew perhaps?


Zev

Re: Hot-plugging non-sensor devices on non-PnP buses (was: Re: entity-manager: SBTSI and hwmontempsensor)

[Paul Fertser]

Hi Zev,

Thank you for answering. A quick additional point inline.

On Mon, May 08, 2023 at 03:46:13PM -0700, Zev Weiss wrote:
> On Fri, May 05, 2023 at 02:23:06AM PDT, Paul Fertser wrote:
> > What further complicates situation with leds-pca955x specifically is
> > that it /needs/ DT or platform data to work, and that makes it try
> > binding automatically on startup, and probe() fails while the host
> > system is off, and "new_device" sysfs node can't be used to retry (as
> > the device is already defined), so either the driver needs to be
> > modular and reloaded with essentially rmmod/insmod sequence or the
> > userspace can use sysfs "bind" node to call probe() again (this is
> > also problematic with entity-manager as $Address template argument
> > isn't suitable for a string like 5-0019, where 19 is in hex).
> > 
> 
> This seems like the trickier part to me.  AFAIK the kernel as it stands
> doesn't really offer any way of specifying any of the additional parameters
> that DT properties and such can provide when dynamically instantiating
> devices, so if you need any non-default configuration your only option is a
> statically-defined device (via a DT node), and if that's not an option
> because you need dynamic instantiation then you're kind of out of luck
> unfortunately.

Dynamic instantiation is still possible either by having the
corresponding kernel driver modular or by using "bind" and "unbind"
sysfs nodes. In this specific case we tested having the driver
built-in, it tries binding on BMC startup, fails if the host is off,
then at any point of time one can do "echo 5-0019 >
/sys/bus/i2c/drivers/leds-pca955x/bind" and it'll re-try binding, and
that works if the host is on at the moment. And of course it can be
"unbind" later if needed. This could even almost work with current
entity-manager code if it was listening for host power state events,
if it wasn't skipping the devices that it already tried exporting, and
if it had something like $HexAddress for the template arguments.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: Hot-plugging non-sensor devices on non-PnP buses (was: Re: entity-manager: SBTSI and hwmontempsensor)

[Zev Weiss]

On Tue, May 09, 2023 at 12:40:26AM PDT, Paul Fertser wrote:
>Hi Zev,
>
>Thank you for answering. A quick additional point inline.
>
>On Mon, May 08, 2023 at 03:46:13PM -0700, Zev Weiss wrote:
>> On Fri, May 05, 2023 at 02:23:06AM PDT, Paul Fertser wrote:
>> > What further complicates situation with leds-pca955x specifically is
>> > that it /needs/ DT or platform data to work, and that makes it try
>> > binding automatically on startup, and probe() fails while the host
>> > system is off, and "new_device" sysfs node can't be used to retry (as
>> > the device is already defined), so either the driver needs to be
>> > modular and reloaded with essentially rmmod/insmod sequence or the
>> > userspace can use sysfs "bind" node to call probe() again (this is
>> > also problematic with entity-manager as $Address template argument
>> > isn't suitable for a string like 5-0019, where 19 is in hex).
>> >
>>
>> This seems like the trickier part to me.  AFAIK the kernel as it stands
>> doesn't really offer any way of specifying any of the additional parameters
>> that DT properties and such can provide when dynamically instantiating
>> devices, so if you need any non-default configuration your only option is a
>> statically-defined device (via a DT node), and if that's not an option
>> because you need dynamic instantiation then you're kind of out of luck
>> unfortunately.
>
>Dynamic instantiation is still possible either by having the
>corresponding kernel driver modular 

True to an extent, though it's rather more of a blunt instrument, since 
it's a per-driver operation instead of per-device (it's a different 
operation that happens to trigger the desired behavior as a side-effect, 
I'd say).

>or by using "bind" and "unbind"
>sysfs nodes.

Ah right, I'd forgotten about that one -- I actually sent some patches 
upstream to try to make that mode work a little more gracefully a while 
back (adding a way of preventing automatic driver-bind on boot so it 
would only be done when explicitly requested), but never arrived at 
anything that all the relevant maintainers approved of.

Also (this is arguably somewhat pedantic, but for the kinds of things we 
might end up dealing with perhaps relevant): while it is at least 
operating at the same per-device granularity, bind/unbind is still 
semantically a distinct operation from true dynamic instantiation 
though.  For a bind operation you need an extant device to attach a 
driver to, whereas the i2c new_device operation actually creates a new 
thing that wasn't previously there.  There are many cases where 
bind/unbind might be sufficient, but in other scenarios it might not be.

>In this specific case we tested having the driver
>built-in, it tries binding on BMC startup, fails if the host is off,
>then at any point of time one can do "echo 5-0019 >
>/sys/bus/i2c/drivers/leds-pca955x/bind" and it'll re-try binding, and
>that works if the host is on at the moment. And of course it can be
>"unbind" later if needed. This could even almost work with current
>entity-manager code if it was listening for host power state events,
>if it wasn't skipping the devices that it already tried exporting, and
>if it had something like $HexAddress for the template arguments.
>

Perhaps that's something worth experimenting with and posting E-M 
patches for?  Though the "bouncing off the guardrails" aspect (the 
futile bind attempt on boot) is a bit unfortunate of course...and it 
might need some additional consideration of what happens if the host 
_does_ happen to be powered on when the BMC boots (and the boot-time 
bind succeeds instead of failing).


Zev

Re: Hot-plugging non-sensor devices on non-PnP buses (was: Re: entity-manager: SBTSI and hwmontempsensor)

[Paul Fertser]

On Tue, May 09, 2023 at 03:35:09AM -0700, Zev Weiss wrote:
> On Tue, May 09, 2023 at 12:40:26AM PDT, Paul Fertser wrote:
> > In this specific case we tested having the driver
> > built-in, it tries binding on BMC startup, fails if the host is off,
> > then at any point of time one can do "echo 5-0019 >
> > /sys/bus/i2c/drivers/leds-pca955x/bind" and it'll re-try binding, and
> > that works if the host is on at the moment. And of course it can be
> > "unbind" later if needed. This could even almost work with current
> > entity-manager code if it was listening for host power state events,
> > if it wasn't skipping the devices that it already tried exporting, and
> > if it had something like $HexAddress for the template arguments.
> > 
> 
> Perhaps that's something worth experimenting with and posting E-M patches
> for?

I tried toying with the idea despite entity-manager code giving me
head spinning (especially after seeing how 9b86787adea3f added even
more asynchronicity without any obvious way to pass the end result of
the "exporting" back to the upper layers, and with D-Bus objects
getting added irregardless of exporting failures etc), so that
reasoning about all the possible race conditions feels beyond my
abilities) and also that current code seems to be written under
assumption either it retries the whole board or none of it
(deriveNewConfiguration() tracks only top-level objects); the news
that the functionality is moving to other daemons made me think that
it's probably not worth persuing, and that I certainly need some
feedback from the E-M maintainers to avoid wasting even more time on
this.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: entity-manager: SBTSI and hwmontempsensor

[Oskar Senft]

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

Hi Zev

Thank you for your quick response!

Ali got it to work - it was much easier than I had thought. Thanks for all
your great work!

Oskar.

On Thu, May 4, 2023 at 5:10 PM Zev Weiss <zev@bewilderbeest.net> wrote:

> On Thu, May 04, 2023 at 01:00:47PM PDT, Oskar Senft wrote:
> >Hi Zev
> >
> >In
> >
> https://github.com/openbmc/entity-manager/commit/e22143df37faa0b0f5e2918d2f505b9f64e74b0f
> >you "removed devices now managed by hwmontempsensor".
> >
> >I'm trying to figure out how to add SBTSI support for the TYAN S8036 board
> >(AMD Milan). Do I just add the device to the DTS and then reference it in
> >EntityManager board configuration via its bus and address?
> >
> >Is there still a way to dynamically get Entity Manager (or now
> >dbus-sensors) to bind the driver or does it HAVE to happen in the DTS now?
> >
> >Thanks!
> >
> >Oskar.
>
> Hi Oskar,
>
> Assuming you use a corresponding, recent enough version of dbus-sensors
> (i.e. including commit a1456c4aba, though hopefully also with at least
> commit 7627c860fa, which was a follow-up bugfix), it *should* work the
> same way it did before, with the device described in your E-M config.
>
> It shouldn't be in your DTS, because then it'd be statically defined and
> hwmontempsensor wouldn't be able to remove it when the host is powered
> off (which I assume you'd want).
>
> In terms of userspace/kernel separation, it's overall pretty similar to
> how it had been previously, just with the management of device lifetimes
> (instantiation & removal) moved to hwmontempsensor instead of
> entity-manager.
>
> Hopefully that's clear?  Let me know if you're having trouble getting
> things working though.
>
>
> Zev
>
>

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