[pmbusplus] userspace i2c, pmbus interactions

[pmbusplus] userspace i2c, pmbus interactions

[Jason Ling]

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

Hi all,

What started as an attempt to create a simple command line tool to perform
pmbus device upgrades over i2c has turned into the start of a user-space
i2c library (with some pmbus support).

I've already reused this library in some other obmc applications and it's
been fairly well unit-tested. It also comes with all the public interfaces
mocked (so you can unit test your own code).

The idea is that more and more classes get added that will support
different pmbus devices.
General idea is that each device that gets support can expose methods to
allow device upgrade, black box retrieval, etc..

Anyways, wanted to gauge community interest in this so I can determine how
motivated I should be to upstream it.

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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Andrew Jeffery]

On Sat, 24 Apr 2021, at 07:52, Jason Ling wrote:
> Hi all,
> 
> What started as an attempt to create a simple command line tool to 
> perform pmbus device upgrades over i2c has turned into the start of a 
> user-space i2c library (with some pmbus support).
> 
> I've already reused this library in some other obmc applications and 
> it's been fairly well unit-tested. It also comes with all the public 
> interfaces mocked (so you can unit test your own code).
> 
> The idea is that more and more classes get added that will support 
> different pmbus devices. 
> General idea is that each device that gets support can expose methods 
> to allow device upgrade, black box retrieval, etc..
> 
> Anyways, wanted to gauge community interest in this so I can determine 
> how motivated I should be to upstream it.

How does this interact with the efforts in the phosphor-power repo?

https://github.com/openbmc/phosphor-power

Can we exploit your library there? Can your library utilise work from 
phosphor-power?

Andrew

Re: [pmbusplus] userspace i2c, pmbus interactions

[Mike Jones]

Jason,

I am interested, because within the PMBus Specification Committee, we are working on a data language intended for device programming. And there is hope that eventually it can become adopted into linux and/or OBMC.

There is a particular use model that is being driven by the IC suppliers and their tools. One reason is that all the vendors have proprietary tools, but they see no competitive advantage, and would rather support a universal standard.

Imagine that programming might be done for:

- ICT
- Proto Builds
- Engineering Bringup
- Remote upgrades

And the context is more than CPU based systems, but includes Networking, other boards with ASICS, etc. So broad context. Hence, it has to work within linux without OBMC.

My view is it is a linux library anyone can use, and OBMC is the piping if it were exposed to a web service, state management, etc.

Now, imagine the IC manufacturer's tool produces a file that can represent a qualified algorithm that is known to work under all possible scenarios, including CRC errors in parts, corrupt NVM, etc. This is what all the vendors do today. They take care of all the things that can go wrong. In the case of ADI, if power was lost while programming, and the BMC or linux can boot from an aux supply, our data files (encoding algorithms), can repair the part under ALL possible random values in the corrupt part.

Furthermore, an integrator (CM, Design House, software team) has to deal with segmented I2C busses, muxes, etc. And the integrator wants to write a wrapper file that integrates all the vendor files. So this integration file has to take care of muxes, order of operations, calling vendor files, etc.

My interest is two part:

1) I am interested in anything that enables our work
2) I am interested in inviting someone from the community, not IC vendor, to our meetings to offer advice and help us define something useful

Mike



> On Apr 23, 2021, at 4:22 PM, Jason Ling <jasonling@google.com> wrote:
> 
> Hi all,
> 
> What started as an attempt to create a simple command line tool to perform pmbus device upgrades over i2c has turned into the start of a user-space i2c library (with some pmbus support).
> 
> I've already reused this library in some other obmc applications and it's been fairly well unit-tested. It also comes with all the public interfaces mocked (so you can unit test your own code).
> 
> The idea is that more and more classes get added that will support different pmbus devices. 
> General idea is that each device that gets support can expose methods to allow device upgrade, black box retrieval, etc..
> 
> Anyways, wanted to gauge community interest in this so I can determine how motivated I should be to upstream it.
> 

Re: [pmbusplus] userspace i2c, pmbus interactions

[Jason Ling]

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

Thanks Mike for the feedback

And the context is more than CPU based systems, but includes Networking,
> other boards with ASICS, etc. So broad context. Hence, it has to work
> within linux without OBMC.

Ack, although the library was written for a use-case that involves obmc, it
doesn't *require* obmc. Should work just fine in general Linux.

 Now, imagine the IC manufacturer's tool produces a file that can represent
> a qualified algorithm that is known to work under all possible scenarios,
> including CRC errors in parts, corrupt NVM, etc. This is what all the
> vendors do today. They take care of all the things that can go wrong. In
> the case of ADI, if power was lost while programming, and the BMC or linux
> can boot from an aux supply, our data files (encoding algorithms), can
> repair the part under ALL possible random values in the corrupt part.

This would be great, especially if this is codified in the pmbus spec.
Right now the library provides a pmbus interface but *part programming* is
specific to each device class...no guarantee of a common interface across
multiple parts.

1) I am interested in anything that enables our work

Sure, I'll start carving out more time to make this work suitable for
upstreaming. At the very least it should give you a mockable interface to
allow for strong unit testing of upper layers.


> 2) I am interested in inviting someone from the community, not IC vendor,
> to our meetings to offer advice and help us define something useful

Sounds good, feel free to reach out to me on an individual basis.

On Mon, Apr 26, 2021 at 7:33 PM Mike Jones <proclivis@gmail.com> wrote:

> Jason,
>
> I am interested, because within the PMBus Specification Committee, we are
> working on a data language intended for device programming. And there is
> hope that eventually it can become adopted into linux and/or OBMC.
>
> There is a particular use model that is being driven by the IC suppliers
> and their tools. One reason is that all the vendors have proprietary tools,
> but they see no competitive advantage, and would rather support a universal
> standard.
>
> Imagine that programming might be done for:
>
> - ICT
> - Proto Builds
> - Engineering Bringup
> - Remote upgrades
>
> And the context is more than CPU based systems, but includes Networking,
> other boards with ASICS, etc. So broad context. Hence, it has to work
> within linux without OBMC.
>
> My view is it is a linux library anyone can use, and OBMC is the piping if
> it were exposed to a web service, state management, etc.
>
> Now, imagine the IC manufacturer's tool produces a file that can represent
> a qualified algorithm that is known to work under all possible scenarios,
> including CRC errors in parts, corrupt NVM, etc. This is what all the
> vendors do today. They take care of all the things that can go wrong. In
> the case of ADI, if power was lost while programming, and the BMC or linux
> can boot from an aux supply, our data files (encoding algorithms), can
> repair the part under ALL possible random values in the corrupt part.
>
> Furthermore, an integrator (CM, Design House, software team) has to deal
> with segmented I2C busses, muxes, etc. And the integrator wants to write a
> wrapper file that integrates all the vendor files. So this integration file
> has to take care of muxes, order of operations, calling vendor files, etc.
>
> My interest is two part:
>
> 1) I am interested in anything that enables our work
> 2) I am interested in inviting someone from the community, not IC vendor,
> to our meetings to offer advice and help us define something useful
>
> Mike
>
>
>
> > On Apr 23, 2021, at 4:22 PM, Jason Ling <jasonling@google.com> wrote:
> >
> > Hi all,
> >
> > What started as an attempt to create a simple command line tool to
> perform pmbus device upgrades over i2c has turned into the start of a
> user-space i2c library (with some pmbus support).
> >
> > I've already reused this library in some other obmc applications and
> it's been fairly well unit-tested. It also comes with all the public
> interfaces mocked (so you can unit test your own code).
> >
> > The idea is that more and more classes get added that will support
> different pmbus devices.
> > General idea is that each device that gets support can expose methods to
> allow device upgrade, black box retrieval, etc..
> >
> > Anyways, wanted to gauge community interest in this so I can determine
> how motivated I should be to upstream it.
> >
>
>

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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Mike Jones]

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

