hwmon driver with misc interface

hwmon driver with misc interface

[Benjamin Herrenschmidt]

Hi Guenter !

I'm looking at a driver we're currently keeping out of tree which I
want to rework a bit and submit upstream. (You may have seen earlier
versions of it, it's the POWER8/POWER9 "OCC" driver). I have a couple
of design questions however, which I'd like to discuss with you before
I make choices that you may or may not accept.

Background: the OCC is a microcontroller inside a POWER8 or POWER9
processor which controls various power management functions of the
chip, and monitor various chip internal metrics including temperature.

The driver is for the kernel of the BMC (the management controller),
typically a little ARM SoC, which needs to communicate with the above
OCC on the main processor, to perform various system power and thermal
management tasks.

Our current out-of-tree driver has a "common" part which exposes all of
the OCC "sensors" via hwmon, and two backends, one for P8 and one for
P9, handling the communication with the actual OCC implementation.

The backend gets instanciated from the device-tree and brings the
common parts in, so far it's rather standard.

The communication on POWER8 goes via i2c which is rather simple. There
is no other channel and all is good.

On POWER9, it however uses our "FSI" interface (drivers/fsi), more
specifically it's stacked on top of another driver (fsi-sbefifo.c, not
yet upstream, working on it too) which provides the communication with
the actual OCC (it's much faster than i2c). In addition, on POWER9, we
also need a separate userspace interface to the OCC for various
management tasks beyond the simple sensors, which we provide via a misc
chardev (/dev/occ*).

The current design of the driver is a bit convoluted though:

 - The sbefifo (transport controller if you want) creates a child
platform device for the occ which represents its ability to communicate
with the occ.

 - A first driver (fsi-occ) binds to that, it's not an hwmon driver. It
provides the /dev/occ* interface, and exports in-kernel functions for
sending commands and receiving responses from the OCC (simple wrappers
on top of the sbefifo). It also creates another platform device
underneath itself.

 - A second driver (occ-hwmon) then binds to that and uses the above
exported functions to communicate with the occ (this is the POWER9
backend of the hwmon driver).

This is all a bit convoluted and I think I can simplify it quite a bit
by removing the fsi-occ.c layer. The "wrappers" for sending commands to
the OCC via sbefifo aren't particularily useful, the POWER9 hwmon occ
backend could do that directly like the POWER8 one directly formats the
i2c commands.

However, I would have to bring the misc device into the hwmon driver.

Do you have a policy against this ? I noticed a couple of drivers in
drivers/hwmon already do something similar, but I wanted to double
check.

Additionally, there's another problem for which I'm not entirely fan of
our solution... The OCC isn't always there. IE, when the server is
powered off, the POWER9 chip is off, thus one cannot communicate with
the OCC. Only after it's powered on and has gone sufficiently far
during boot so that the POWER9 firmware has started the OCC can we
communicate with it.

Currently, what happens is that the occ-hwmon driver fails to bind when
the server is off (in my latest code base due to specific error codes
from sbefifo). Later on, the BMC userspace who monitors the system
state gets notified via an IPMI message by the POWER9 firmware that the
OCCs are up, and manually re-binds the driver.

It works ... but I'm not fan of it. Do you have a better suggestion ?
Should we keep the driver bound but error out on the values ? Should we
play with the "power state" of the device ? What's the policy when
accessing hwmon for a device in "suspended" state ?

Thanks for your time,

Cheers,
Ben.

Re: hwmon driver with misc interface

[Benjamin Herrenschmidt]

On Tue, 2018-05-22 at 09:31 +1000, Benjamin Herrenschmidt wrote:
> I'm looking at a driver we're currently keeping out of tree which I
> want to rework a bit and submit upstream. (You may have seen earlier
> versions of it, it's the POWER8/POWER9 "OCC" driver). I have a couple
> of design questions however, which I'd like to discuss with you before
> I make choices that you may or may not accept.

I also found some of the earlier threads on the matter and notice you
weren't fan of the fact that we expose other attributes in sysfs that
aren't per-se hwmon.

This is a bit of a connundrum though.

There's a single communication channel providing all the values for
both the "HW sensors" and the more high level synthetic informations
provided by the OCC such as its status, whether it's throttling
the main processor etc...

It's all coming in via the same structure, under the same lock etc...
and it would make little sense to explode that into many drivers.

Now it is not unheard of to have a single driver expose multiple
interfaces without creating multiple "devices" in sysfs.

In this specific case (OCC) we essentially have these "interfaces":

 - Actual sensors (thermal, power, etc...)

 - Additional attributes going with each sensor, some corresponding to
attributes defined in sysfs-interface (_label, _fault, ...) , some are
specific to our chips (_fru_type, _accumulator, _update_time..). I
understand you aren't fan of our "specific" ones, but some of them are
necessary for us, and since they are sensor specific attributes,
putting them elsewhere doesn't make much sense. Should we continue as
we have done and just document them ? Should we "prefix" the names with
something like an "vendor" prefix like we do in the device-tree to
avoid namespace clashes ?

 - General information from the OCC. This is one of your point of
contention. This is just currently 8 values we expose to userspace, so
a separate driver for that seems rather overkill, this includes things
like whether this is the master OCC (vs. slave OCC, ie multi-socket
machines have an OCC in each socket), the state of the OCC, whether
it's throttling the system memory, etc... All these come along with the
sensor values in the "poll" response from the OCC.

 - On POWER9, what I described in my previous email, a chardev
interface to send various commands to the OCC. Because these go via the
same channel used to poll the sensor values, it needs to be done by the
same kernel component. Currently a separate driver but it would
simplify things a lot (and reduce code & memory footprint) to make it
simply another interface exposed by the same driver as it's a rather
trivial read/write chardev.

Now, we really want to get to a point where we can upstream this, so I
want to work with you to find an acceptable design for all this, while
keeping in mind this is all running on a rather small and slow SoC, so
we don't want to "bloat" things if it can be avoided. Also simpler code
has less bugs.

Cheers,
Ben.

Re: hwmon driver with misc interface

[Guenter Roeck]

Ben,

first, sorry for dropping the ball on this one.

On 05/21/2018 06:36 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2018-05-22 at 09:31 +1000, Benjamin Herrenschmidt wrote:
>> I'm looking at a driver we're currently keeping out of tree which I
>> want to rework a bit and submit upstream. (You may have seen earlier
>> versions of it, it's the POWER8/POWER9 "OCC" driver). I have a couple
>> of design questions however, which I'd like to discuss with you before
>> I make choices that you may or may not accept.
> 
> I also found some of the earlier threads on the matter and notice you
> weren't fan of the fact that we expose other attributes in sysfs that
> aren't per-se hwmon.
> 
> This is a bit of a connundrum though.
> 
> There's a single communication channel providing all the values for
> both the "HW sensors" and the more high level synthetic informations
> provided by the OCC such as its status, whether it's throttling
> the main processor etc...
> 
> It's all coming in via the same structure, under the same lock etc...
> and it would make little sense to explode that into many drivers.
> 
> Now it is not unheard of to have a single driver expose multiple
> interfaces without creating multiple "devices" in sysfs.
> 
> In this specific case (OCC) we essentially have these "interfaces":
> 
>   - Actual sensors (thermal, power, etc...)
> 
>   - Additional attributes going with each sensor, some corresponding to
> attributes defined in sysfs-interface (_label, _fault, ...) , some are
> specific to our chips (_fru_type, _accumulator, _update_time..). I
> understand you aren't fan of our "specific" ones, but some of them are
> necessary for us, and since they are sensor specific attributes,
> putting them elsewhere doesn't make much sense. Should we continue as
> we have done and just document them ? Should we "prefix" the names with
> something like an "vendor" prefix like we do in the device-tree to
> avoid namespace clashes ?
> 
>   - General information from the OCC. This is one of your point of
> contention. This is just currently 8 values we expose to userspace, so
> a separate driver for that seems rather overkill, this includes things
> like whether this is the master OCC (vs. slave OCC, ie multi-socket
> machines have an OCC in each socket), the state of the OCC, whether
> it's throttling the system memory, etc... All these come along with the
> sensor values in the "poll" response from the OCC.
> 
>   - On POWER9, what I described in my previous email, a chardev
> interface to send various commands to the OCC. Because these go via the
> same channel used to poll the sensor values, it needs to be done by the
> same kernel component. Currently a separate driver but it would
> simplify things a lot (and reduce code & memory footprint) to make it
> simply another interface exposed by the same driver as it's a rather
> trivial read/write chardev.
> 
> Now, we really want to get to a point where we can upstream this, so I
> want to work with you to find an acceptable design for all this, while
> keeping in mind this is all running on a rather small and slow SoC, so
> we don't want to "bloat" things if it can be avoided. Also simpler code
> has less bugs.
> 

Trying to be be reasonable....

Let's make some ground rules.

- Do not attach foreign attributes (not related to hardware monitoring) to
   the hwmon device. Attach foreign attributes to its parent, eg the platform
   or i2c driver, or to a separate (misc ?) device if that is not feasible
   for some reason.
- Avoid foreign subsystem drivers. If the chip has an input device, a watchdog,
   and a hardware monitor, there should be three drivers.
   This is to some degree flexible; for example, PMBus drivers may register
   as power regulators, and some chips also have gpio support. But what,
   for example, the applesmc driver does is really not acceptable.
- Private hwmon attributes are acceptable as long as they are clearly
   documented and explained as necessary. This is not a free ride; you should
   have good reasons for private attributes and be able to explain that and why
   you need them. In this context, "because the hardware provides the information"
   is not a valid reason. The use case is important, not the fact that
   the hardware provides some random information.

Can you work with that ?

Thanks,
Guenter

Re: hwmon driver with misc interface

[Benjamin Herrenschmidt]

On Sun, 2018-07-08 at 16:30 -0700, Guenter Roeck wrote:
> 
> Trying to be be reasonable....
> 
> Let's make some ground rules.
> 
> - Do not attach foreign attributes (not related to hardware monitoring) to
>    the hwmon device. Attach foreign attributes to its parent, eg the platform
>    or i2c driver, or to a separate (misc ?) device if that is not feasible
>    for some reason.
> - Avoid foreign subsystem drivers. If the chip has an input device, a watchdog,
>    and a hardware monitor, there should be three drivers.
>    This is to some degree flexible; for example, PMBus drivers may register
>    as power regulators, and some chips also have gpio support. But what,
>    for example, the applesmc driver does is really not acceptable.

This rule can be a bit nasty if the various "parts" of the chip need
tight interlock, share an interrupt etc... the solution to that is to
have most of the common code in a "parent" driver that creates child
devices with separate drivers that directly link onto the parent and
use exported functions, but it can easily bloat the driver
significantly for little benefit.

That said, this is maybe not *too much* of an issue in the OCC case,
see below.

> - Private hwmon attributes are acceptable as long as they are clearly
>    documented and explained as necessary. This is not a free ride; you should
>    have good reasons for private attributes and be able to explain that and why
>    you need them. In this context, "because the hardware provides the information"
>    is not a valid reason. The use case is important, not the fact that
>    the hardware provides some random information.
> 
> Can you work with that ?

Anything is always possible :-) The main question for me here is
whether to keep what we do today:

	* sbefifo (the transport driver)
	  |
	  * fsi-occ platform driver
            ("passes occ hwmon commands to sbefifo and adds /dev/occ")
             |
             * occ-hwmon

Or can I collapse fsi-occ and occ-hwmon into one.

Now /dev/occ is just a "raw" interface to send commands to the OCC, via
the same path occ-hwmon does. There's locking needed there between the
two so it currently happens in fsi-occ.

>From what you are saying, you prefer that we keep it separate, which is
our current design. I find it a bit messy but it's not a huge deal
frankly, so let's do so.

The pre-requisite sbefifo driver is now in Greg's tree (and I'll have
some fixes for it in -next this week), so you should be able to at
least test build when Eddie resubmits.

As for the various sysfs files for monitoring the base functionality of
the occ, Eddie, you can always move them to fsi-occ.

Cheers,
Ben.

Re: hwmon driver with misc interface

[Guenter Roeck]

On 07/08/2018 06:05 PM, Benjamin Herrenschmidt wrote:
> On Sun, 2018-07-08 at 16:30 -0700, Guenter Roeck wrote:
>>
>> Trying to be be reasonable....
>>
>> Let's make some ground rules.
>>
>> - Do not attach foreign attributes (not related to hardware monitoring) to
>>     the hwmon device. Attach foreign attributes to its parent, eg the platform
>>     or i2c driver, or to a separate (misc ?) device if that is not feasible
>>     for some reason.
>> - Avoid foreign subsystem drivers. If the chip has an input device, a watchdog,
>>     and a hardware monitor, there should be three drivers.
>>     This is to some degree flexible; for example, PMBus drivers may register
>>     as power regulators, and some chips also have gpio support. But what,
>>     for example, the applesmc driver does is really not acceptable.
> 
> This rule can be a bit nasty if the various "parts" of the chip need
> tight interlock, share an interrupt etc... the solution to that is to
> have most of the common code in a "parent" driver that creates child
> devices with separate drivers that directly link onto the parent and
> use exported functions, but it can easily bloat the driver
> significantly for little benefit.
> 

But that is what mfd drivers are for, or am I missing something ?
After all, it has "multi-function device" right in its name.

Sure, there is somebloat, but on the plus side it ensures that
all bits and pieces are reviewed by the respective maintainers,
and the cross-functional API is _forced_ to be clean.

> That said, this is maybe not *too much* of an issue in the OCC case,
> see below.
> 
>> - Private hwmon attributes are acceptable as long as they are clearly
>>     documented and explained as necessary. This is not a free ride; you should
>>     have good reasons for private attributes and be able to explain that and why
>>     you need them. In this context, "because the hardware provides the information"
>>     is not a valid reason. The use case is important, not the fact that
>>     the hardware provides some random information.
>>
>> Can you work with that ?
> 
> Anything is always possible :-) The main question for me here is
> whether to keep what we do today:
> 
> 	* sbefifo (the transport driver)
> 	  |
> 	  * fsi-occ platform driver
>              ("passes occ hwmon commands to sbefifo and adds /dev/occ")
>               |
>               * occ-hwmon
> 
> Or can I collapse fsi-occ and occ-hwmon into one.
> 
> Now /dev/occ is just a "raw" interface to send commands to the OCC, via
> the same path occ-hwmon does. There's locking needed there between the
> two so it currently happens in fsi-occ.
> 
>>From what you are saying, you prefer that we keep it separate, which is
> our current design. I find it a bit messy but it's not a huge deal
> frankly, so let's do so.
> 
Other drivers solve that with an API from parent to child, either with
a direct function call or with a callback function provided to the child.
Another option would be to handle it through regmap; That nowadays
supports custom accesses implemented in the parent driver (see regmap_read
and regmap_write in struct regmap_config). The child driver gets the
regmap pointer and uses the regmap API. I don't see that as messy.

