Re: [PATCH 1/1] nvme : Add ioctl to query nvme attributes

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 11/3/22 05:27, Joel Granados wrote:
> An ioctl (NVME_IOCTL_GET_ATTR) is added to query nvme controller
> attributes needed to effectively write to the char device without needing
> to be a privileged user.  They are also provided on block devices for
> convenience. The attributes here are meant to complement what you can
> already get with the allowed identify commands.
> 
> New members are added to the nvme_ctrl struct: dmsl, vsl and wusl from
> the I/O Command Set Specific Identify Controller Data Structure in the nvme
> spec.
> 
> We add NVME_IOCTL_GET_ATTR_{V0SZ,CURZS} in order to make the ioctl
> extensible. These serve as both size and version. The caller is expected to
> pass the structure size as the first member of the struct. For example:
> 
> 	{...
> 	struct nvme_get_attr nvme_get_attr = {0};
> 	nvme_get_attr.argsz = sizeof(struct nvme_get_attr);
> 	...}
> 
> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
>   drivers/nvme/host/core.c        |  5 ++-
>   drivers/nvme/host/ioctl.c       | 57 +++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h        | 11 +++++
>   include/uapi/linux/nvme_ioctl.h | 74 +++++++++++++++++++++++++++++++++
>   4 files changed, 146 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0090dc0b3ae6..267f28592b9d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3062,9 +3062,12 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
>   
>   	if (id->dmrl)
>   		ctrl->max_discard_segments = id->dmrl;
> -	ctrl->dmrsl = le32_to_cpu(id->dmrsl);
>   	if (id->wzsl)
>   		ctrl->max_zeroes_sectors = nvme_mps_to_sectors(ctrl, id->wzsl);
> +	ctrl->dmrsl = le32_to_cpu(id->dmrsl);
> +	ctrl->dmsl = le64_to_cpu(id->dmsl);
> +	ctrl->vsl = id->vsl;
> +	ctrl->wusl = id->wusl;
>   
>   free_data:
>   	kfree(id);
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 9273db147872..2197a2b7ab56 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -53,6 +53,57 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
>   	return (void __user *)ptrval;
>   }
>   
> +static inline u32 nvme_sectors_to_mps(struct nvme_ctrl *ctrl, u32 unit)
> +{
> +	return ilog2(unit) + 9 - 12 - NVME_CAP_MPSMIN(ctrl->cap);
> +}
> +
> +static int nvme_ctrl_export_attrs(struct nvme_ctrl *ctrl,
> +				  struct nvme_get_attr __user *arg)
> +{
> +	struct nvme_get_attr nvme_get_attr = {0};
> +	__u32 usize, retsize;
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(struct nvme_get_attr) < NVME_IOCTL_GET_ATTR_V0SZ);
> +	BUILD_BUG_ON(sizeof(struct nvme_get_attr) != NVME_IOCTL_GET_ATTR_CURSZ);
> +

Is there a common helper function where al the sizes validated ?
if so please move it there ....

> +	if (copy_from_user(&nvme_get_attr, arg, 2 * sizeof(__u32)))
> +		return -EFAULT;
> +
> +	if (nvme_get_attr.flags != 0)
> +		return -EINVAL;
> +
> +	if (nvme_get_attr.argsz < NVME_IOCTL_GET_ATTR_V0SZ)
> +		return -EINVAL;
> +	usize = nvme_get_attr.argsz;
> +
> +	// Enforce backwards compatibility

/**/ style comments only ...

> +	ret = copy_struct_from_user(&nvme_get_attr, NVME_IOCTL_GET_ATTR_CURSZ, arg, usize);

overly long line ?

> +	if (ret)
> +		return ret;
> +
> +	nvme_get_attr.argsz = NVME_IOCTL_GET_ATTR_CURSZ;
> +	nvme_get_attr.mpsmin = NVME_CAP_MPSMIN(ctrl->cap);
> +	nvme_get_attr.vsl = ctrl->vsl;
> +	nvme_get_attr.wusl = ctrl->wusl;
> +	nvme_get_attr.dmrl = ctrl->max_discard_segments;
> +	nvme_get_attr.dmsl = ctrl->dmsl;
> +	nvme_get_attr.dmrsl = ctrl->dmrsl;
> +	nvme_get_attr.oncs = ctrl->oncs;
> +	if (ctrl->max_zeroes_sectors > 0)
> +		nvme_get_attr.wzsl = nvme_sectors_to_mps(ctrl, ctrl->max_zeroes_sectors);
> +	if (ctrl->max_hw_sectors > 0)
> +		nvme_get_attr.mdts = nvme_sectors_to_mps(ctrl, ctrl->max_hw_sectors);
> +
> +	retsize = min(usize, NVME_IOCTL_GET_ATTR_CURSZ);
> +	if (copy_to_user((struct nvme_get_attr __user *)arg, &nvme_get_attr,
> +			 retsize))

normal function style is

ret = function_call();
if(ret)
	return ret;


> +		return -EFAULT;
> +
> +	return 0;
> +}
> +

-ck