Re: [PATCH 4/4] scsi_debug: add resp_write_scat function

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

On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
> -static const struct opcode_info_t vl_iarr[1] = {	/* VARIABLE LENGTH */
> +static const struct opcode_info_t vl_iarr[2] = {	/* VARIABLE LENGTH */

Please leave out the array size and let the compiler determine the array size.

>  	{0, 0x7f, 0xb, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_dt0,
> -	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0xb, 0xfa,
> -		   0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */
> +	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0xb, 0xfa,
> +		0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */

Shouldn't this change have been included in the patch that fixes the group
number mask?
 
>  static const struct opcode_info_t maint_in_iarr[2] = {
> @@ -518,9 +523,10 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>  	    {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>  	{1, 0x9e, 0x10, F_SA_LOW | F_D_IN, resp_readcap16, sa_in_iarr,
>  	    {16,  0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> -	     0xff, 0xff, 0xff, 0x1, 0xc7} },	/* READ CAPACITY(16) */
> -	{0, 0, 0, F_INV_OP | FF_RESPOND, NULL, NULL, /* SA OUT */
> -	    {0,  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
> +	     0xff, 0xff, 0xff, 0x1, 0xc7} },/* SA_IN(16), READ CAPACITY(16) */

Shouldn't the above change be folded into one of the other patches?

> @@ -529,9 +535,9 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>  	{0, 0x2f, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, NULL, NULL, /* VERIFY(10) */
>  	    {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7,
>  	     0, 0, 0, 0, 0, 0} },
> -	{1, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
> -	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0x9, 0xfe, 0,
> -		      0xff, 0xff, 0xff, 0xff} },/* VARIABLE LENGTH, READ(32) */
> +	{2, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
> +	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0x9, 0xfe, 0,
> +		0xff, 0xff, 0xff, 0xff} },    /* VARIABLE LENGTH, READ(32) */

Have you considered to use ARRAY_SIZE(vl_iarr) instead of hard-coding the array
size?

> +	if (cmd[0] == VARIABLE_LENGTH_CMD) {
> +		is_16 = false;
> +		wrprotect = (cmd[10] >> 5) & 0x7;
> +		lbdof = get_unaligned_be16(cmd + 12);
> +		num_lrd = get_unaligned_be16(cmd + 16);
> +		bt_len = get_unaligned_be32(cmd + 28);
> +		check_prot = false;
> +	} else {        /* that leaves WRITE SCATTERED(16) */
> +		is_16 = true;
> +		wrprotect = (cmd[2] >> 5) & 0x7;
> +		lbdof = get_unaligned_be16(cmd + 4);
> +		num_lrd = get_unaligned_be16(cmd + 8);
> +		bt_len = get_unaligned_be32(cmd + 10);
> +		check_prot = true;
> +	}

It's not clear to me why check_prot is set to false for WRITE SCATTERED(32)
and set to true for WRITE SCATTERED(16)?

Bart.