On Wed, Oct 02, 2024 at 06:05:31PM +0200, Steffen Eiden wrote:
> Add an extended List Secrets IOCTL. In contrast to the first list IOCTL
> this accepts an index as the first two bytes of the provided page as an
> input. This index is then taken as the index offset for the list UVC to
> receive later entries for the list.
>
> Signed-off-by: Steffen Eiden <seiden@xxxxxxxxxxxxx>
> ---
> arch/s390/include/uapi/asm/uvdevice.h | 4 ++
> drivers/s390/char/uvdevice.c | 96 ++++++++++++++++++++-------
> 2 files changed, 75 insertions(+), 25 deletions(-)
...
> +/* The actual list(_ext) IOCTL.
> + * If list_ext is true, the first two bytes of the user buffer set the starting index of the
> + * list-UVC
> + */
This is not standard kernel comment style; there is no need for a line
with more than >80 characters here; and in this case it would be nice
to add a "." at the end of the sentence :)
> +static int list_secrets(struct uvio_ioctl_cb *uv_ioctl, bool list_ext)
> +{
> + void __user *user_buf_arg = (void __user *)uv_ioctl->argument_addr;
> + u16 __user *user_index = (u16 __user *)uv_ioctl->argument_addr;
> + u8 *secrets = NULL;
No need to preinitialize.
> + u16 start_idx = 0;
> + int ret;
> +
> + if (uv_ioctl->argument_len != UVIO_LIST_SECRETS_LEN)
> + return -EINVAL;
> +
> + BUILD_BUG_ON(UVIO_LIST_SECRETS_LEN != PAGE_SIZE);
> + secrets = (u8 *)get_zeroed_page(GFP_KERNEL);
> + if (!secrets)
> + return -ENOMEM;
> +
> + /* The extended call accepts an u16 index as input */
> + ret = -EFAULT;
> + if (list_ext) {
> + if (get_user(start_idx, user_index))
> + goto err;
> + }
> +
> + uv_list_secrets(secrets, start_idx, &uv_ioctl->uv_rc, &uv_ioctl->uv_rrc);
> +
> + if (copy_to_user(user_buf_arg, secrets, UVIO_LIST_SECRETS_LEN))
> + goto err;
> + ret = 0;
> +
> +err:
> + free_pages((unsigned long)secrets, 0);
> + return ret;
> +}
If you would change the order of the code a bit, it would be possible
to write this shorter:
int ret = 0;
...
if (list_ext && get_user(start_idx, user_index))
return -EFAULT;
secrets = (u8 *)get_zeroed_page(GFP_KERNEL);
if (!secrets)
return -ENOMEM;
uv_list_secrets(secrets, start_idx, &uv_ioctl->uv_rc, &uv_ioctl->uv_rrc);
if (copy_to_user(user_buf_arg, secrets, UVIO_LIST_SECRETS_LEN))
ret = -EFAULT;
free_page((unsigned long)secrets);
return ret;
> +/** uvio_list_secrets_ext() - perform a List Secret UVC with a starting index
> + * @uv_ioctl: ioctl control block
This is not kernel doc style.
Anyway, just my 0.02.