Jason

> On Apr 27, 2021, at 10:20 AM, Jason Ling <jasonling@google.com> wrote:
> 
> Thanks Mike for the feedback
> 
> And the context is more than CPU based systems, but includes Networking, other boards with ASICS, etc. So broad context. Hence, it has to work within linux without OBMC.
> Ack, although the library was written for a use-case that involves obmc, it doesn't require obmc. Should work just fine in general Linux.
> 
>  Now, imagine the IC manufacturer's tool produces a file that can represent a qualified algorithm that is known to work under all possible scenarios, including CRC errors in parts, corrupt NVM, etc. This is what all the vendors do today. They take care of all the things that can go wrong. In the case of ADI, if power was lost while programming, and the BMC or linux can boot from an aux supply, our data files (encoding algorithms), can repair the part under ALL possible random values in the corrupt part.
> This would be great, especially if this is codified in the pmbus spec. Right now the library provides a pmbus interface but part programming is specific to each device class...no guarantee of a common interface across multiple parts.

The vendors will never agree to an industry specification for programming beyond the standard STORE_USER_ALL. Most real world programming is MFR.

This is the reason for a description file, it enables each vendor to innovate, yet the end user can process the file with a single engine.

The problem with STORE_USER_ALL is if the power fails or the NVM fails.

For example, some companies just change a few commands, say voltage, and then store. But if power is lost during the store, the other 99 values are corrupted and need repair, and the part may not even have an address on the bus without special commands to reset a few things. Or it might be write protected. Or PEC might be required, Etc.

> 
> 1) I am interested in anything that enables our work
> Sure, I'll start carving out more time to make this work suitable for upstreaming. At the very least it should give you a mockable interface to allow for strong unit testing of upper layers.

ADI has code that can implement its proprietary programming via /dev/i2c. All that is needed is standard SMBus code. If the programming happens in user space, we imagined the data processing engine using that.

In that context, it would be interesting to know what you are doing, as you are adding value for sure. If there is an API that delineates the function implemented, it would be nice to review it so I can understand your work.

>  
> 2) I am interested in inviting someone from the community, not IC vendor, to our meetings to offer advice and help us define something useful
> Sounds good, feel free to reach out to me on an individual basis. 

Ok. Vivek is on the same list, and he can reach out for an official invite.

Mike
> 
> On Mon, Apr 26, 2021 at 7:33 PM Mike Jones <proclivis@gmail.com <mailto:proclivis@gmail.com>> wrote:
> Jason,
> 
> I am interested, because within the PMBus Specification Committee, we are working on a data language intended for device programming. And there is hope that eventually it can become adopted into linux and/or OBMC.
> 
> There is a particular use model that is being driven by the IC suppliers and their tools. One reason is that all the vendors have proprietary tools, but they see no competitive advantage, and would rather support a universal standard.
> 
> Imagine that programming might be done for:
> 
> - ICT
> - Proto Builds
> - Engineering Bringup
> - Remote upgrades
> 
> And the context is more than CPU based systems, but includes Networking, other boards with ASICS, etc. So broad context. Hence, it has to work within linux without OBMC.
> 
> My view is it is a linux library anyone can use, and OBMC is the piping if it were exposed to a web service, state management, etc.
> 
> Now, imagine the IC manufacturer's tool produces a file that can represent a qualified algorithm that is known to work under all possible scenarios, including CRC errors in parts, corrupt NVM, etc. This is what all the vendors do today. They take care of all the things that can go wrong. In the case of ADI, if power was lost while programming, and the BMC or linux can boot from an aux supply, our data files (encoding algorithms), can repair the part under ALL possible random values in the corrupt part.
> 
> Furthermore, an integrator (CM, Design House, software team) has to deal with segmented I2C busses, muxes, etc. And the integrator wants to write a wrapper file that integrates all the vendor files. So this integration file has to take care of muxes, order of operations, calling vendor files, etc.
> 
> My interest is two part:
> 
> 1) I am interested in anything that enables our work
> 2) I am interested in inviting someone from the community, not IC vendor, to our meetings to offer advice and help us define something useful
> 
> Mike
> 
> 
> 
> > On Apr 23, 2021, at 4:22 PM, Jason Ling <jasonling@google.com <mailto:jasonling@google.com>> wrote:
> > 
> > Hi all,
> > 
> > What started as an attempt to create a simple command line tool to perform pmbus device upgrades over i2c has turned into the start of a user-space i2c library (with some pmbus support).
> > 
> > I've already reused this library in some other obmc applications and it's been fairly well unit-tested. It also comes with all the public interfaces mocked (so you can unit test your own code).
> > 
> > The idea is that more and more classes get added that will support different pmbus devices. 
> > General idea is that each device that gets support can expose methods to allow device upgrade, black box retrieval, etc..
> > 
> > Anyways, wanted to gauge community interest in this so I can determine how motivated I should be to upstream it.
> > 
> 


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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Vivek Gani]

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

Thanks Mike, to follow up with your response,

I agree that most real-world configuration ends up being mfr-specific. Over
time specs like PMBus Application Profiles may help on some aspects but
from personal experience there's variation between vendors on aspects like
detection of PMBus devices, or occasionally complexity around command
timing & order. This has led us to think around a need of defining a
standardized file format with the possibility of conventions around how
procedures are named (e.g. configure, identify, on/off, etc.) and naming
side-effects (e.g. requires power-cycle or boot stage) to make things
easier for a systems engineer to orchestrate.

We've been facilitating calls around what we'd want in a format and making
some initial rough prototypes for discussion. Most recently we looked at a
format [0] that could decouple IC vendor and system engineer aspects,
facilitate real-world scenarios of needing human-editability, and export to
JSON for use obmc runtime configuration.

This week we're trying to pick up where we left off a month ago so this is
a great time to bring up any opinions/ideas. I want to extend the
invitation out to anyone interested, send me an email and I can arrange to
get you on our mailing list or would if like me to reach-out after our next
meeting to share some highlights of what's happening.

[0]:
https://github.com/PMBusOrg/format-language-evaluation/tree/master/dhall

