Re: [PATCH 04/10] scsi: simplify varlen CDB length checking

[Bart Van Assche <bvanassche@xxxxxxx>]

On 10/5/20 1:41 AM, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
> index 8ea44c6595efa7..b6222df7254a3a 100644
> --- a/drivers/scsi/scsi_logging.c
> +++ b/drivers/scsi/scsi_logging.c
> @@ -111,7 +111,7 @@ static size_t scsi_format_opcode_name(char *buffer, size_t buf_len,
>   
>   	cdb0 = cdbp[0];
>   	if (cdb0 == VARIABLE_LENGTH_CMD) {
> -		int len = scsi_varlen_cdb_length(cdbp);
> +		int len = cdbp[7] + 8;
>   
>   		if (len < 10) {
>   			off = scnprintf(buffer, buf_len,
> diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
> index 731ac09ed23135..297fc1881607b6 100644
> --- a/include/scsi/scsi_common.h
> +++ b/include/scsi/scsi_common.h
> @@ -9,20 +9,15 @@
>   #include <linux/types.h>
>   #include <scsi/scsi_proto.h>
>   
> -static inline unsigned
> -scsi_varlen_cdb_length(const void *hdr)
> -{
> -	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
> -}
> -
>   extern const unsigned char scsi_command_size_tbl[8];
>   #define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
>   
>   static inline unsigned
>   scsi_command_size(const unsigned char *cmnd)
>   {
> -	return (cmnd[0] == VARIABLE_LENGTH_CMD) ?
> -		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);
> +	if (cmnd[0] == VARIABLE_LENGTH_CMD)
> +		return cmnd[7] + 8;
> +	return COMMAND_SIZE(cmnd[0]);
>   }
>   
>   /* Returns a human-readable name for the device */
> diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
> index c3686011193224..c57f9cd8185526 100644
> --- a/include/scsi/scsi_proto.h
> +++ b/include/scsi/scsi_proto.h
> @@ -176,16 +176,6 @@
>   
>   #define SCSI_MAX_VARLEN_CDB_SIZE 260
>   
> -/* defined in T10 SCSI Primary Commands-2 (SPC2) */
> -struct scsi_varlen_cdb_hdr {
> -	__u8 opcode;        /* opcode always == VARIABLE_LENGTH_CMD */
> -	__u8 control;
> -	__u8 misc[5];
> -	__u8 additional_cdb_length;         /* total cdb length - 8 */
> -	__be16 service_action;
> -	/* service specific data follows */
> -};

I'm OK with removing struct scsi_varlen_cdb_hdr but not with the removal of the
scsi_varlen_cdb_length() function. I'd like to keep that function because I think
it makes code that handles variable length CDBs easier to read.

Thanks,

Bart.