Re: [PATCH v4 0/9] misc: Add AMD side band interface(SBI) functionality

Re: [PATCH v4 0/9] misc: Add AMD side band interface(SBI) functionality

[Gupta, Akshay]

On 9/12/2024 12:38 PM, Akshay Gupta wrote:
> AMD's Advanced Platform Management Link (APML) interface provides system
> management functionality accessed by the baseboard management controller (BMC).
> sbrmi driver under hwmon subsystem, which is probed as an i2c driver and
> reports power using APML specified protocol.
> However, APML interface defines few other protocols to support
> full system management functionality out-of-band.
> Out-of-band management is term used for BMC talking to system management unit
> (IP in the processor). AMD's documentation called this link as side band interface.
>
> This patchset is an attempt to add all APML core functionality in one place
> and provide hwmon and user space interface
> 1. [Patch 1] Move the i2c client probe, hwmon sensors and sbrmi core functionality
>     from drivers/hwmon to drivers/misc/
> 2. [Patch 2] Convert i2c to regmap which provides multiple benefits
>     over direct smbus APIs.
>      a. i2c/i3c support and
>      b. 1 byte/2 byte RMI register size addressing
> 3. [Patch 3] Optimize wait condition with regmap API regmap_read_poll_timeout as per
>     suggestion from Arnd
> 4. [Patch 4, 5] Register a misc device which provides
>      a. An ioctl interface through node /dev/sbrmiX
>      b. Register sets is common across APML protocols. IOCTL is providing
>         synchronization among protocols as transactions may create
>         race condition.
> 5. [Subsequent patches 6, 7 and 8] add support for AMD custom protocols
>      a. CPUID
>      b. MCAMSR
>      c. Register xfer
> 6. [Patch 9] AMD side band description document
>
> Open-sourced and widely used https://github.com/amd/esmi_oob_library
> will continue to provide user-space programmable API.
>
> Akshay Gupta (9):
>    hwmon/misc: amd-sbi: Move core sbrmi from hwmon to misc
>    misc: amd-sbi: Use regmap subsystem
>    misc: amd-sbi: Optimize the wait condition for mailbox command
>      completion
>    misc: amd-sbi: Add support for AMD_SBI IOCTL
>    misc: amd-sbi: Add support for mailbox error codes
>    misc: amd-sbi: Add support for CPUID protocol
>    misc: amd-sbi: Add support for MCA register protocol
>    misc: amd-sbi: Add supoort for register xfer
>    misc: amd-sbi: Add document for AMD SB IOCTL description

Hi Greg, Arnd,

You have previously reviewed v3 of patch set and I have addressed the 
review comments in v4.

Can you please take review v4 patch set?

Thank you.

>
>   Documentation/misc-devices/amd-sbi.rst        |  84 ++++
>   Documentation/misc-devices/index.rst          |   1 +
>   .../userspace-api/ioctl/ioctl-number.rst      |   2 +
>   drivers/hwmon/Kconfig                         |  10 -
>   drivers/hwmon/sbrmi.c                         | 357 --------------
>   drivers/misc/Kconfig                          |   1 +
>   drivers/misc/Makefile                         |   1 +
>   drivers/misc/amd-sbi/Kconfig                  |   9 +
>   drivers/misc/amd-sbi/Makefile                 |   3 +
>   drivers/misc/amd-sbi/rmi-core.c               | 452 ++++++++++++++++++
>   drivers/misc/amd-sbi/rmi-core.h               |  67 +++
>   drivers/misc/amd-sbi/rmi-hwmon.c              | 122 +++++
>   drivers/misc/amd-sbi/rmi-i2c.c                | 135 ++++++
>   include/uapi/misc/amd-apml.h                  |  97 ++++
>   14 files changed, 974 insertions(+), 367 deletions(-)
>   create mode 100644 Documentation/misc-devices/amd-sbi.rst
>   delete mode 100644 drivers/hwmon/sbrmi.c
>   create mode 100644 drivers/misc/amd-sbi/Kconfig
>   create mode 100644 drivers/misc/amd-sbi/Makefile
>   create mode 100644 drivers/misc/amd-sbi/rmi-core.c
>   create mode 100644 drivers/misc/amd-sbi/rmi-core.h
>   create mode 100644 drivers/misc/amd-sbi/rmi-hwmon.c
>   create mode 100644 drivers/misc/amd-sbi/rmi-i2c.c
>   create mode 100644 include/uapi/misc/amd-apml.h
>