> The pre-requisite sbefifo driver is now in Greg's tree (and I'll have
> some fixes for it in -next this week), so you should be able to at
> least test build when Eddie resubmits.
> 
Ok.

> As for the various sysfs files for monitoring the base functionality of
> the occ, Eddie, you can always move them to fsi-occ.
> 

Yes, I think that would be more appropriate.

Thanks,
Guenter

> Cheers,
> Ben.
> 

Re: hwmon driver with misc interface

[Benjamin Herrenschmidt]

On Sun, 2018-07-08 at 18:26 -0700, Guenter Roeck wrote:
> On 07/08/2018 06:05 PM, Benjamin Herrenschmidt wrote:
> > On Sun, 2018-07-08 at 16:30 -0700, Guenter Roeck wrote:
> > > 
> > > Trying to be be reasonable....
> > > 
> > > Let's make some ground rules.
> > > 
> > > - Do not attach foreign attributes (not related to hardware monitoring) to
> > >     the hwmon device. Attach foreign attributes to its parent, eg the platform
> > >     or i2c driver, or to a separate (misc ?) device if that is not feasible
> > >     for some reason.
> > > - Avoid foreign subsystem drivers. If the chip has an input device, a watchdog,
> > >     and a hardware monitor, there should be three drivers.
> > >     This is to some degree flexible; for example, PMBus drivers may register
> > >     as power regulators, and some chips also have gpio support. But what,
> > >     for example, the applesmc driver does is really not acceptable.
> > 
> > This rule can be a bit nasty if the various "parts" of the chip need
> > tight interlock, share an interrupt etc... the solution to that is to
> > have most of the common code in a "parent" driver that creates child
> > devices with separate drivers that directly link onto the parent and
> > use exported functions, but it can easily bloat the driver
> > significantly for little benefit.
> > 
> 
> But that is what mfd drivers are for, or am I missing something ?
> After all, it has "multi-function device" right in its name.

