On Sat, 2017-06-17 at 20:00 +0900, Minwoo Im wrote:
> - if ((tf->protocol = ata_scsi_map_proto(cdb[1])) == ATA_PROT_UNKNOWN) {
> + /*
> + * if SCSI operation code in cdb[0] is ATA_12 or ATA_16,
> + * then cdb[1] will contain protocol of ATA PASS-THROUGH.
> + * otherwise, Its operation code shall be ATA_32(7Fh).
> + * in this case, cdb[10] will contain protocol of it.
> + * we call this command as a variable-length cdb.
> + */
> + if (cdb[0] == ATA_12 || cdb[0] == ATA_16)
> + tf->protocol = ata_scsi_map_proto(cdb[1]);
> + else
> + tf->protocol = ata_scsi_map_proto(cdb[10]);
> +
> + if (tf->protocol == ATA_PROT_UNKNOWN) {
> fp = 1;
> goto invalid_fld;
> }
Hello Minwoo,
Please consider introducing a variable (e.g. called "cdb_offset") in which the
value 9 is stored for 32-byte CDBs and 0 for 12-byte and 16-byte CDBs. That
will allow to rewrite the above code as follows:
tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset])
I think that will allow to remove most "if (cdb[0] == ATA_12 || cdb[0] == ATA_16)"
statements introduced by this patch.
> + tf->auxiliary = (cdb[28] << 24) | (cdb[29] << 16)
> + | (cdb[30] << 8) | cdb[31];
Please use get_unaligned_be32() instead of open-coding it.
> + const u16 sa = (cdb[8] >> 8) | cdb[9]; /* service action */
Please use get_unaligned_be16() instead of open-coding it.
> @@ -4385,7 +4463,12 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
> shost->max_id = 16;
> shost->max_lun = 1;
> shost->max_channel = 1;
> - shost->max_cmd_len = 16;
> + /*
> + * SPC-3, SPC-4: Definition of CDB
> + * A 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;
Does ATA pass-through really support 260-byte CDBs or is the maximum CDB length
that is supported by ATA_32 perhaps 32 bytes?
Thanks,
Bart.
