From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"dgilbert@interlog.com" <dgilbert@interlog.com>
Cc: "hare@suse.de" <hare@suse.de>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>
Subject: Re: [PATCH 4/4] scsi_debug: add resp_write_scat function
Date: Tue, 12 Dec 2017 19:40:55 +0000 [thread overview]
Message-ID: <1513107652.2999.42.camel@sandisk.com> (raw)
In-Reply-To: <20171212011008.27689-5-dgilbert@interlog.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.
next prev parent reply other threads:[~2017-12-12 19:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-12 1:10 [PATCH 0/4] scsi_debug: add write scattered support Douglas Gilbert
2017-12-12 1:10 ` [PATCH 1/4] scsi_debug: tab, kstrto changes, requested by checkpatch.pl Douglas Gilbert
2017-12-12 18:32 ` Bart Van Assche
2017-12-12 1:10 ` [PATCH 2/4] scsi_debug: fix group_number mask 0x1f->0x3f Douglas Gilbert
2017-12-12 18:34 ` Bart Van Assche
2017-12-12 1:10 ` [PATCH 3/4] scsi_debug: expand do_device_access to take sg offset Douglas Gilbert
2017-12-12 18:35 ` Bart Van Assche
2017-12-12 1:10 ` [PATCH 4/4] scsi_debug: add resp_write_scat function Douglas Gilbert
2017-12-12 19:40 ` Bart Van Assche [this message]
2017-12-12 20:47 ` Douglas Gilbert
2017-12-13 19:35 ` Douglas Gilbert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1513107652.2999.42.camel@sandisk.com \
--to=bart.vanassche@wdc.com \
--cc=dgilbert@interlog.com \
--cc=hare@suse.de \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).