The mfd "framework" looks like just gratuitous bloat to me frankly, but
I won't fight a battle with you on that onne :-)

> Sure, there is somebloat, but on the plus side it ensures that
> all bits and pieces are reviewed by the respective maintainers,
> and the cross-functional API is _forced_ to be clean.

Ok. In this case I don't think it qualifies as an mfd really. There's
no real "other function", the chardev is mostly a lower level interface
for userspace to monitor the health of the OCC itself, etc.... 

> > That said, this is maybe not *too much* of an issue in the OCC case,
> > see below.
> > 
> > > - Private hwmon attributes are acceptable as long as they are clearly
> > >     documented and explained as necessary. This is not a free ride; you should
> > >     have good reasons for private attributes and be able to explain that and why
> > >     you need them. In this context, "because the hardware provides the information"
> > >     is not a valid reason. The use case is important, not the fact that
> > >     the hardware provides some random information.
> > > 
> > > Can you work with that ?
> > 
> > Anything is always possible :-) The main question for me here is
> > whether to keep what we do today:
> > 
> > 	* sbefifo (the transport driver)
> > 	  |
> > 	  * fsi-occ platform driver
> >              ("passes occ hwmon commands to sbefifo and adds /dev/occ")
> >               |
> >               * occ-hwmon
> > 
> > Or can I collapse fsi-occ and occ-hwmon into one.
> > 
> > Now /dev/occ is just a "raw" interface to send commands to the OCC, via
> > the same path occ-hwmon does. There's locking needed there between the
> > two so it currently happens in fsi-occ.
> > 
> > > From what you are saying, you prefer that we keep it separate, which is
> > 
> > our current design. I find it a bit messy but it's not a huge deal
> > frankly, so let's do so.
> > 
> 
> Other drivers solve that with an API from parent to child, either with
> a direct function call or with a callback function provided to the child.

