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