Re: [PATCH v4 0/9] misc: Add AMD side band interface(SBI) functionality

[Greg KH]

On Fri, Sep 20, 2024 at 12:15:37PM +0530, Gupta, Akshay wrote:
> 
> On 9/12/2024 12:38 PM, Akshay Gupta wrote:
> > AMD's Advanced Platform Management Link (APML) interface provides system
> > management functionality accessed by the baseboard management controller (BMC).
> > sbrmi driver under hwmon subsystem, which is probed as an i2c driver and
> > reports power using APML specified protocol.
> > However, APML interface defines few other protocols to support
> > full system management functionality out-of-band.
> > Out-of-band management is term used for BMC talking to system management unit
> > (IP in the processor). AMD's documentation called this link as side band interface.
> > 
> > This patchset is an attempt to add all APML core functionality in one place
> > and provide hwmon and user space interface
> > 1. [Patch 1] Move the i2c client probe, hwmon sensors and sbrmi core functionality
> >     from drivers/hwmon to drivers/misc/
> > 2. [Patch 2] Convert i2c to regmap which provides multiple benefits
> >     over direct smbus APIs.
> >      a. i2c/i3c support and
> >      b. 1 byte/2 byte RMI register size addressing
> > 3. [Patch 3] Optimize wait condition with regmap API regmap_read_poll_timeout as per
> >     suggestion from Arnd
> > 4. [Patch 4, 5] Register a misc device which provides
> >      a. An ioctl interface through node /dev/sbrmiX
> >      b. Register sets is common across APML protocols. IOCTL is providing
> >         synchronization among protocols as transactions may create
> >         race condition.
> > 5. [Subsequent patches 6, 7 and 8] add support for AMD custom protocols
> >      a. CPUID
> >      b. MCAMSR
> >      c. Register xfer
> > 6. [Patch 9] AMD side band description document
> > 
> > Open-sourced and widely used https://github.com/amd/esmi_oob_library
> > will continue to provide user-space programmable API.
> > 
> > Akshay Gupta (9):
> >    hwmon/misc: amd-sbi: Move core sbrmi from hwmon to misc
> >    misc: amd-sbi: Use regmap subsystem
> >    misc: amd-sbi: Optimize the wait condition for mailbox command
> >      completion
> >    misc: amd-sbi: Add support for AMD_SBI IOCTL
> >    misc: amd-sbi: Add support for mailbox error codes
> >    misc: amd-sbi: Add support for CPUID protocol
> >    misc: amd-sbi: Add support for MCA register protocol
> >    misc: amd-sbi: Add supoort for register xfer
> >    misc: amd-sbi: Add document for AMD SB IOCTL description
> 
> Hi Greg, Arnd,
> 
> You have previously reviewed v3 of patch set and I have addressed the review
> comments in v4.
> 
> Can you please take review v4 patch set?

Please wait until after the merge window is over (i.e. -rc1 is out) as
we can't do anything until then as you know.  Also remember that most of
us are traveling all of this week (and some of us next week) for
conferences, and will get to the backlog when we return.

To help us out, please go through the lists and review other submissions
for these subsystems so that your changes move to the top of unreviewed
ones.

thanks,

greg k-h

Re: [PATCH v4 4/9] misc: amd-sbi: Add support for AMD_SBI IOCTL

[Gupta, Akshay]