-Vivek Gani (CV <https://vivekgani.com/vivek_gani_resume.pdf>)
vivek@gani.org

On Tue, Apr 27, 2021 at 1:01 PM Mike Jones <proclivis@gmail.com> wrote:

> Jason
>
> On Apr 27, 2021, at 10:20 AM, Jason Ling <jasonling@google.com> wrote:
>
> Thanks Mike for the feedback
>
> And the context is more than CPU based systems, but includes Networking,
>> other boards with ASICS, etc. So broad context. Hence, it has to work
>> within linux without OBMC.
>
> Ack, although the library was written for a use-case that involves obmc,
> it doesn't *require* obmc. Should work just fine in general Linux.
>
>  Now, imagine the IC manufacturer's tool produces a file that can
>> represent a qualified algorithm that is known to work under all possible
>> scenarios, including CRC errors in parts, corrupt NVM, etc. This is what
>> all the vendors do today. They take care of all the things that can go
>> wrong. In the case of ADI, if power was lost while programming, and the BMC
>> or linux can boot from an aux supply, our data files (encoding algorithms),
>> can repair the part under ALL possible random values in the corrupt part.
>
> This would be great, especially if this is codified in the pmbus spec.
> Right now the library provides a pmbus interface but *part programming* is
> specific to each device class...no guarantee of a common interface across
> multiple parts.
>
>
> The vendors will never agree to an industry specification for programming
> beyond the standard STORE_USER_ALL. Most real world programming is MFR.
>
> This is the reason for a description file, it enables each vendor to
> innovate, yet the end user can process the file with a single engine.
>
> The problem with STORE_USER_ALL is if the power fails or the NVM fails.
>
> For example, some companies just change a few commands, say voltage, and
> then store. But if power is lost during the store, the other 99 values are
> corrupted and need repair, and the part may not even have an address on the
> bus without special commands to reset a few things. Or it might be write
> protected. Or PEC might be required, Etc.
>
>
> 1) I am interested in anything that enables our work
>
> Sure, I'll start carving out more time to make this work suitable for
> upstreaming. At the very least it should give you a mockable interface to
> allow for strong unit testing of upper layers.
>
>
> ADI has code that can implement its proprietary programming via /dev/i2c.
> All that is needed is standard SMBus code. If the programming happens in
> user space, we imagined the data processing engine using that.
>
> In that context, it would be interesting to know what you are doing, as
> you are adding value for sure. If there is an API that delineates the
> function implemented, it would be nice to review it so I can understand
> your work.
>
>
>
>> 2) I am interested in inviting someone from the community, not IC vendor,
>> to our meetings to offer advice and help us define something useful
>
> Sounds good, feel free to reach out to me on an individual basis.
>
>
> Ok. Vivek is on the same list, and he can reach out for an official invite.
>
> Mike
>
>
> On Mon, Apr 26, 2021 at 7:33 PM Mike Jones <proclivis@gmail.com> wrote:
>
>> Jason,
>>
>> I am interested, because within the PMBus Specification Committee, we are
>> working on a data language intended for device programming. And there is
>> hope that eventually it can become adopted into linux and/or OBMC.
>>
>> There is a particular use model that is being driven by the IC suppliers
>> and their tools. One reason is that all the vendors have proprietary tools,
>> but they see no competitive advantage, and would rather support a universal
>> standard.
>>
>> Imagine that programming might be done for:
>>
>> - ICT
>> - Proto Builds
>> - Engineering Bringup
>> - Remote upgrades
>>
>> And the context is more than CPU based systems, but includes Networking,
>> other boards with ASICS, etc. So broad context. Hence, it has to work
>> within linux without OBMC.
>>
>> My view is it is a linux library anyone can use, and OBMC is the piping
>> if it were exposed to a web service, state management, etc.
>>
>> Now, imagine the IC manufacturer's tool produces a file that can
>> represent a qualified algorithm that is known to work under all possible
>> scenarios, including CRC errors in parts, corrupt NVM, etc. This is what
>> all the vendors do today. They take care of all the things that can go
>> wrong. In the case of ADI, if power was lost while programming, and the BMC
>> or linux can boot from an aux supply, our data files (encoding algorithms),
>> can repair the part under ALL possible random values in the corrupt part.
>>
>> Furthermore, an integrator (CM, Design House, software team) has to deal
>> with segmented I2C busses, muxes, etc. And the integrator wants to write a
>> wrapper file that integrates all the vendor files. So this integration file
>> has to take care of muxes, order of operations, calling vendor files, etc.
>>
>> My interest is two part:
>>
>> 1) I am interested in anything that enables our work
>> 2) I am interested in inviting someone from the community, not IC vendor,
>> to our meetings to offer advice and help us define something useful
>>
>> Mike
>>
>>
>>
>> > On Apr 23, 2021, at 4:22 PM, Jason Ling <jasonling@google.com> wrote:
>> >
>> > Hi all,
>> >
>> > What started as an attempt to create a simple command line tool to
>> perform pmbus device upgrades over i2c has turned into the start of a
>> user-space i2c library (with some pmbus support).
>> >
>> > I've already reused this library in some other obmc applications and
>> it's been fairly well unit-tested. It also comes with all the public
>> interfaces mocked (so you can unit test your own code).
>> >
>> > The idea is that more and more classes get added that will support
>> different pmbus devices.
>> > General idea is that each device that gets support can expose methods
>> to allow device upgrade, black box retrieval, etc..
>> >
>> > Anyways, wanted to gauge community interest in this so I can determine
>> how motivated I should be to upstream it.
>> >
>>
>>
>

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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Patrick Williams]

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

On Fri, Apr 23, 2021 at 03:22:35PM -0700, Jason Ling wrote:
> Hi all,
> 
> What started as an attempt to create a simple command line tool to perform
> pmbus device upgrades over i2c has turned into the start of a user-space
> i2c library (with some pmbus support).
> 
> I've already reused this library in some other obmc applications and it's
> been fairly well unit-tested. It also comes with all the public interfaces
> mocked (so you can unit test your own code).

I assume you mean "internal to Google applications"?

> The idea is that more and more classes get added that will support
> different pmbus devices.
> General idea is that each device that gets support can expose methods to
> allow device upgrade, black box retrieval, etc..

I have two questions that come to mind:

    1. Why was the library provided by i2c-tools not good enough?
       (ie. What are you bringing to the table that should make people
       want to use your library rather than a widely used existing
       library?)

    2. How does the availability (and potential recommendation of usage
       by our community) affect the effort to ensure that all i2c and
       pmbus drivers are upstreamed?

       To further clarify this question, we've generally said we want to
       see code using and contributing to upstream Linux subsystems when
       those subsystems already exist.  We don't want a reimplementation
       of the i2c and pmbus subsystem in userspace when those are
       already well-supported upstream by the kernel.

       What is it that makes you want to write your code using low-level
       i2c and PMBus APIs directly in userspace instead of writing or
       updating drivers and using the various high-level user APIs
       provided by kernel subsystems?

       I see you mentioned "pmbus device upgrades" and I know the PMBus
       subsystem doesn't currently support firmware upgrade paths.  But,
       there has been LKML threads about it and what I recollect was
       that it wasn't "not allowed" but just "not implemented".
       Shouldn't we be focusing our efforts on enhancing features for
       the whole OSS community rather than building our own different
       library?

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [pmbusplus] userspace i2c, pmbus interactions

[Jason Ling]

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

>
> I assume you mean "internal to Google applications"?

Yes
I have two questions that come to mind:

    1. Why was the library provided by i2c-tools not good enough?
>        (ie. What are you bringing to the table that should make people
>        want to use your library rather than a widely used existing
>        library?)

i2c-tools are functionally fine; the initial motivation of this library was
to improve testability of the code that uses it. As a result C++ i2c-tools
bindings that themselves are unit tested and mockable (so applications that
interact with i2c could also be unit tested).


    2. How does the availability (and potential recommendation of usage
>        by our community) affect the effort to ensure that all i2c and
>        pmbus drivers are upstreamed?

Well, this library is meant for userspace usage. So i2c (hwmon?) and pmbus
drivers would continue to be upstreamed as per usual.
Motivating use case for userspace i2c transactions are pmbus firmware
updates. With adm1266 we tried to upstream sequencer configuration update
via the hwmon/pmbus driver, it failed spectacularly. So we have to do it
userspace.

       To further clarify this question, we've generally said we want to
