On Tue, May 04, 2021 at 12:07:44PM +0200, Fabio M. De Francesco wrote:
> -static void setup_scsitaskmgmt_handles(struct idr *idrtable, spinlock_t *lock,
> - struct uiscmdrsp *cmdrsp,
> +static int setup_scsitaskmgmt_handles(struct xarray *xa, struct uiscmdrsp *cmdrsp,
> wait_queue_head_t *event, int *result)
> {
> - /* specify the event that has to be triggered when this */
> - /* cmd is complete */
> - cmdrsp->scsitaskmgmt.notify_handle =
> - simple_idr_get(idrtable, event, lock);
> - cmdrsp->scsitaskmgmt.notifyresult_handle =
> - simple_idr_get(idrtable, result, lock);
> + int ret;
> + u32 id;
> +
> + /* specify the event that has to be triggered when this cmd is complete */
> + ret = xa_alloc_irq(xa, &id, event, xa_limit_32b, GFP_KERNEL);
> + if (ret == 0)
> + cmdrsp->scsitaskmgmt.notify_handle = id;
> + else
> + return ret;
Reverse the condition. Always do error handling, not success handling.
Try keep the success path at one tab indent and the failure path at two
tab indents.
ret = xa_alloc_irq(xa, &id, event, xa_limit_32b, GFP_KERNEL);
if (ret)
return ret;
cmdrsp->scsitaskmgmt.notify_handle = id;
ret = xa_alloc_irq(xa, &id, result, xa_limit_32b, GFP_KERNEL);
if (ret) {
xa_erase(xa, cmdrsp->scsitaskmgmt.notify_handle);
return ret;
}
cmdrsp->scsitaskmgmt.notifyresult_handle = id;
> +
> + ret = xa_alloc_irq(xa, &id, result, xa_limit_32b, GFP_KERNEL);
> + if (ret == 0)
> + cmdrsp->scsitaskmgmt.notifyresult_handle = id;
> + else {
> + xa_erase(xa, cmdrsp->scsitaskmgmt.notify_handle);
> + return ret;
> + }
> +
> + return ret;
return 0;
> }
>
> /*
> * cleanup_scsitaskmgmt_handles - Forget handles created by
> * setup_scsitaskmgmt_handles()
> - * @idrtable: The data object maintaining the pointer<-->int mappings
> + * @xa: The data object maintaining the pointer<-->int mappings
> * @cmdrsp: Response from the IOVM
> */
> -static void cleanup_scsitaskmgmt_handles(struct idr *idrtable,
> +static void cleanup_scsitaskmgmt_handles(struct xarray *xa,
> struct uiscmdrsp *cmdrsp)
> {
> - if (cmdrsp->scsitaskmgmt.notify_handle)
> - idr_remove(idrtable, cmdrsp->scsitaskmgmt.notify_handle);
> - if (cmdrsp->scsitaskmgmt.notifyresult_handle)
> - idr_remove(idrtable, cmdrsp->scsitaskmgmt.notifyresult_handle);
> + xa_erase(xa, cmdrsp->scsitaskmgmt.notify_handle);
> + xa_erase(xa, cmdrsp->scsitaskmgmt.notifyresult_handle);
> }
>
> /*
> @@ -269,6 +249,7 @@ static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
> int notifyresult = 0xffff;
> wait_queue_head_t notifyevent;
> int scsicmd_id;
> + int setup_ret;
This should be "int ret;". Don't be fancy with the names.
>
> if (devdata->serverdown || devdata->serverchangingstate)
> return FAILED;
> @@ -284,8 +265,16 @@ static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
>
> /* issue TASK_MGMT_ABORT_TASK */
> cmdrsp->cmdtype = CMD_SCSITASKMGMT_TYPE;
> - setup_scsitaskmgmt_handles(&devdata->idr, &devdata->privlock, cmdrsp,
> +
> + setup_ret = setup_scsitaskmgmt_handles(&devdata->xa, cmdrsp,
> ¬ifyevent, ¬ifyresult);
> + if (setup_ret < 0) {
You changed setup_scsitaskmgmt_handles() to return non-zero on error so
check for non-zero here. Be consistent.
if (ret)
return FAILED;
> + dev_dbg(&scsidev->sdev_gendev,
> + "visorhba: setup_scsitaskmgmt_handles returned %d\n",
> + setup_ret);
> + return FAILED;
> + }
> +
>
> /* save destination */
> cmdrsp->scsitaskmgmt.tasktype = tasktype;
> @@ -311,14 +300,14 @@ static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
> dev_dbg(&scsidev->sdev_gendev,
> "visorhba: taskmgmt type=%d success; result=0x%x\n",
> tasktype, notifyresult);
> - cleanup_scsitaskmgmt_handles(&devdata->idr, cmdrsp);
> + cleanup_scsitaskmgmt_handles(&devdata->xa, cmdrsp);
> return SUCCESS;
>
> err_del_scsipending_ent:
> dev_dbg(&scsidev->sdev_gendev,
> "visorhba: taskmgmt type=%d not executed\n", tasktype);
> del_scsipending_ent(devdata, scsicmd_id);
> - cleanup_scsitaskmgmt_handles(&devdata->idr, cmdrsp);
> + cleanup_scsitaskmgmt_handles(&devdata->xa, cmdrsp);
> return FAILED;
> }
>
> @@ -654,13 +643,13 @@ DEFINE_SHOW_ATTRIBUTE(info_debugfs);
> * Service Partition returned the result of the task management
> * command. Wake up anyone waiting for it.
> */
> -static void complete_taskmgmt_command(struct idr *idrtable,
> +static void complete_taskmgmt_command(struct xarray *xa,
> struct uiscmdrsp *cmdrsp, int result)
> {
> wait_queue_head_t *wq =
> - idr_find(idrtable, cmdrsp->scsitaskmgmt.notify_handle);
> + xa_load(xa, cmdrsp->scsitaskmgmt.notify_handle);
> int *scsi_result_ptr =
> - idr_find(idrtable, cmdrsp->scsitaskmgmt.notifyresult_handle);
> + xa_load(xa, cmdrsp->scsitaskmgmt.notifyresult_handle);
> if (unlikely(!(wq && scsi_result_ptr))) {
> pr_err("visorhba: no completion context; cmd will time out\n");
> return;
> @@ -708,7 +697,7 @@ static void visorhba_serverdown_complete(struct visorhba_devdata *devdata)
> break;
> case CMD_SCSITASKMGMT_TYPE:
> cmdrsp = pendingdel->sent;
> - complete_taskmgmt_command(&devdata->idr, cmdrsp,
> + complete_taskmgmt_command(&devdata->xa, cmdrsp,
> TASK_MGMT_FAILED);
> break;
> default:
> @@ -905,7 +894,7 @@ static void drain_queue(struct uiscmdrsp *cmdrsp,
> if (!del_scsipending_ent(devdata,
> cmdrsp->scsitaskmgmt.handle))
> break;
> - complete_taskmgmt_command(&devdata->idr, cmdrsp,
> + complete_taskmgmt_command(&devdata->xa, cmdrsp,
> cmdrsp->scsitaskmgmt.result);
> } else if (cmdrsp->cmdtype == CMD_NOTIFYGUEST_TYPE)
> dev_err_once(&devdata->dev->device,
> @@ -1053,7 +1042,7 @@ static int visorhba_probe(struct visor_device *dev)
> if (err)
> goto err_debugfs_info;
>
> - idr_init(&devdata->idr);
> + xa_init(&devdata->xa);
>
> devdata->cmdrsp = kmalloc(sizeof(*devdata->cmdrsp), GFP_ATOMIC);
> visorbus_enable_channel_interrupts(dev);
> @@ -1096,8 +1085,6 @@ static void visorhba_remove(struct visor_device *dev)
> scsi_remove_host(scsihost);
> scsi_host_put(scsihost);
>
> - idr_destroy(&devdata->idr);
Do we not have to call xa_destroy()?
> -
> dev_set_drvdata(&dev->device, NULL);
> debugfs_remove(devdata->debugfs_info);
> debugfs_remove_recursive(devdata->debugfs_dir);
regards,
dan carpenter