On 10/13/2024 8:52 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Sep 12, 2024 at 07:08:05AM +0000, Akshay Gupta wrote:
>> +     switch (msg.cmd) {
>> +     case 0 ... 0x999:
>> +             /* Mailbox protocol */
>> +             ret = rmi_mailbox_xfer(data, &msg);
>> +             break;
>> +     default:
>> +             pr_err("Command:0x%x not recognized\n", msg.cmd);
> You now just allowed userspace to spam the kernel logs for no good
> reason :(
Thanks, will remove the pr_err to not to spam the logs.
>
> Also, always use dev_*() calls in a driver, not pr_*() ones, as then you
> will know exactly what driver/device is sending out the message.
Thank you for the input, will do.
>
>> +             break;
> And you returned the wrong error code if this happens :(
>
> greg k-h
my bad, will take care of this.

Re: [PATCH v4 5/9] misc: amd-sbi: Add support for mailbox error codes

[Gupta, Akshay]

On 10/13/2024 8:49 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Sep 12, 2024 at 07:08:06AM +0000, Akshay Gupta wrote:
>> --- a/include/uapi/misc/amd-apml.h
>> +++ b/include/uapi/misc/amd-apml.h
>> @@ -38,6 +38,10 @@ struct apml_message {
>>                __u32 mb_in[2];
>>                __u8 reg_in[8];
>>        } data_in;
>> +     /*
>> +      * Error code is returned in case of soft mailbox
>> +      */
>> +     __u32 fw_ret_code;
>>   } __attribute__((packed));
> You can not just randomly change the size of a user/kernel structure
> like this, what just broke because of this?
>
> confused,

The changes are not because of anything is broken, we support 3 
different protocol under 1 IOCTL using the same structure. I split the 
patch to make it easy to review.
Modification in patch 4, is only for the existing code. This patch 
(patch 5) has additional functionality, so we do not want add multiple 
changes in single patch (patch 4).

The changes done in patches are as follows:

Patch 4:

- Adding basic structure as per current protocol in upstream kernel

Patch 5:

- Adding additional error code from PMFW.

Patch 6:

- Add changes required to support CPUID protocol

Patch 7:

- Comments modification for MCAMSR protocol (structure remains same as 
CPUID)

> greg k-h
>

Re: [PATCH v4 5/9] misc: amd-sbi: Add support for mailbox error codes

[Greg KH]

On Tue, Oct 15, 2024 at 02:42:08PM +0530, Gupta, Akshay wrote:
> On 10/13/2024 8:49 PM, Greg KH wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Thu, Sep 12, 2024 at 07:08:06AM +0000, Akshay Gupta wrote:
> > > --- a/include/uapi/misc/amd-apml.h
> > > +++ b/include/uapi/misc/amd-apml.h
> > > @@ -38,6 +38,10 @@ struct apml_message {
> > >                __u32 mb_in[2];
> > >                __u8 reg_in[8];
> > >        } data_in;
> > > +     /*
> > > +      * Error code is returned in case of soft mailbox
> > > +      */
> > > +     __u32 fw_ret_code;
> > >   } __attribute__((packed));
> > You can not just randomly change the size of a user/kernel structure
> > like this, what just broke because of this?
> > 
> > confused,
> 
> The changes are not because of anything is broken, we support 3 different
> protocol under 1 IOCTL using the same structure. I split the patch to make
> it easy to review.
> Modification in patch 4, is only for the existing code. This patch (patch 5)
> has additional functionality, so we do not want add multiple changes in
> single patch (patch 4).
> 
> The changes done in patches are as follows:
> 
> Patch 4:
> 
> - Adding basic structure as per current protocol in upstream kernel

So what if we only take the first 4 patches?  Now any changes after that
would change the user/kernel api and break things.

Please don't write changes and then "fix them up" later on, that's not
how to do stuff as it makes it very difficult to review.  What would you
want to see if _you_ had to review this patch series?

thanks,

greg k-h

Re: [PATCH v4 5/9] misc: amd-sbi: Add support for mailbox error codes

[Gupta, Akshay]

On 10/15/2024 3:34 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Tue, Oct 15, 2024 at 02:42:08PM +0530, Gupta, Akshay wrote:
>> On 10/13/2024 8:49 PM, Greg KH wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Thu, Sep 12, 2024 at 07:08:06AM +0000, Akshay Gupta wrote:
>>>> --- a/include/uapi/misc/amd-apml.h
>>>> +++ b/include/uapi/misc/amd-apml.h
>>>> @@ -38,6 +38,10 @@ struct apml_message {
>>>>                 __u32 mb_in[2];
>>>>                 __u8 reg_in[8];
>>>>         } data_in;
>>>> +     /*
>>>> +      * Error code is returned in case of soft mailbox
>>>> +      */
>>>> +     __u32 fw_ret_code;
>>>>    } __attribute__((packed));
>>> You can not just randomly change the size of a user/kernel structure
>>> like this, what just broke because of this?
>>>
>>> confused,
>> The changes are not because of anything is broken, we support 3 different
>> protocol under 1 IOCTL using the same structure. I split the patch to make
>> it easy to review.
>> Modification in patch 4, is only for the existing code. This patch (patch 5)
>> has additional functionality, so we do not want add multiple changes in
>> single patch (patch 4).
>>
>> The changes done in patches are as follows:
>>
>> Patch 4:
>>
>> - Adding basic structure as per current protocol in upstream kernel
> So what if we only take the first 4 patches?  Now any changes after that
> would change the user/kernel api and break things.