>        see code using and contributing to upstream Linux subsystems when
>        those subsystems already exist.  We don't want a reimplementation
>        of the i2c and pmbus subsystem in userspace when those are
>        already well-supported upstream by the kernel.

Agreed, but some things just get rejected when you try to push them
upstream. Like updating sequencer firmware, or updating some other non
pmbus device via i2c. We've tried, patches were rejected.

       What is it that makes you want to write your code using low-level
>        i2c and PMBus APIs directly in userspace instead of writing or
>        updating drivers and using the various high-level user APIs
>        provided by kernel subsystems?

I speak in the context of hwmon/pmbus but these patches were simply
rejected. A lot of times the device you want to upgrade is also the device
you're gathering telemetry from.

       I see you mentioned "pmbus device upgrades" and I know the PMBus
>        subsystem doesn't currently support firmware upgrade paths.  But,
>        there has been LKML threads about it and what I recollect was
>        that it wasn't "not allowed" but just "not implemented".
>        Shouldn't we be focusing our efforts on enhancing features for
>        the whole OSS community rather than building our own different
>        library?

Fair point but I don't see them as mutually exclusive, use hwmon/pmbus
drivers where they make sense and work for you. Switch to userspace for
stuff that gets strong pushback from hwmon/pmbus maintainer or stuff that
just doesn't make sense to put into a driver.



On Fri, Apr 30, 2021 at 7:18 AM Patrick Williams <patrick@stwcx.xyz> wrote:

> On Fri, Apr 23, 2021 at 03:22:35PM -0700, Jason Ling wrote:
> > Hi all,
> >
> > What started as an attempt to create a simple command line tool to
> perform
> > pmbus device upgrades over i2c has turned into the start of a user-space
> > i2c library (with some pmbus support).
> >
> > I've already reused this library in some other obmc applications and it's
> > been fairly well unit-tested. It also comes with all the public
> interfaces
> > mocked (so you can unit test your own code).
>
> I assume you mean "internal to Google applications"?
>
> > The idea is that more and more classes get added that will support
> > different pmbus devices.
> > General idea is that each device that gets support can expose methods to
> > allow device upgrade, black box retrieval, etc..
>
> I have two questions that come to mind:
>
>     1. Why was the library provided by i2c-tools not good enough?
>        (ie. What are you bringing to the table that should make people
>        want to use your library rather than a widely used existing
>        library?)
>
>     2. How does the availability (and potential recommendation of usage
>        by our community) affect the effort to ensure that all i2c and
>        pmbus drivers are upstreamed?
>
>        To further clarify this question, we've generally said we want to
>        see code using and contributing to upstream Linux subsystems when
>        those subsystems already exist.  We don't want a reimplementation
>        of the i2c and pmbus subsystem in userspace when those are
>        already well-supported upstream by the kernel.
>
>        What is it that makes you want to write your code using low-level
>        i2c and PMBus APIs directly in userspace instead of writing or
>        updating drivers and using the various high-level user APIs
>        provided by kernel subsystems?
>
>        I see you mentioned "pmbus device upgrades" and I know the PMBus
>        subsystem doesn't currently support firmware upgrade paths.  But,
>        there has been LKML threads about it and what I recollect was
>        that it wasn't "not allowed" but just "not implemented".
>        Shouldn't we be focusing our efforts on enhancing features for
>        the whole OSS community rather than building our own different
>        library?
>
> --
> Patrick Williams
>

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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Patrick Williams]

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

On Fri, Apr 30, 2021 at 07:52:36AM -0700, Jason Ling wrote:
> 
>     2. How does the availability (and potential recommendation of usage
> >        by our community) affect the effort to ensure that all i2c and
> >        pmbus drivers are upstreamed?
> 
> Well, this library is meant for userspace usage. So i2c (hwmon?) and pmbus
> drivers would continue to be upstreamed as per usual.
> Motivating use case for userspace i2c transactions are pmbus firmware
> updates. With adm1266 we tried to upstream sequencer configuration update
> via the hwmon/pmbus driver, it failed spectacularly. So we have to do it
> userspace.

Do you have pointers to the discussion?
 
> >       What is it that makes you want to write your code using low-level
> >        i2c and PMBus APIs directly in userspace instead of writing or
> >        updating drivers and using the various high-level user APIs
> >        provided by kernel subsystems?
> 
> I speak in the context of hwmon/pmbus but these patches were simply
> rejected. A lot of times the device you want to upgrade is also the device
> you're gathering telemetry from.

I think the "is also" is the part that gives me concern.  Generally this
means binding/unbinding the kernel side of it, which isn't pleasant.

> 
>        I see you mentioned "pmbus device upgrades" and I know the PMBus
> >        subsystem doesn't currently support firmware upgrade paths.  But,
> >        there has been LKML threads about it and what I recollect was
> >        that it wasn't "not allowed" but just "not implemented".
> >        Shouldn't we be focusing our efforts on enhancing features for
> >        the whole OSS community rather than building our own different
> >        library?
> 
> Fair point but I don't see them as mutually exclusive, use hwmon/pmbus
> drivers where they make sense and work for you. Switch to userspace for
> stuff that gets strong pushback from hwmon/pmbus maintainer or stuff that
> just doesn't make sense to put into a driver.

My feeling is that we need more definition on what that boundary is.  As
long as we are really only doing stuff from userspace when there is no
other path forward, I don't have much concern.  But, I've seen too many
cases where someone has tried to write an i2c-driver-in-userspace
because they "didn't like working with the kernel" or some similar
excuse.

What is something that doesn't make sense to put into a driver and why?

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [pmbusplus] userspace i2c, pmbus interactions

[Mike Jones]

> On Apr 30, 2021, at 9:10 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
> 
> On Fri, Apr 30, 2021 at 07:52:36AM -0700, Jason Ling wrote:
>> 
>>    2. How does the availability (and potential recommendation of usage
>>>       by our community) affect the effort to ensure that all i2c and
>>>       pmbus drivers are upstreamed?
>> 
>> Well, this library is meant for userspace usage. So i2c (hwmon?) and pmbus
>> drivers would continue to be upstreamed as per usual.
>> Motivating use case for userspace i2c transactions are pmbus firmware
>> updates. With adm1266 we tried to upstream sequencer configuration update
>> via the hwmon/pmbus driver, it failed spectacularly. So we have to do it
>> userspace.
> 
> Do you have pointers to the discussion?
> 
>>>      What is it that makes you want to write your code using low-level
>>>       i2c and PMBus APIs directly in userspace instead of writing or
>>>       updating drivers and using the various high-level user APIs
>>>       provided by kernel subsystems?
>> 
>> I speak in the context of hwmon/pmbus but these patches were simply
>> rejected. A lot of times the device you want to upgrade is also the device
>> you're gathering telemetry from.
> 
> I think the "is also" is the part that gives me concern.  Generally this
> means binding/unbinding the kernel side of it, which isn't pleasant.
> 
>> 
>>       I see you mentioned "pmbus device upgrades" and I know the PMBus
>>>       subsystem doesn't currently support firmware upgrade paths.  But,
>>>       there has been LKML threads about it and what I recollect was
>>>       that it wasn't "not allowed" but just "not implemented".
>>>       Shouldn't we be focusing our efforts on enhancing features for
>>>       the whole OSS community rather than building our own different
>>>       library?
>> 
>> Fair point but I don't see them as mutually exclusive, use hwmon/pmbus
>> drivers where they make sense and work for you. Switch to userspace for
>> stuff that gets strong pushback from hwmon/pmbus maintainer or stuff that
>> just doesn't make sense to put into a driver.
> 
> My feeling is that we need more definition on what that boundary is.  As
> long as we are really only doing stuff from userspace when there is no
> other path forward, I don't have much concern.  But, I've seen too many
> cases where someone has tried to write an i2c-driver-in-userspace
> because they "didn't like working with the kernel" or some similar
> excuse.

