Re: [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32).

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Sat, 2017-06-24 at 01:50 +0900, Minwoo Im wrote:
> - *     Handles either 12 or 16-byte versions of the CDB.
> + *     Handles either 12 16, or 32-byte versions of the CDB.

Please insert a comma between "12" and "16" to avoid that this comment
gets confusing.

> +       if ((tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]))
> +                       == ATA_PROT_UNKNOWN) {

Have you verified this patch with checkpatch? The recommended style is
*not* to use assignments in the expression that controls an if-statement
and also if a comparison expression has to be split to keep the comparison
operator at the end of the first line.

> -               shost->max_cmd_len = 16;
> +               /*
> +                * SPC says that CDB may have a fixed length of up to 16 bytes
> +                * or variable length of between 12 and 260 bytes.
> +                */
> +               shost->max_cmd_len = 260;

As mentioned before, I think a maximum CDB length of 260 is overkill.

>  /* Values for T10/04-262r7 */
> +#define        ATA_32                0x1ff0    /* 32-byte pass-thru,
> service action */
>  #define        ATA_16                0x85      /* 16-byte pass-thru */
>  #define        ATA_12                0xa1      /* 12-byte pass-thru */

Defining ATA_32 just above ATA_12 and ATA_16 is misleading because
the latter two are CDB opcodes while the former is a service action
code. Please move the definition of the ATA_32 service action code
one line up such that it appears at the end of the list of already
defined service codes, namely this list:

/* values for variable length command */
#define XDREAD_32	      0x03
#define XDWRITE_32	      0x04
#define XPWRITE_32	      0x06
#define XDWRITEREAD_32	      0x07
#define READ_32		      0x09
#define VERIFY_32	      0x0a
#define WRITE_32	      0x0b
#define WRITE_SAME_32	      0x0d

Thanks,

Bart.