Yes, that's exactly what our current one does. So I suspect it's fine
by you. I was just wondering whether I could "flatten" the OCC chardev
& glue with the hwmon one but it looks like you won't like it so we'll
keep it as-is.

> Another option would be to handle it through regmap; That nowadays
> supports custom accesses implemented in the parent driver (see regmap_read
> and regmap_write in struct regmap_config). The child driver gets the
> regmap pointer and uses the regmap API. I don't see that as messy.

That works for purely MMIO based things, this isn't though.

> > The pre-requisite sbefifo driver is now in Greg's tree (and I'll have
> > some fixes for it in -next this week), so you should be able to at
> > least test build when Eddie resubmits.
> > 
> 
> Ok.
> 
> > As for the various sysfs files for monitoring the base functionality of
> > the occ, Eddie, you can always move them to fsi-occ.
> > 
> 
> Yes, I think that would be more appropriate.

Thanks for your replies !

Cheers,
Ben.

> Thanks,
> Guenter
> 
> > Cheers,
> > Ben.
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: hwmon driver with misc interface

[Guenter Roeck]

On 07/08/2018 11:04 PM, Benjamin Herrenschmidt wrote:

>> Another option would be to handle it through regmap; That nowadays
>> supports custom accesses implemented in the parent driver (see regmap_read
>> and regmap_write in struct regmap_config). The child driver gets the
>> regmap pointer and uses the regmap API. I don't see that as messy.
> 
> That works for purely MMIO based things, this isn't though.
> 

