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

Message ID 20201005084130.143273-5-hch@lst.de
State New
Headers show
Series
  • [01/10] scsi: don't export scsi_device_from_queue
Related show

Commit Message

Christoph Hellwig Oct. 5, 2020, 8:41 a.m.
Directly access the cdb array like we do everywhere else insted of
overlaying a structure on top of it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_logging.c |  2 +-
 include/scsi/scsi_common.h  | 11 +++--------
 include/scsi/scsi_proto.h   | 10 ----------
 3 files changed, 4 insertions(+), 19 deletions(-)

Comments

Hannes Reinecke Oct. 5, 2020, 9:03 a.m. | #1
On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> Directly access the cdb array like we do everywhere else insted of

> overlaying a structure on top of it.

> 

> Signed-off-by: Christoph Hellwig <hch@lst.de>

> ---

>   drivers/scsi/scsi_logging.c |  2 +-

>   include/scsi/scsi_common.h  | 11 +++--------

>   include/scsi/scsi_proto.h   | 10 ----------

>   3 files changed, 4 insertions(+), 19 deletions(-)

> 

> 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 */

> -};

> -

>   /*

>    *  SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft

>    *  T10/1561-D Revision 4 Draft dated 7th November 2002.

> 

Reviewed-by: Hannes Reinecke <hare@suse.de>


Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
Bart Van Assche Oct. 5, 2020, 4:06 p.m. | #2
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.
Martin K. Petersen Oct. 7, 2020, 2:36 a.m. | #3
Bart,

> 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.

I agree. Please drop the header and simplify the wrapper function.

Patch

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 */
-};
-
 /*
  *  SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft
  *  T10/1561-D Revision 4 Draft dated 7th November 2002.