Re: [PATCH v6 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

[Gupta, Akshay]

On 3/24/2025 9:10 PM, Arnd Bergmann wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Mon, Mar 24, 2025, at 15:58, Akshay Gupta wrote:
> > ---
> > Changes since v5:
> > - Address review comment
> >   - Address Arnd comments
> >   - Add check for cmd in ioctl
> I think you missed one of my comments.
>
> > +static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned
> > long arg)
> > +{
> > +     struct apml_message msg = { 0 };
> > +     struct sbrmi_data *data;
> > +     bool read = false;
> > +     int ret;
> > +
> > +     if (cmd != SBRMI_IOCTL_CMD)
> > +             return -ENOTTY;
> > +
> > +     if (_IOC_SIZE(cmd) != sizeof(msg))
> > +             return -EINVAL;
> You are checking the 'cmd' argument to the function now, which
> is good. There is no need to also check _IOC_SIZE, since
> that is implied by the definition.
> rue;
Ack.
> > +
> > +     switch (msg.cmd) {
> > +     case 0 ... 0x999:
> > +             /* Mailbox protocol */
> > +             ret = rmi_mailbox_xfer(data, &msg);
> > +             break;
> What is however missing here is a specific check for the
> individual commands: I don't think it's a good idea to
> treat all 2458 mailbox commands the same way and just
> pass them through unfiltered here, and I would also not
> pass the specific 'cmd' as part of a multiplexer structure.
>
> Instead, I think there should be a separate ioctl command
> for each thing you can do with the mailbox. From the existing
> driver it appears that these are the commands currently
> supported:
>
> enum sbrmi_msg_id {
>          SBRMI_READ_PKG_PWR_CONSUMPTION = 0x1,
>          SBRMI_WRITE_PKG_PWR_LIMIT,
>          SBRMI_READ_PKG_PWR_LIMIT,
>          SBRMI_READ_PKG_MAX_PWR_LIMIT,
> };
>
> which is just the first four out of the 2458, and clearly small
> enough to justify one ioctl command each. I don't know what
> the command specific arguments would look like, so it's
> possible you may also want to define a separate structure
> for each one.
>
>         Arnd

Link for the documentation: 
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/57883.zip 
<https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/57883.zip>
You may refer to section: 5.4.2.2 SB-RMI Mailbox Service
At present, more than 70 mailbox commands ids are supported.
The number is increasing with each platforms.
however, the input and output for all mailbox commands are maintained as 
32 bit,
hence, we can use the same structure and differentiate the functionality.
As per current AMD's implementation the maximum can go up to 0xFF, I 
will update the
case to 0 ... 0XFF