Sorry, I meant reg_read and reg_write callbacks in struct regmap_config.
I don't think those callbacks are limited to mmio; I find many other
access types by browsing through code providing those callbacks.

Sure, one could consider regmap to be another bloat, but I find it to
be a quite convenient abstraction layer.

Guenter

Re: hwmon driver with misc interface

[Benjamin Herrenschmidt]

On Sun, 2018-07-08 at 23:37 -0700, Guenter Roeck wrote:
> On 07/08/2018 11:04 PM, Benjamin Herrenschmidt wrote:
> 
> > > Another option would be to handle it through regmap; That nowadays
> > > supports custom accesses implemented in the parent driver (see regmap_read
> > > and regmap_write in struct regmap_config). The child driver gets the
> > > regmap pointer and uses the regmap API. I don't see that as messy.
> > 
> > That works for purely MMIO based things, this isn't though.
> > 
> 
> Sorry, I meant reg_read and reg_write callbacks in struct regmap_config.
> I don't think those callbacks are limited to mmio; I find many other
> access types by browsing through code providing those callbacks.
> 
> Sure, one could consider regmap to be another bloat, but I find it to
> be a quite convenient abstraction layer.

It has been for some things. We use it in some of our drivers.

In the specific case of OCC, it's a bit more complex, we need to send
potentially fairly large "commands" to the SBE via the sbefifo driver
and receive the corresponding response in a buffer, so we have an API
between the drivers, which works well.

Cheers,
Ben.

Re: hwmon driver with misc interface

[Eddie James]

