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

["Arnd Bergmann" <arnd@arndb.de>]

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