On Thu, Jul 21, 2022 at 07:16:29AM +0200, Christoph Hellwig wrote:
> Move all per-command work into the per-command ublk_ctrl_* helpers
> instead of being split over those, ublk_ctrl_cmd_validate, and the main
> ublk_ctrl_uring_cmd handler. To facilitate that, the old
> ublk_ctrl_stop_dev function that just contained two function calls is
> folded into both callers.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> drivers/block/ublk_drv.c | 234 +++++++++++++++++++--------------------
> 1 file changed, 116 insertions(+), 118 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 1f7bbbc3276a2..af70c18796e70 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -813,13 +813,6 @@ static void ublk_stop_dev(struct ublk_device *ub)
> cancel_delayed_work_sync(&ub->monitor_work);
> }
>
> -static int ublk_ctrl_stop_dev(struct ublk_device *ub)
> -{
> - ublk_stop_dev(ub);
> - cancel_work_sync(&ub->stop_work);
> - return 0;
> -}
> -
> static inline bool ublk_queue_ready(struct ublk_queue *ubq)
> {
> return ubq->nr_io_ready == ubq->q_depth;
> @@ -1205,8 +1198,8 @@ static int ublk_add_dev(struct ublk_device *ub)
>
> static void ublk_remove(struct ublk_device *ub)
> {
> - ublk_ctrl_stop_dev(ub);
> -
> + ublk_stop_dev(ub);
> + cancel_work_sync(&ub->stop_work);
> cdev_device_del(&ub->cdev, &ub->cdev_dev);
> put_device(&ub->cdev_dev);
> }
> @@ -1227,36 +1220,45 @@ static struct ublk_device *ublk_get_device_from_id(int idx)
> return ub;
> }
>
> -static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd)
> +static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
> {
> struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
> - int ret = -EINVAL;
> int ublksrv_pid = (int)header->data[0];
> unsigned long dev_blocks = header->data[1];
> + struct ublk_device *ub;
> + int ret = -EINVAL;
>
> if (ublksrv_pid <= 0)
> - return ret;
> + return -EINVAL;
> +
> + ub = ublk_get_device_from_id(header->dev_id);
> + if (!ub)
> + return -EINVAL;
>
> wait_for_completion_interruptible(&ub->completion);
>
> schedule_delayed_work(&ub->monitor_work, UBLK_DAEMON_MONITOR_PERIOD);
>
> mutex_lock(&ub->mutex);
> - if (!disk_live(ub->ub_disk)) {
> - /* We may get disk size updated */
> - if (dev_blocks) {
> - ub->dev_info.dev_blocks = dev_blocks;
> - ublk_update_capacity(ub);
> - }
> - ub->dev_info.ublksrv_pid = ublksrv_pid;
> - ret = add_disk(ub->ub_disk);
> - if (!ret)
> - ub->dev_info.state = UBLK_S_DEV_LIVE;
> - } else {
> + if (disk_live(ub->ub_disk)) {
> ret = -EEXIST;
> + goto out_unlock;
> }
> - mutex_unlock(&ub->mutex);
>
> + /* We may get disk size updated */
> + if (dev_blocks) {
> + ub->dev_info.dev_blocks = dev_blocks;
> + ublk_update_capacity(ub);
> + }
> + ub->dev_info.ublksrv_pid = ublksrv_pid;
> + ret = add_disk(ub->ub_disk);
> + if (ret)
> + goto out_unlock;
> +
> + ub->dev_info.state = UBLK_S_DEV_LIVE;
> +out_unlock:
> + mutex_unlock(&ub->mutex);
> + ublk_put_device(ub);
> return ret;
> }
>
> @@ -1281,6 +1283,13 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
> unsigned long queue;
> unsigned int retlen;
> int ret = -EINVAL;
> +
> + if (header->len * BITS_PER_BYTE < nr_cpu_ids)
> + return -EINVAL;
> + if (header->len & (sizeof(unsigned long)-1))
> + return -EINVAL;
> + if (!header->addr)
> + return -EINVAL;
>
> ub = ublk_get_device_from_id(header->dev_id);
> if (!ub)
> @@ -1311,38 +1320,64 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
> return ret;
> }
>
> -static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_dev_info *info,
> - void __user *argp, int idx)
> +static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
> {
> + pr_devel("%s: dev id %d flags %llx\n", __func__,
> + info->dev_id, info->flags[0]);
> + pr_devel("\t nr_hw_queues %d queue_depth %d block size %d dev_capacity %lld\n",
> + info->nr_hw_queues, info->queue_depth,
> + info->block_size, info->dev_blocks);
> +}
> +
> +static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
> +{
> + struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
> + void __user *argp = (void __user *)(unsigned long)header->addr;
> + struct ublksrv_ctrl_dev_info info;
> struct ublk_device *ub;
> - int ret;
> + int ret = -EINVAL;
> +
> + if (header->len < sizeof(info) || !header->addr)
> + return -EINVAL;
> + if (header->queue_id != (u16)-1) {
> + pr_warn("%s: queue_id is wrong %x\n",
> + __func__, header->queue_id);
> + return -EINVAL;
> + }
> + if (copy_from_user(&info, argp, sizeof(info)))
> + return -EFAULT;
> + ublk_dump_dev_info(&ub->dev_info);
The above line should have been ublk_dump_dev_info(&info), otherwise
looks fine:
Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx>
Thanks,
Ming