Yes, it will break. We need all the patches to go.

>
> Please don't write changes and then "fix them up" later on, that's not
> how to do stuff as it makes it very difficult to review.  What would you
> want to see if _you_ had to review this patch series?

We submitted a single patch in v1, later split the patch based on each 
functionality for ease of review.

I will squash and submit along with other review comments addressed.

>
> thanks,
>
> greg k-h

Re: [PATCH v4 5/9] misc: amd-sbi: Add support for mailbox error codes

[Greg KH]

On Fri, Oct 18, 2024 at 02:53:26PM +0530, Gupta, Akshay wrote:
> 
> On 10/15/2024 3:34 PM, Greg KH wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Tue, Oct 15, 2024 at 02:42:08PM +0530, Gupta, Akshay wrote:
> > > On 10/13/2024 8:49 PM, Greg KH wrote:
> > > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > > > 
> > > > 
> > > > On Thu, Sep 12, 2024 at 07:08:06AM +0000, Akshay Gupta wrote:
> > > > > --- a/include/uapi/misc/amd-apml.h
> > > > > +++ b/include/uapi/misc/amd-apml.h
> > > > > @@ -38,6 +38,10 @@ struct apml_message {
> > > > >                 __u32 mb_in[2];
> > > > >                 __u8 reg_in[8];
> > > > >         } data_in;
> > > > > +     /*
> > > > > +      * Error code is returned in case of soft mailbox
> > > > > +      */
> > > > > +     __u32 fw_ret_code;
> > > > >    } __attribute__((packed));
> > > > You can not just randomly change the size of a user/kernel structure
> > > > like this, what just broke because of this?
> > > > 
> > > > confused,
> > > The changes are not because of anything is broken, we support 3 different
> > > protocol under 1 IOCTL using the same structure. I split the patch to make
> > > it easy to review.
> > > Modification in patch 4, is only for the existing code. This patch (patch 5)
> > > has additional functionality, so we do not want add multiple changes in
> > > single patch (patch 4).
> > > 
> > > The changes done in patches are as follows:
> > > 
> > > Patch 4:
> > > 
> > > - Adding basic structure as per current protocol in upstream kernel
> > So what if we only take the first 4 patches?  Now any changes after that
> > would change the user/kernel api and break things.
> 
> Yes, it will break. We need all the patches to go.

That's not how to submit a patch series.  Please work with the other
kernel developers at your company to do this right before resubmitting.
You shouldn't rely on the community to point out basic engineering
problems like this.  Would you want to review a series like this?

