On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote:
> On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote:
> > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote:
> > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI
> > > Data-Out buffer can differ from the size of the data area on the
> > > storage medium that is affected by the command. Make sure that
> > > the Data-Out buffer size is computed correctly if the BYTCHK
> > > field in the CDB is zero. This patch reverts commit 984a9d4c40be
> > > and thereby restores commit 0e2eb7d12eaa. Additionally,
> > > sbc_parse_cdb() is modified such that the data buffer size is
> > > computed correctly for the affected commands if BYTCHK == 0.
> > > This patch is the combination of two patches that got positive
> > > reviews.
> > >
> > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"")
> > > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
> > > Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> > > Cc: Hannes Reinecke <hare@xxxxxxxx>
> > > Cc: Christoph Hellwig <hch@xxxxxx>
> > > Cc: Andy Grover <agrover@xxxxxxxxxx>
> > > Cc: David Disseldorp <ddiss@xxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 67 insertions(+), 12 deletions(-)
> > >
> >
> > This patch ignored the review comments from the last round:
> >
> > http://www.spinics.net/lists/target-devel/msg15306.html
> > http://www.spinics.net/lists/target-devel/msg15327.html
> >
> > Until these are addressed as requested, dropping this patch for now.
>
> Hello Nic,
>
> In this patch series I have addressed all comments that made sense to me. Sorry
> if you feel offended because I had not addressed the two comments you referred to
> above. The reason I had not addressed these comments is because these comments
> are wrong in my opinion. Hence, please reconsider this patch.
Nope. Here are the details again.
First, it drops setting SCF_SCSI_DATA_CDB for WRITE_VERIFY in all cases,
and only sets it for BYTCHK=0.
Yes, I understand the spec says hosts are not supposed to send a payload
when BYTCHK=0, but that doesn't stop some from trying.
Any CDB that can potentially allocate SGLS via target_alloc_sgl() must
set this flag. No other CDBs set SCF_SCSI_DATA_CDB based on bits in the
CDB, and *_VERIFY is no exception.
Secondly, the force setting of size in sbc_parse_verify(), instead of
what was actually received over the write is totally wrong. Like I said
before, the size in sbc_parse_cdb() is what's extracted from the CDB
transfer length, and not what the spec says the correct size should be.
Tthat said, I already reverted this patch once because you didn't want
to make these very simple changes, so I'll not be merging it until the
changes are made.