I had a similar discussion with Guenter, who suggested that any i2c code in user space is problematic, but I think it also said there was enough locking in the drivers that SMBus transactions from /dev/i2c or i2c-tools would not interact with hwmon.

However, in general, ADI PMBus devices have a PAGE command, especially the LTC388X and LTC297X families.

This means that many actions require locking the bus to create compound transactions or the use of PAGE_PLUS.

The In System Programming code we support on linux via /dev/i2c has this issue. If another process touched hwmon during programming, and touches a PAGE, the programming may fail, or worse it sends the wrong settings.

If the work within the PMBus community to have a standard programming file format, if it were to be applied in user space, OBMC would have to disable hwmon and all telemetry while the programming is in process.

If the file (or data blob) was applied in a kernel driver, it could lock the i2c during the process so that all hwmon activity and telemetry are held off.

This problem is not unique to our desire for a file format. That is driven by the vendors complexity and business models.

But, in the context of the ADM1266, it is a multipage device. The DS does not say it can do PAGE_PLUS. Therefore, unless I am missing something, the above problems apply unless using a PAGEless bulk programming mechanism. I don’t know this part well enough to know how that works, or if it is published.

The other two families LTC388X and LTC297X do have a PAGEless bulk programming. We don’t like do it with telemetry hammering it, mainly because it feels risky.

Finally, one could argue that most OMBC systems are using the 1266 and not the other parts. But I can say I have sent patches for other parts to OBMC users, so they are in use.

Re: [pmbusplus] userspace i2c, pmbus interactions

[Patrick Williams]

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

On Fri, Apr 30, 2021 at 09:43:21AM -0600, Mike Jones wrote:
 
> I had a similar discussion with Guenter, who suggested that any i2c code in user space is problematic, but I think it also said there was enough locking in the drivers that SMBus transactions from /dev/i2c or i2c-tools would not interact with hwmon.

I'm pretty sure there is locking such that the kernel won't let you talk to a
device that is already bound to a driver.  Once you unbind the kernel side
you're free to do what you want.

What I don't recall is how much of the i2c-mux support gets propagated
out to userspace.  Hopefully you don't have to mess with moving muxes
and talking past those devices.  (I've seen some nasty shell scripts
using i2c-tools doing things like this in the past.)

> However, in general, ADI PMBus devices have a PAGE command, especially the LTC388X and LTC297X families.
> 
> This means that many actions require locking the bus to create compound transactions or the use of PAGE_PLUS.
> 
> The In System Programming code we support on linux via /dev/i2c has this issue. If another process touched hwmon during programming, and touches a PAGE, the programming may fail, or worse it sends the wrong settings.

I'm not sure how this is even possible.  How did an hwmon driver touch
the device and userspace access it?  I'm fairly sure.

> If the work within the PMBus community to have a standard programming file format, if it were to be applied in user space, OBMC would have to disable hwmon and all telemetry while the programming is in process.
> 
> If the file (or data blob) was applied in a kernel driver, it could lock the i2c during the process so that all hwmon activity and telemetry are held off.

Right.  I would expect the locking at a pmbus level so that the
pmbus-hwmon parts would block on a mutex until the firmware update is
complete (if firmware update were done in the pmbus driver).

> This problem is not unique to our desire for a file format. That is driven by the vendors complexity and business models.
> 
> But, in the context of the ADM1266, it is a multipage device. The DS does not say it can do PAGE_PLUS. Therefore, unless I am missing something, the above problems apply unless using a PAGEless bulk programming mechanism. I don’t know this part well enough to know how that works, or if it is published.
> 
> The other two families LTC388X and LTC297X do have a PAGEless bulk programming. We don’t like do it with telemetry hammering it, mainly because it feels risky.
> 
> Finally, one could argue that most OMBC systems are using the 1266 and not the other parts. But I can say I have sent patches for other parts to OBMC users, so they are in use.

There is already some firmware blob support in the kernel, just not for
pmbus.  Has anyone investigated what (if anything) we'd need to do to be
able to leverage this?

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [pmbusplus] userspace i2c, pmbus interactions

[Jason Ling]

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

>
> > However, in general, ADI PMBus devices have a PAGE command, especially
> the LTC388X and LTC297X families.
> >
> > This means that many actions require locking the bus to create compound
> transactions or the use of PAGE_PLUS.
> >
> > The In System Programming code we support on linux via /dev/i2c has this
> issue. If another process touched hwmon during programming, and touches a
> PAGE, the programming may fail, or worse it sends the wrong settings.
> I'm not sure how this is even possible.  How did an hwmon driver touch
> the device and userspace access it?  I'm fairly sure.


IIRC there is some protection if you use the smbus_readX smbus_writeX APIs
but if you're doing ioctl(I2C_RDWR) then a hwmon driver being bound to a
device won't stop you from hammering on it.
Also userspace can pass the force option to i2c-dev and it'll let you
hammer away on the device as well.

On Fri, Apr 30, 2021 at 1:54 PM Patrick Williams <patrick@stwcx.xyz> wrote:

> On Fri, Apr 30, 2021 at 09:43:21AM -0600, Mike Jones wrote:
>
> > I had a similar discussion with Guenter, who suggested that any i2c code
> in user space is problematic, but I think it also said there was enough
> locking in the drivers that SMBus transactions from /dev/i2c or i2c-tools
> would not interact with hwmon.
>
> I'm pretty sure there is locking such that the kernel won't let you talk
> to a
> device that is already bound to a driver.  Once you unbind the kernel side
> you're free to do what you want.
>
> What I don't recall is how much of the i2c-mux support gets propagated
> out to userspace.  Hopefully you don't have to mess with moving muxes
> and talking past those devices.  (I've seen some nasty shell scripts
> using i2c-tools doing things like this in the past.)
>
> > However, in general, ADI PMBus devices have a PAGE command, especially
> the LTC388X and LTC297X families.
> >
> > This means that many actions require locking the bus to create compound
> transactions or the use of PAGE_PLUS.
> >
> > The In System Programming code we support on linux via /dev/i2c has this
> issue. If another process touched hwmon during programming, and touches a
> PAGE, the programming may fail, or worse it sends the wrong settings.
>
> I'm not sure how this is even possible.  How did an hwmon driver touch
> the device and userspace access it?  I'm fairly sure.
>
> > If the work within the PMBus community to have a standard programming
> file format, if it were to be applied in user space, OBMC would have to
> disable hwmon and all telemetry while the programming is in process.
> >
> > If the file (or data blob) was applied in a kernel driver, it could lock
> the i2c during the process so that all hwmon activity and telemetry are
> held off.
>
> Right.  I would expect the locking at a pmbus level so that the
> pmbus-hwmon parts would block on a mutex until the firmware update is
> complete (if firmware update were done in the pmbus driver).
>
> > This problem is not unique to our desire for a file format. That is
> driven by the vendors complexity and business models.
> >
> > But, in the context of the ADM1266, it is a multipage device. The DS
> does not say it can do PAGE_PLUS. Therefore, unless I am missing something,
> the above problems apply unless using a PAGEless bulk programming
> mechanism. I don’t know this part well enough to know how that works, or if
> it is published.
> >
> > The other two families LTC388X and LTC297X do have a PAGEless bulk
> programming. We don’t like do it with telemetry hammering it, mainly
> because it feels risky.
> >
> > Finally, one could argue that most OMBC systems are using the 1266 and
> not the other parts. But I can say I have sent patches for other parts to
> OBMC users, so they are in use.
>
> There is already some firmware blob support in the kernel, just not for
> pmbus.  Has anyone investigated what (if anything) we'd need to do to be
> able to leverage this?
>
> --
> Patrick Williams
>

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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Mike Jones]

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

