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

From: Arnd Bergmann
Date: Mon Mar 24 2025 - 11:41:24 EST


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;

> +
> + 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