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@xxxxxxx>
> ---
> 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;