> > Please don't write changes and then "fix them up" later on, that's not
> > how to do stuff as it makes it very difficult to review.  What would you
> > want to see if _you_ had to review this patch series?
> 
> We submitted a single patch in v1, later split the patch based on each
> functionality for ease of review.
> 
> I will squash and submit along with other review comments addressed.

No, don't squash, do it in a patch series, one at a time properly such
that if we were to take any moment in time of the series, all would
still work correctly.  That's the proper way to do any sort of software
engineering, this isn't unique to us at all.

thanks,

greg k-h

Re: [PATCH v4 5/9] misc: amd-sbi: Add support for mailbox error codes

[Gupta, Akshay]

On 10/18/2024 3:05 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Fri, Oct 18, 2024 at 02:53:26PM +0530, Gupta, Akshay wrote:
>> On 10/15/2024 3:34 PM, Greg KH wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Tue, Oct 15, 2024 at 02:42:08PM +0530, Gupta, Akshay wrote:
>>>> On 10/13/2024 8:49 PM, Greg KH wrote:
>>>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>>>
>>>>>
>>>>> On Thu, Sep 12, 2024 at 07:08:06AM +0000, Akshay Gupta wrote:
>>>>>> --- a/include/uapi/misc/amd-apml.h
>>>>>> +++ b/include/uapi/misc/amd-apml.h
>>>>>> @@ -38,6 +38,10 @@ struct apml_message {
>>>>>>                  __u32 mb_in[2];
>>>>>>                  __u8 reg_in[8];
>>>>>>          } data_in;
>>>>>> +     /*
>>>>>> +      * Error code is returned in case of soft mailbox
>>>>>> +      */
>>>>>> +     __u32 fw_ret_code;
>>>>>>     } __attribute__((packed));
>>>>> You can not just randomly change the size of a user/kernel structure
>>>>> like this, what just broke because of this?
>>>>>
>>>>> confused,
>>>> The changes are not because of anything is broken, we support 3 different
>>>> protocol under 1 IOCTL using the same structure. I split the patch to make
>>>> it easy to review.
>>>> Modification in patch 4, is only for the existing code. This patch (patch 5)
>>>> has additional functionality, so we do not want add multiple changes in
>>>> single patch (patch 4).
>>>>
>>>> The changes done in patches are as follows:
>>>>
>>>> Patch 4:
>>>>
>>>> - Adding basic structure as per current protocol in upstream kernel
>>> So what if we only take the first 4 patches?  Now any changes after that
>>> would change the user/kernel api and break things.
>> Yes, it will break. We need all the patches to go.
> That's not how to submit a patch series.  Please work with the other
> kernel developers at your company to do this right before resubmitting.
> You shouldn't rely on the community to point out basic engineering
> problems like this.  Would you want to review a series like this?
>
>>> Please don't write changes and then "fix them up" later on, that's not
>>> how to do stuff as it makes it very difficult to review.  What would you
>>> want to see if _you_ had to review this patch series?
>> We submitted a single patch in v1, later split the patch based on each
>> functionality for ease of review.
>>
>> I will squash and submit along with other review comments addressed.
> No, don't squash, do it in a patch series, one at a time properly such
> that if we were to take any moment in time of the series, all would
> still work correctly.  That's the proper way to do any sort of software
> engineering, this isn't unique to us at all.
>
> thanks,
>
> greg k-h

Hi Greg,

We have compiled and verified individual patch in the patch-set over 
reference BMC platforms.

We have an open-sourced user space library 
https://github.com/amd/esmi_oob_library/ 
<https://github.com/amd/esmi_oob_library/> which depend on the 
out-of-tree kernel modules open-sourced 
https://github.com/amd/apml_modules. <https://github.com/amd/apml_modules.>

This patch-set is an effort to upstream the out-of-tree kernel modules 
open-sourced at https://github.com/amd/apml_modules 
<https://github.com/amd/apml_modules>.

After all the patches are accepted into the Linux, we want to update the 
user-space consumers to move to drivers from Linux kernel and deprecate 
out-of-tree modules.

Thanks,

Akshay