On 07/08/2018 08:26 PM, Guenter Roeck wrote:
> On 07/08/2018 06:05 PM, Benjamin Herrenschmidt wrote:
>> On Sun, 2018-07-08 at 16:30 -0700, Guenter Roeck wrote:
>>>
>>> Trying to be be reasonable....
>>>
>>> Let's make some ground rules.
>>>
>>> - Do not attach foreign attributes (not related to hardware 
>>> monitoring) to
>>>     the hwmon device. Attach foreign attributes to its parent, eg 
>>> the platform
>>>     or i2c driver, or to a separate (misc ?) device if that is not 
>>> feasible
>>>     for some reason.
>>> - Avoid foreign subsystem drivers. If the chip has an input device, 
>>> a watchdog,
>>>     and a hardware monitor, there should be three drivers.
>>>     This is to some degree flexible; for example, PMBus drivers may 
>>> register
>>>     as power regulators, and some chips also have gpio support. But 
>>> what,
>>>     for example, the applesmc driver does is really not acceptable.
>>
>> This rule can be a bit nasty if the various "parts" of the chip need
>> tight interlock, share an interrupt etc... the solution to that is to
>> have most of the common code in a "parent" driver that creates child
>> devices with separate drivers that directly link onto the parent and
>> use exported functions, but it can easily bloat the driver
>> significantly for little benefit.
>>
>
> But that is what mfd drivers are for, or am I missing something ?
> After all, it has "multi-function device" right in its name.
>
> Sure, there is somebloat, but on the plus side it ensures that
> all bits and pieces are reviewed by the respective maintainers,
> and the cross-functional API is _forced_ to be clean.
>
>> That said, this is maybe not *too much* of an issue in the OCC case,
>> see below.
>>
>>> - Private hwmon attributes are acceptable as long as they are clearly
>>>     documented and explained as necessary. This is not a free ride; 
>>> you should
>>>     have good reasons for private attributes and be able to explain 
>>> that and why
>>>     you need them. In this context, "because the hardware provides 
>>> the information"
>>>     is not a valid reason. The use case is important, not the fact that
>>>     the hardware provides some random information.
>>>
>>> Can you work with that ?
>>
>> Anything is always possible :-) The main question for me here is
>> whether to keep what we do today:
>>
>>     * sbefifo (the transport driver)
>>       |
>>       * fsi-occ platform driver
>>              ("passes occ hwmon commands to sbefifo and adds /dev/occ")
>>               |
>>               * occ-hwmon
>>
>> Or can I collapse fsi-occ and occ-hwmon into one.
>>
>> Now /dev/occ is just a "raw" interface to send commands to the OCC, via
>> the same path occ-hwmon does. There's locking needed there between the
>> two so it currently happens in fsi-occ.
>>
>>> From what you are saying, you prefer that we keep it separate, which is
>> our current design. I find it a bit messy but it's not a huge deal
>> frankly, so let's do so.
>>
> Other drivers solve that with an API from parent to child, either with
> a direct function call or with a callback function provided to the child.
> Another option would be to handle it through regmap; That nowadays
> supports custom accesses implemented in the parent driver (see 
> regmap_read
> and regmap_write in struct regmap_config). The child driver gets the
> regmap pointer and uses the regmap API. I don't see that as messy.
>
>> The pre-requisite sbefifo driver is now in Greg's tree (and I'll have
>> some fixes for it in -next this week), so you should be able to at
>> least test build when Eddie resubmits.
>>
> Ok.
>
>> As for the various sysfs files for monitoring the base functionality of
>> the occ, Eddie, you can always move them to fsi-occ.
>>
>
> Yes, I think that would be more appropriate.

This still won't work, since then we wouldn't have those attributes 
available in the P8 version of the driver (which has no fsi-occ driver). 
In addition, how would the poll response data get from the hwmon driver 
to the fsi-occ driver? Yet another interface? Seems awkward.

How about debugfs? We don't really mind where the attributes are, just 
that the data is exposed somewhere...

Thanks,
Eddie

>
> Thanks,
> Guenter
>
>> Cheers,
>> Ben.
>>
>

Re: hwmon driver with misc interface

[Guenter Roeck]

