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

["Gupta, Akshay" <Akshay.Gupta@amd.com>]

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
>