For some additional context, devices like LTC2991/2 have no EEPROM. The configuration is set when the driver initializes using values in the device tree.

So if they are updated in the field somehow, it has to be done after every boot, because they will be reset to the device tree values.

So any provisioning for updates could include provision for initial values if you want it to be controlled outside device tree.

> On Apr 30, 2021, at 3:00 PM, Jason Ling <jasonling@google.com> wrote:
> 
> > However, in general, ADI PMBus devices have a PAGE command, especially the LTC388X and LTC297X families.
> >
> > This means that many actions require locking the bus to create compound transactions or the use of PAGE_PLUS.
> >
> > The In System Programming code we support on linux via /dev/i2c has this issue. If another process touched hwmon during programming, and touches a PAGE, the programming may fail, or worse it sends the wrong settings.
> I'm not sure how this is even possible.  How did an hwmon driver touch
> the device and userspace access it?  I'm fairly sure.
> 
> IIRC there is some protection if you use the smbus_readX smbus_writeX APIs but if you're doing ioctl(I2C_RDWR) then a hwmon driver being bound to a device won't stop you from hammering on it.
> Also userspace can pass the force option to i2c-dev and it'll let you hammer away on the device as well. 
> 
> On Fri, Apr 30, 2021 at 1:54 PM Patrick Williams <patrick@stwcx.xyz <mailto:patrick@stwcx.xyz>> wrote:
> On Fri, Apr 30, 2021 at 09:43:21AM -0600, Mike Jones wrote:
> 
> > I had a similar discussion with Guenter, who suggested that any i2c code in user space is problematic, but I think it also said there was enough locking in the drivers that SMBus transactions from /dev/i2c or i2c-tools would not interact with hwmon.
> 
> I'm pretty sure there is locking such that the kernel won't let you talk to a
> device that is already bound to a driver.  Once you unbind the kernel side
> you're free to do what you want.
> 
> What I don't recall is how much of the i2c-mux support gets propagated
> out to userspace.  Hopefully you don't have to mess with moving muxes
> and talking past those devices.  (I've seen some nasty shell scripts
> using i2c-tools doing things like this in the past.)
> 
> > However, in general, ADI PMBus devices have a PAGE command, especially the LTC388X and LTC297X families.
> > 
> > This means that many actions require locking the bus to create compound transactions or the use of PAGE_PLUS.
> > 
> > The In System Programming code we support on linux via /dev/i2c has this issue. If another process touched hwmon during programming, and touches a PAGE, the programming may fail, or worse it sends the wrong settings.
> 
> I'm not sure how this is even possible.  How did an hwmon driver touch
> the device and userspace access it?  I'm fairly sure.
> 
> > If the work within the PMBus community to have a standard programming file format, if it were to be applied in user space, OBMC would have to disable hwmon and all telemetry while the programming is in process.
> > 
> > If the file (or data blob) was applied in a kernel driver, it could lock the i2c during the process so that all hwmon activity and telemetry are held off.
> 
> Right.  I would expect the locking at a pmbus level so that the
> pmbus-hwmon parts would block on a mutex until the firmware update is
> complete (if firmware update were done in the pmbus driver).
> 
> > This problem is not unique to our desire for a file format. That is driven by the vendors complexity and business models.
> > 
> > But, in the context of the ADM1266, it is a multipage device. The DS does not say it can do PAGE_PLUS. Therefore, unless I am missing something, the above problems apply unless using a PAGEless bulk programming mechanism. I don’t know this part well enough to know how that works, or if it is published.
> > 
> > The other two families LTC388X and LTC297X do have a PAGEless bulk programming. We don’t like do it with telemetry hammering it, mainly because it feels risky.
> > 
> > Finally, one could argue that most OMBC systems are using the 1266 and not the other parts. But I can say I have sent patches for other parts to OBMC users, so they are in use.
> 
> There is already some firmware blob support in the kernel, just not for
> pmbus.  Has anyone investigated what (if anything) we'd need to do to be
> able to leverage this?
> 
> -- 
> Patrick Williams


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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Mike Jones]

> On Apr 30, 2021, at 2:54 PM, Patrick Williams <patrick@stwcx.xyz> wrote:
> 
> On Fri, Apr 30, 2021 at 09:43:21AM -0600, Mike Jones wrote:
> 
>> I had a similar discussion with Guenter, who suggested that any i2c code in user space is problematic, but I think it also said there was enough locking in the drivers that SMBus transactions from /dev/i2c or i2c-tools would not interact with hwmon.
> 
> I'm pretty sure there is locking such that the kernel won't let you talk to a
> device that is already bound to a driver.  Once you unbind the kernel side
> you're free to do what you want.
> 
> What I don't recall is how much of the i2c-mux support gets propagated
> out to userspace.  Hopefully you don't have to mess with moving muxes
> and talking past those devices.  (I've seen some nasty shell scripts
> using i2c-tools doing things like this in the past.)

In networking boards, I have seen many cases where muxes are part of programming. Because of this, the data language we are working on, which is not very procedural, but does have things like order, polling, retry, etc, is a layered system, where there are files/data per device, and integration file/data that pulls in the per device files, and deals with muxes and such.

That is a little difficult when considering how linux handles them. My understanding is they are in a hierarchy, and there are multiple /dev/i2cN, and when you write to one of them, linux drivers move the mux for you. So any user space application applying a file/data, that handles muxes, has to handle this. So we are thinking that each mux has an ID, and perhaps there is a translation table that maps an abstract mux Id to the actual one, etc.

> 
>> However, in general, ADI PMBus devices have a PAGE command, especially the LTC388X and LTC297X families.
>> 
>> This means that many actions require locking the bus to create compound transactions or the use of PAGE_PLUS.
>> 
>> The In System Programming code we support on linux via /dev/i2c has this issue. If another process touched hwmon during programming, and touches a PAGE, the programming may fail, or worse it sends the wrong settings.
> 
> I'm not sure how this is even possible.  How did an hwmon driver touch
> the device and userspace access it?  I'm fairly sure.

I think I have tested this and it worked, but it was long ago. My recollection was hwmon and other drivers lock the i2c per transaction, not the driver loads/opens and keeps it locked. But I am not an expert here, so I could be wrong.