On Tue, Jul 10, 2018 at 03:04:33PM -0500, Eddie James wrote:
> 
> 
> On 07/08/2018 08:26 PM, Guenter Roeck wrote:
> >On 07/08/2018 06:05 PM, Benjamin Herrenschmidt wrote:
> >>On Sun, 2018-07-08 at 16:30 -0700, Guenter Roeck wrote:
> >>>
> >>>Trying to be be reasonable....
> >>>
> >>>Let's make some ground rules.
> >>>
> >>>- Do not attach foreign attributes (not related to hardware
> >>>monitoring) to
> >>>    the hwmon device. Attach foreign attributes to its parent, eg the
> >>>platform
> >>>    or i2c driver, or to a separate (misc ?) device if that is not
> >>>feasible
> >>>    for some reason.
> >>>- Avoid foreign subsystem drivers. If the chip has an input device, a
> >>>watchdog,
> >>>    and a hardware monitor, there should be three drivers.
> >>>    This is to some degree flexible; for example, PMBus drivers may
> >>>register
> >>>    as power regulators, and some chips also have gpio support. But
> >>>what,
> >>>    for example, the applesmc driver does is really not acceptable.
> >>
> >>This rule can be a bit nasty if the various "parts" of the chip need
> >>tight interlock, share an interrupt etc... the solution to that is to
> >>have most of the common code in a "parent" driver that creates child
> >>devices with separate drivers that directly link onto the parent and
> >>use exported functions, but it can easily bloat the driver
> >>significantly for little benefit.
> >>
> >
> >But that is what mfd drivers are for, or am I missing something ?
> >After all, it has "multi-function device" right in its name.
> >
> >Sure, there is somebloat, but on the plus side it ensures that
> >all bits and pieces are reviewed by the respective maintainers,
> >and the cross-functional API is _forced_ to be clean.
> >
> >>That said, this is maybe not *too much* of an issue in the OCC case,
> >>see below.
> >>
> >>>- Private hwmon attributes are acceptable as long as they are clearly
> >>>    documented and explained as necessary. This is not a free ride;
> >>>you should
> >>>    have good reasons for private attributes and be able to explain
> >>>that and why
> >>>    you need them. In this context, "because the hardware provides the
> >>>information"
> >>>    is not a valid reason. The use case is important, not the fact that
> >>>    the hardware provides some random information.
> >>>
> >>>Can you work with that ?
> >>
> >>Anything is always possible :-) The main question for me here is
> >>whether to keep what we do today:
> >>
> >>    * sbefifo (the transport driver)
> >>      |
> >>      * fsi-occ platform driver
> >>             ("passes occ hwmon commands to sbefifo and adds /dev/occ")
> >>              |
> >>              * occ-hwmon
> >>
> >>Or can I collapse fsi-occ and occ-hwmon into one.
> >>
> >>Now /dev/occ is just a "raw" interface to send commands to the OCC, via
> >>the same path occ-hwmon does. There's locking needed there between the
> >>two so it currently happens in fsi-occ.
> >>
> >>>From what you are saying, you prefer that we keep it separate, which is
> >>our current design. I find it a bit messy but it's not a huge deal
> >>frankly, so let's do so.
> >>
> >Other drivers solve that with an API from parent to child, either with
> >a direct function call or with a callback function provided to the child.
> >Another option would be to handle it through regmap; That nowadays
> >supports custom accesses implemented in the parent driver (see regmap_read
> >and regmap_write in struct regmap_config). The child driver gets the
> >regmap pointer and uses the regmap API. I don't see that as messy.
> >
> >>The pre-requisite sbefifo driver is now in Greg's tree (and I'll have
> >>some fixes for it in -next this week), so you should be able to at
> >>least test build when Eddie resubmits.
> >>
> >Ok.
> >
> >>As for the various sysfs files for monitoring the base functionality of
> >>the occ, Eddie, you can always move them to fsi-occ.
> >>
> >
> >Yes, I think that would be more appropriate.
> 
> This still won't work, since then we wouldn't have those attributes
> available in the P8 version of the driver (which has no fsi-occ driver). In
> addition, how would the poll response data get from the hwmon driver to the
> fsi-occ driver? Yet another interface? Seems awkward.
> 
> How about debugfs? We don't really mind where the attributes are, just that
> the data is exposed somewhere...
> 
You are essentially confirming that using sysfs attributes would not be
appropriate. I have no problems with using debugfs; you have a free ride
there.

Guenter

Re: hwmon driver with misc interface

[Benjamin Herrenschmidt]

On Tue, 2018-07-10 at 13:44 -0700, Guenter Roeck wrote:
> > > Yes, I think that would be more appropriate.
> > 
> > This still won't work, since then we wouldn't have those attributes
> > available in the P8 version of the driver (which has no fsi-occ driver). In
> > addition, how would the poll response data get from the hwmon driver to the
> > fsi-occ driver? Yet another interface? Seems awkward.
> > 
> > How about debugfs? We don't really mind where the attributes are, just that
> > the data is exposed somewhere...
> > 
> 
> You are essentially confirming that using sysfs attributes would not be
> appropriate. I have no problems with using debugfs; you have a free ride
> there.

I disagree.

If the attributes are used for the normal operation of the system then
they should be in sysfs.

