Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 4 May 2021 14:00:01 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     outreachy-kernel@googlegroups.com,
        David Kershner <david.kershner@unisys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        sparmaintainer@unisys.com, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, Matthew Wilcox <willy@infradead.org>,
        Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH v6] staging: unisys: visorhba: Convert module from IDR to
 XArray
Message-ID: <20210504110001.GV1981@kadam>
References: <20210504100744.28073-1-fmdefrancesco@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210504100744.28073-1-fmdefrancesco@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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,
>  				   &notifyevent, &notifyresult);
> +	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