> 
>> If the work within the PMBus community to have a standard programming file format, if it were to be applied in user space, OBMC would have to disable hwmon and all telemetry while the programming is in process.
>> 
>> If the file (or data blob) was applied in a kernel driver, it could lock the i2c during the process so that all hwmon activity and telemetry are held off.
> 
> Right.  I would expect the locking at a pmbus level so that the
> pmbus-hwmon parts would block on a mutex until the firmware update is
> complete (if firmware update were done in the pmbus driver).
> 
>> This problem is not unique to our desire for a file format. That is driven by the vendors complexity and business models.
>> 
>> But, in the context of the ADM1266, it is a multipage device. The DS does not say it can do PAGE_PLUS. Therefore, unless I am missing something, the above problems apply unless using a PAGEless bulk programming mechanism. I don’t know this part well enough to know how that works, or if it is published.
>> 
>> The other two families LTC388X and LTC297X do have a PAGEless bulk programming. We don’t like do it with telemetry hammering it, mainly because it feels risky.
>> 
>> Finally, one could argue that most OMBC systems are using the 1266 and not the other parts. But I can say I have sent patches for other parts to OBMC users, so they are in use.
> 
> There is already some firmware blob support in the kernel, just not for
> pmbus.  Has anyone investigated what (if anything) we'd need to do to be
> able to leverage this?
> 
> -- 
> Patrick Williams

Re: [pmbusplus] userspace i2c, pmbus interactions

[Alex Qiu]

> I think I have tested this and it worked, but it was long ago. My recollection was hwmon and other drivers lock the i2c per transaction, not the driver loads/opens and keeps it locked. But I am not an expert here, so I could be wrong.
IIRC, some function like i2c_master_xfer() in the kernel locks the bus. PMBus Page register and the following register access sits above this layer, so they are not protected by a single lock, and this is where a userspace process can interrupt and cause trouble as Mike pointed out.

- Alex Qiu

Re: [pmbusplus] userspace i2c, pmbus interactions

[Jason Ling]

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

On Fri, Apr 30, 2021 at 07:52:36AM -0700, Jason Ling wrote:
>
>     2. How does the availability (and potential recommendation of usage
> >        by our community) affect the effort to ensure that all i2c and
> >        pmbus drivers are upstreamed?
>
> Well, this library is meant for userspace usage. So i2c (hwmon?) and pmbus
> drivers would continue to be upstreamed as per usual.
> Motivating use case for userspace i2c transactions are pmbus firmware
> updates. With adm1266 we tried to upstream sequencer configuration update
> via the hwmon/pmbus driver, it failed spectacularly. So we have to do it
> userspace.

Do you have pointers to the discussion?

Whew...took a lot longer to find the thread but here is the explicit
rejection of firmware and configuration upgrade from within the kernel

https://lkml.org/lkml/2020/8/7/565

other things like don't put in ioctls

https://lkml.org/lkml/2020/6/24/830

This is the strongest use-case as it's been explicitly rejected by the
subsystem maintainer.

I suspect that things like adjusting voltages would similarly be rejected
but honestly we haven't gone down that path yet.


On Fri, Apr 30, 2021 at 8:10 AM Patrick Williams <patrick@stwcx.xyz> wrote:

> On Fri, Apr 30, 2021 at 07:52:36AM -0700, Jason Ling wrote:
> >
> >     2. How does the availability (and potential recommendation of usage
> > >        by our community) affect the effort to ensure that all i2c and
> > >        pmbus drivers are upstreamed?
> >
> > Well, this library is meant for userspace usage. So i2c (hwmon?) and
> pmbus
> > drivers would continue to be upstreamed as per usual.
> > Motivating use case for userspace i2c transactions are pmbus firmware
> > updates. With adm1266 we tried to upstream sequencer configuration update
> > via the hwmon/pmbus driver, it failed spectacularly. So we have to do it
> > userspace.
>
> Do you have pointers to the discussion?
>
> > >       What is it that makes you want to write your code using low-level
> > >        i2c and PMBus APIs directly in userspace instead of writing or
> > >        updating drivers and using the various high-level user APIs
> > >        provided by kernel subsystems?
> >
> > I speak in the context of hwmon/pmbus but these patches were simply
> > rejected. A lot of times the device you want to upgrade is also the
> device
> > you're gathering telemetry from.
>
> I think the "is also" is the part that gives me concern.  Generally this
> means binding/unbinding the kernel side of it, which isn't pleasant.
>
> >
> >        I see you mentioned "pmbus device upgrades" and I know the PMBus
> > >        subsystem doesn't currently support firmware upgrade paths.
> But,
> > >        there has been LKML threads about it and what I recollect was
> > >        that it wasn't "not allowed" but just "not implemented".
> > >        Shouldn't we be focusing our efforts on enhancing features for
> > >        the whole OSS community rather than building our own different
> > >        library?
> >
> > Fair point but I don't see them as mutually exclusive, use hwmon/pmbus
> > drivers where they make sense and work for you. Switch to userspace for
> > stuff that gets strong pushback from hwmon/pmbus maintainer or stuff that
> > just doesn't make sense to put into a driver.
>
> My feeling is that we need more definition on what that boundary is.  As
> long as we are really only doing stuff from userspace when there is no
> other path forward, I don't have much concern.  But, I've seen too many
> cases where someone has tried to write an i2c-driver-in-userspace
> because they "didn't like working with the kernel" or some similar
> excuse.
>
> What is something that doesn't make sense to put into a driver and why?
>
> --
> Patrick Williams
>

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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Jason Ling]

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

>
> > >       What is it that makes you want to write your code using low-level
> > >        i2c and PMBus APIs directly in userspace instead of writing or
> > >        updating drivers and using the various high-level user APIs
> > >        provided by kernel subsystems?
> >
> > I speak in the context of hwmon/pmbus but these patches were simply
> > rejected. A lot of times the device you want to upgrade is also the
> device
> > you're gathering telemetry from.
> I think the "is also" is the part that gives me concern.  Generally this
> means binding/unbinding the kernel side of it, which isn't pleasant.


Yup, this definitely isn't pleasant but it's doable. Have you had
experiences where unbinding/binding caused lots of pain? The only pain that
I've seen is that telemetry daemons generally don't take well to having
hwmon sysfs attributes yanked from underneath them.
Just spitballing.. but for devices that need to be upgradeable *and* need
to report telemetry, that such things should be done in a single service
and perhaps it makes the most sense to do it all in userspace (to avoid
ugly unbinding/binding).


>
> >        I see you mentioned "pmbus device upgrades" and I know the PMBus
> > >        subsystem doesn't currently support firmware upgrade paths.
> But,
> > >        there has been LKML threads about it and what I recollect was
> > >        that it wasn't "not allowed" but just "not implemented".
> > >        Shouldn't we be focusing our efforts on enhancing features for
> > >        the whole OSS community rather than building our own different
> > >        library?
> >
> > Fair point but I don't see them as mutually exclusive, use hwmon/pmbus
> > drivers where they make sense and work for you. Switch to userspace for
> > stuff that gets strong pushback from hwmon/pmbus maintainer or stuff that
> > just doesn't make sense to put into a driver.
> My feeling is that we need more definition on what that boundary is.  As
> long as we are really only doing stuff from userspace when there is no
> other path forward, I don't have much concern.  But, I've seen too many
> cases where someone has tried to write an i2c-driver-in-userspace
> because they "didn't like working with the kernel" or some similar
> excuse.
> What is something that doesn't make sense to put into a driver and why?


