Re: [PATCHv2 08/11] mpt3sas: always use smid 1 for ioctl passthrough

[Christoph Hellwig <hch@lst.de>]

On Fri, Feb 17, 2017 at 09:23:07AM +0100, Hannes Reinecke wrote:
> ioctl passthrough commands require a SCSIIO smid, but cannot
> easily integrate with the block layer. But as we're only ever
> allowing one ioctl command at a time we can reserve smid 1
> for ioctl commands.

I like the idea, but I don't think the implementation is correct.
More below.

> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2355,13 +2355,20 @@ struct scsiio_tracker *
>  		return 0;
>  	}
>  
> -	request = list_entry(ioc->free_list.next,
> -	    struct scsiio_tracker, tracker_list);
> -	request->scmd = scmd;
> +	if (!scmd) {
> +		/* ioctl command, always use the first slot */
> +		request = ioc->lookup[0];
> +		request->scmd = NULL;
> +	} else {
> +		request = list_entry(ioc->free_list.next,
> +				     struct scsiio_tracker, tracker_list);
> +		request->scmd = scmd;
> +	}
>  	request->cb_idx = cb_idx;
>  	smid = request->smid;
>  	request->msix_io = _base_get_msix_index(ioc);
> -	list_del(&request->tracker_list);
> +	if (scmd)
> +		list_del(&request->tracker_list);
>  	spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
>  	return smid;

The freelist check just before the patch context and the locking
don't make much sense here.  I'd say just use a separate helper for the
ioctl smid, ala:

u16
mpt3sas_base_init_smid_pt(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx)
{
	struct scsiio_tracker *request = ioc->lookup[0];

	request->cb_idx = cb_idx;
	request->msix_io = _base_get_msix_index(ioc);
	return request->smid;
}

or given how trivial this is just open-code it in the caller.


>  		ioc->scsi_lookup[i].cb_idx = 0xFF;
>  		ioc->scsi_lookup[i].scmd = NULL;
>  		ioc->scsi_lookup[i].direct_io = 0;
> -		list_add(&ioc->scsi_lookup[i].tracker_list, &ioc->free_list);
> +		if (i > 0)
> +			list_add(&ioc->scsi_lookup[i].tracker_list,
> +				 &ioc->free_list);

Here we special case only request 0 as not added to the list.

> @@ -5165,14 +5174,17 @@ struct scsiio_tracker *
>  	spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
>  	INIT_LIST_HEAD(&ioc->free_list);
>  	smid = 1;
> -	for (i = 0; i < ioc->scsiio_depth; i++, smid++) {
> +	for (i = 1; i < ioc->scsiio_depth; i++, smid++) {
>  		INIT_LIST_HEAD(&ioc->scsi_lookup[i].chain_list);
>  		ioc->scsi_lookup[i].cb_idx = 0xFF;
>  		ioc->scsi_lookup[i].smid = smid;
>  		ioc->scsi_lookup[i].scmd = NULL;
>  		ioc->scsi_lookup[i].direct_io = 0;
> -		list_add_tail(&ioc->scsi_lookup[i].tracker_list,
> -		    &ioc->free_list);
> +		if (i == 1)
> +			INIT_LIST_HEAD(&ioc->lookup[i].tracker_list);
> +		else
> +			list_add_tail(&ioc->scsi_lookup[i].tracker_list,
> +				      &ioc->free_list);

And here we don't even iterate over smid 0, and then special case smid 1.

And note that the code in both loops is otherwise duplicated to start
with and could realy use a little helper to initialize the scsiio_tracker
structure.

Last but not least can_queue is initialized as:

        ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;

which doesn't take this new stolen smid into account.