A system should be able to function normally without debugfs mounted.

Those attributes are about monitoring proper operations of the OCC
right ? Or are there other things here ? I don't see any reason why
those couldn't hang off the device sysfs node...

Cheers,
Ben.

Re: hwmon driver with misc interface

[Benjamin Herrenschmidt]

On Wed, 2018-07-11 at 09:26 +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2018-07-10 at 13:44 -0700, Guenter Roeck wrote:
> > > > Yes, I think that would be more appropriate.
> > > 
> > > This still won't work, since then we wouldn't have those attributes
> > > available in the P8 version of the driver (which has no fsi-occ driver). In
> > > addition, how would the poll response data get from the hwmon driver to the
> > > fsi-occ driver? Yet another interface? Seems awkward.
> > > 
> > > How about debugfs? We don't really mind where the attributes are, just that
> > > the data is exposed somewhere...
> > > 
> > 
> > You are essentially confirming that using sysfs attributes would not be
> > appropriate. I have no problems with using debugfs; you have a free ride
> > there.
> 
> I disagree.
> 
> If the attributes are used for the normal operation of the system then
> they should be in sysfs.
> 
> A system should be able to function normally without debugfs mounted.
> 
> Those attributes are about monitoring proper operations of the OCC
> right ? Or are there other things here ? I don't see any reason why
> those couldn't hang off the device sysfs node...

Eddie: If Guenter really strongly objects to that handful of attributes
being in the hwmon device itself, then we have two alternatives we can
consider:

  - Not upstream P8 :-)

  - Use a similar 2-level driver for P8/i2c, which additionally
provides the ability to create a p8 variant of /dev/occ if needed.

I don't like the way the P8 backend works today anyway. It basically
uses i2c to do SCOMs which will race/clash with anything else in the
system trying to do the same.

We should ideally have a pure "SCOM" driver providing a P8 /dev/scom
and lay on top of that a platform device for OCC like we do with
sbefifo.

That said, I'm also not too fussed about leaving P8 behind as legacy
and not upstreaming it.

Cheers,
Ben.
> 
> Cheers,
> Ben.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: hwmon driver with misc interface

[Guenter Roeck]

On 07/10/2018 07:54 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2018-07-11 at 09:26 +1000, Benjamin Herrenschmidt wrote:
>> On Tue, 2018-07-10 at 13:44 -0700, Guenter Roeck wrote:
>>>>> Yes, I think that would be more appropriate.
>>>>
>>>> This still won't work, since then we wouldn't have those attributes
>>>> available in the P8 version of the driver (which has no fsi-occ driver). In
>>>> addition, how would the poll response data get from the hwmon driver to the
>>>> fsi-occ driver? Yet another interface? Seems awkward.
>>>>
>>>> How about debugfs? We don't really mind where the attributes are, just that
>>>> the data is exposed somewhere...
>>>>
>>>
>>> You are essentially confirming that using sysfs attributes would not be
>>> appropriate. I have no problems with using debugfs; you have a free ride
>>> there.
>>
>> I disagree.
>>
>> If the attributes are used for the normal operation of the system then
>> they should be in sysfs.
>>
>> A system should be able to function normally without debugfs mounted.
>>
>> Those attributes are about monitoring proper operations of the OCC
>> right ? Or are there other things here ? I don't see any reason why
>> those couldn't hang off the device sysfs node...
> 
> Eddie: If Guenter really strongly objects to that handful of attributes

Please keep in mind that you are not making a string case for those attributes,
whatever they are. Saying that debugfs may be a solution isn't really a strong
case; it is exactly the opposite.

I am pushing back because I have seen too many "I want to have this
attribute because the HW supports it. I have no clue what to use it
for, I just want it". I'll want to see something better than that.

Guenter

> being in the hwmon device itself, then we have two alternatives we can
> consider:
> 
>    - Not upstream P8 :-)
> 
>    - Use a similar 2-level driver for P8/i2c, which additionally
> provides the ability to create a p8 variant of /dev/occ if needed.
> 
> I don't like the way the P8 backend works today anyway. It basically
> uses i2c to do SCOMs which will race/clash with anything else in the
> system trying to do the same.
> 
> We should ideally have a pure "SCOM" driver providing a P8 /dev/scom
> and lay on top of that a platform device for OCC like we do with
> sbefifo.
> 
> That said, I'm also not too fussed about leaving P8 behind as legacy
> and not upstreaming it.
> 
> Cheers,
> Ben.
>>
>> Cheers,
>> Ben.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>