Firmware/config upgrades and the reason is that your patch will get
rejected.
The feeling is that "dangerous" i2c things that could brick the system or
damage it shouldn't be put into the kernel and that the preference is to
have this done in userspace via i2c-dev. This statement was made about
sequencer config and firmware upgrades.
I suspect it would extend to arbitrarily adjusting voltages, putting
devices into special states, sending control commands to a device's non
pmbus standard i2c interface (vendor specific stuff, like indirect register
accesses).

On Fri, Apr 30, 2021 at 8:45 AM Jason Ling <jasonling@google.com> wrote:

> On Fri, Apr 30, 2021 at 07:52:36AM -0700, Jason Ling wrote:
> >
> >     2. How does the availability (and potential recommendation of usage
> > >        by our community) affect the effort to ensure that all i2c and
> > >        pmbus drivers are upstreamed?
> >
> > Well, this library is meant for userspace usage. So i2c (hwmon?) and
> pmbus
> > drivers would continue to be upstreamed as per usual.
> > Motivating use case for userspace i2c transactions are pmbus firmware
> > updates. With adm1266 we tried to upstream sequencer configuration update
> > via the hwmon/pmbus driver, it failed spectacularly. So we have to do it
> > userspace.
>
> Do you have pointers to the discussion?
>
> Whew...took a lot longer to find the thread but here is the explicit
> rejection of firmware and configuration upgrade from within the kernel
>
> https://lkml.org/lkml/2020/8/7/565
>
> other things like don't put in ioctls
>
> https://lkml.org/lkml/2020/6/24/830
>
> This is the strongest use-case as it's been explicitly rejected by the
> subsystem maintainer.
>
> I suspect that things like adjusting voltages would similarly be rejected
> but honestly we haven't gone down that path yet.
>
>
> On Fri, Apr 30, 2021 at 8:10 AM Patrick Williams <patrick@stwcx.xyz>
> wrote:
>
>> On Fri, Apr 30, 2021 at 07:52:36AM -0700, Jason Ling wrote:
>> >
>> >     2. How does the availability (and potential recommendation of usage
>> > >        by our community) affect the effort to ensure that all i2c and
>> > >        pmbus drivers are upstreamed?
>> >
>> > Well, this library is meant for userspace usage. So i2c (hwmon?) and
>> pmbus
>> > drivers would continue to be upstreamed as per usual.
>> > Motivating use case for userspace i2c transactions are pmbus firmware
>> > updates. With adm1266 we tried to upstream sequencer configuration
>> update
>> > via the hwmon/pmbus driver, it failed spectacularly. So we have to do it
>> > userspace.
>>
>> Do you have pointers to the discussion?
>>
>> > >       What is it that makes you want to write your code using
>> low-level
>> > >        i2c and PMBus APIs directly in userspace instead of writing or
>> > >        updating drivers and using the various high-level user APIs
>> > >        provided by kernel subsystems?
>> >
>> > I speak in the context of hwmon/pmbus but these patches were simply
>> > rejected. A lot of times the device you want to upgrade is also the
>> device
>> > you're gathering telemetry from.
>>
>> I think the "is also" is the part that gives me concern.  Generally this
>> means binding/unbinding the kernel side of it, which isn't pleasant.
>>
>> >
>> >        I see you mentioned "pmbus device upgrades" and I know the PMBus
>> > >        subsystem doesn't currently support firmware upgrade paths.
>> But,
>> > >        there has been LKML threads about it and what I recollect was
>> > >        that it wasn't "not allowed" but just "not implemented".
>> > >        Shouldn't we be focusing our efforts on enhancing features for
>> > >        the whole OSS community rather than building our own different
>> > >        library?
>> >
>> > Fair point but I don't see them as mutually exclusive, use hwmon/pmbus
>> > drivers where they make sense and work for you. Switch to userspace for
>> > stuff that gets strong pushback from hwmon/pmbus maintainer or stuff
>> that
>> > just doesn't make sense to put into a driver.
>>
>> My feeling is that we need more definition on what that boundary is.  As
>> long as we are really only doing stuff from userspace when there is no
>> other path forward, I don't have much concern.  But, I've seen too many
>> cases where someone has tried to write an i2c-driver-in-userspace
>> because they "didn't like working with the kernel" or some similar
>> excuse.
>>
>> What is something that doesn't make sense to put into a driver and why?
>>
>> --
>> Patrick Williams
>>
>

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

Re: [pmbusplus] userspace i2c, pmbus interactions

[Patrick Williams]

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

On Fri, Apr 30, 2021 at 08:53:27AM -0700, Jason Ling wrote:
> Yup, this definitely isn't pleasant but it's doable. Have you had
> experiences where unbinding/binding caused lots of pain? The only pain that
> I've seen is that telemetry daemons generally don't take well to having
> hwmon sysfs attributes yanked from underneath them.

Yes, our current hwmon daemons don't take kindly to the device being
pulled out from underneath them.  That's the big concern.

On top of that, other daemons that consume those sensor readings don't
really like the sensor going away either.

> Just spitballing.. but for devices that need to be upgradeable *and* need
> to report telemetry, that such things should be done in a single service
> and perhaps it makes the most sense to do it all in userspace (to avoid
> ugly unbinding/binding).

This is not ideal to me, if I'm understanding what you wrote correctly.
It means that any PMBus device for which we want to do upgrade we won't
be using the pmbus+hwmon subsystem but instead have to rewrite it all
ourselves in userspace.

My top preference would be to work with upstream about supporting field
upgrade on devices.  Second would be to figure out how to safely
bind/unbind.  Reimplenting what the kernel already has and we already
leverage should be way down our preference list.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [pmbusplus] userspace i2c, pmbus interactions

[Patrick Williams]

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

On Fri, Apr 30, 2021 at 08:45:14AM -0700, Jason Ling wrote:
> On Fri, Apr 30, 2021 at 07:52:36AM -0700, Jason Ling wrote:
> Whew...took a lot longer to find the thread but here is the explicit
> rejection of firmware and configuration upgrade from within the kernel
> 
> https://lkml.org/lkml/2020/8/7/565

I feel like we're not really engaging well with Guenter on some of these
hwmon/pmbus use cases.  He rejected it but there was not any response
from the author on why this is useful, or any engagement on how we
handle this.

He said:

    This should be done ... in a controlled environment (eg manufacturing).
    It can easily brick the hardware, and should not be done in the driver.

We do upgrades of firmware outside of manufacturing all the time.  We
need to engage in a real discussion with him.  If we are comfortable
doing this, why can't we do it?  If he is worried about other
environments, like a general Linux desktop that happened to hook up a
PMBus to a power supply, then lets make it a compile option.

There is other firmware update support in the kernel so the "possibility
of bricking" is not a convincing argument to me.

> other things like don't put in ioctls
> 
> https://lkml.org/lkml/2020/6/24/830

Not putting in ioctls isn't unreasonable if there is another way to
accomplish this.  In general, adding ioctls seems to be a rare activity
and there is a preference for sysfs or debugfs interfaces.

> I suspect that things like adjusting voltages would similarly be rejected
> but honestly we haven't gone down that path yet.

About two years ago I was able to modify the voltage on a VR using the
PMBus subsystem and a one-line change to set a sysfs to R/W instead of
R/O.  I didn't contribute it upstream because of [reasons given by a
former employer], but it doesn't seem non-doable.  Again, I think we'd
need to engage if we're rejected on first pass.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]