Re: [PATCH v3 8/8] misc: amd-sbi: Add document for AMD SB IOCTL description

[Gupta, Akshay]

On 8/14/2024 4:03 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Aug 14, 2024 at 09:59:53AM +0000, Akshay Gupta wrote:
> > +The following IOCTL is defined:
> > +
> > +``#define SB_BASE_IOCTL_NR      0xF9``
> > +``#define SBRMI_IOCTL_CMD          _IOWR(SB_BASE_IOCTL_NR, 0, struct apml_message)``
> You only have 1 ioctl, so why are you saying that you are reserving
> 0-1F?
Currently we have only 1 IOCTL, but will add IOCTL for SBTSI.

I have reserved considering the future use cases.

However, we may not need up to 1F will restrict it to 0-Fh

> > +Data structure::
> > +     struct apml_message {
> > +             /* message ids:
> > +              * Mailbox Messages:    0x0 ... 0x999
> > +              * APML_CPUID:          0x1000
> > +              * APML_MCA_MSR:        0x1001
> > +              * APML_REG:            0x1002
> > +              */
> > +             __u32 cmd;
> > +             /*
> > +              * 8 bit data for reg read,
> > +              * 32 bit data in case of mailbox,
> > +              * up to 64 bit in case of cpuid and mca msr
> > +              */
> > +             union {
> > +                     __u64 cpu_msr_out;
> > +                     __u32 mb_out[2];
> > +                     __u8 reg_out[8];
> > +             } data_out;
> > +             /*
> > +              * [0]...[3] mailbox 32bit input
> > +              *           cpuid & mca msr,
> > +              *           rmi rd/wr: reg_offset
> > +              * [4][5] cpuid & mca msr: thread
> > +              * [4] rmi reg wr: value
> > +              * [6] cpuid: ext function & read eax/ebx or ecx/edx
> > +              *      [7:0] -> bits [7:4] -> ext function &
> > +              *      bit [0] read eax/ebx or ecx/edx
> > +              * [7] read/write functionality
> > +              */
> > +             union {
> > +                     __u64 cpu_msr_in;
> > +                     __u32 mb_in[2];
> > +                     __u8 reg_in[8];
> > +             } data_in;
> > +             /*
> > +              * Status code is returned in case of CPUID/MCA access
> > +              * Error code is returned in case of soft mailbox
> > +              */
> > +             __u32 fw_ret_code;
> > +     } __attribute__((packed));
> This does not need to be here, pull it directly from the .h file using
> kernel doc formatting please.
I will update this.
> > +The ioctl would return a non-zero on failure; user can read errno to see
> > +what happened.
> That's not how the ioctl syscall works :(
I will document the errors returned in the header file.
> > The transaction returns 0 on success.
> > +
> > +User space C-APIs are made available by linking against the esmi_oob_library,
> > +which is provided by the E-SMS projecthttps://www.amd.com/en/developer/e-sms.html.
> > +Link:https://github.com/amd/esmi_oob_library
> What is this last line for?
I will rewrite the statement as follows:

User space C-APIs are made available by esmi_oob_library hosted athttps://github.com/amd/esmi_oob_library
which is provided as part of E-SMS projecthttps://www.amd.com/en/developer/e-sms.html.

I will wait for feedback on other patches before submitting next version.
> thanks,
>
> greg k-h