Re: [PATCH 04/39] scsi: Fixup calling convention for scsi_mode_sense()

[Christoph Hellwig <hch@lst.de>]

On Fri, Apr 23, 2021 at 01:39:09PM +0200, Hannes Reinecke wrote:
> The description for scsi_mode_sense() claims to return the number
> of valid bytes on success, which is not what the code does.
> Additionally there is no gain in returning the scsi status, as
> everything the callers do is to check against scsi_result_is_good(),
> which is what scsi_mode_sense() does already.
> So change the calling convention to return a standard error code
> on failure, and 0 on success, and adapt the description and all
> callers.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_lib.c           | 10 ++++++----
>  drivers/scsi/scsi_transport_sas.c |  9 ++++-----
>  drivers/scsi/sd.c                 | 12 ++++++------
>  drivers/scsi/sr.c                 |  2 +-
>  4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index d7c0d5a5f263..c532f9390ae3 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2135,9 +2135,7 @@ EXPORT_SYMBOL_GPL(scsi_mode_select);
>   *	@sshdr: place to put sense data (or NULL if no sense to be collected).
>   *		must be SCSI_SENSE_BUFFERSIZE big.
>   *
> - *	Returns zero if unsuccessful, or the header offset (either 4
> - *	or 8 depending on whether a six or ten byte command was
> - *	issued) if successful.
> + *	Returns zero if successful, or a negative error number on failure
>   */
>  int
>  scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
> @@ -2190,6 +2188,8 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>  	 * byte as the problem.  MODE_SENSE commands can return
>  	 * ILLEGAL REQUEST if the code page isn't supported */
>  
> +	if (result < 0)
> +		return result;

The return would be much easier to read if it was just below the call to
scsi_execute_req that assigns result than under the comment
describing the code below.

>  	if (use_10_for_ms && !scsi_status_is_good(result) &&
>  	    driver_byte(result) == DRIVER_SENSE) {
>  		if (scsi_sense_valid(sshdr)) {
> @@ -2228,13 +2228,15 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>  			data->block_descriptor_length = buffer[3];
>  		}
>  		data->header_length = header_length;
> +		result = 0;
>  	} else if ((status_byte(result) == CHECK_CONDITION) &&
>  		   scsi_sense_valid(sshdr) &&
>  		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
>  		retry_count--;
>  		goto retry;
>  	}
> -
> +	if (result > 0)
> +		result = -EIO;

IMHO it would be nice to restructure the end of the function to do
something like:

	if (!scsi_status_is_good(result)) {
		if (status_byte(result) == CHECK_CONDITION &&
		    scsi_sense_valid(sshdr) &&
		    sshdr->sense_key == UNIT_ATTENTION &&
		    retry_count) {
			retry_count--;
			goto retry;
		}
		return -EIO;
	}

	// various data fixups here

	return 0;