Thanks for the feedback Mike...
On Sun, 25 Oct 2020 20:14:42 -0500, Mike Christie wrote:
> On 10/23/20 3:57 PM, David Disseldorp wrote:
> > SBC-4 r15 5.3 COMPARE AND WRITE command states:
> > if the compare operation does not indicate a match, then terminate the
> > command with CHECK CONDITION status with the sense key set to
> > MISCOMPARE and the additional sense code set to MISCOMPARE DURING
> > VERIFY OPERATION. In the sense data (see 4.18 and SPC-5) the offset
> > from the start of the Data-Out Buffer to the first byte of data that
> > was not equal shall be reported in the INFORMATION field.
> >
> > This change implements the missing logic to report the miscompare offset
> > in the sense data INFORMATION field.
> >
> > Signed-off-by: David Disseldorp <ddiss@xxxxxxx>
> > ---
> > drivers/target/target_core_sbc.c | 35 ++++++++++++++++++--------
> > drivers/target/target_core_transport.c | 1 +
> > 2 files changed, 26 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 79216d0355e7..e40359e45726 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -435,13 +435,13 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
> > }
> >
> > /*
> > - * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare return
> > - * TCM_MISCOMPARE_VERIFY.
> > + * compare @cmp_len bytes of @read_sgl with @cmp_sgl. On miscompare, fill
> > + * @miscmp_off and return TCM_MISCOMPARE_VERIFY.
> > */
> > static sense_reason_t
> > compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
> > struct scatterlist *cmp_sgl, unsigned int cmp_nents,
> > - unsigned int cmp_len)
> > + unsigned int cmp_len, unsigned int *miscmp_off)
> > {
> > unsigned char *buf = NULL;
> > struct scatterlist *sg;
> > @@ -468,17 +468,20 @@ compare_and_write_do_cmp(struct scatterlist *read_sgl, unsigned int read_nents,
> > */
> > offset = 0;
> > for_each_sg(read_sgl, sg, read_nents, i) {
> > + unsigned int i;
> > unsigned int len = min(sg->length, cmp_len);
> > unsigned char *addr = kmap_atomic(sg_page(sg));
> >
> > - if (memcmp(addr, buf + offset, len)) {
> > - pr_warn("Detected MISCOMPARE for addr: %p buf: %p\n",
> > - addr, buf + offset);
> > - kunmap_atomic(addr);
> > + for (i = 0; i < len && addr[i] == buf[offset + i]; i++);
>
> I think it's a little nicer to put the ";" on the next line. It's just
> not a common line of code so your eyes miss it. It should also make the
> checkpatch script happy.
Sure, will change this.
> > +
> > + kunmap_atomic(addr);
> > + if (i < len) {
> > + *miscmp_off = offset + i;
> > + pr_warn("Detected MISCOMPARE at offset %u\n",
> > + *miscmp_off);
> > ret = TCM_MISCOMPARE_VERIFY;
> > goto out;
> > }
> > - kunmap_atomic(addr);
> >
> > offset += len;
> > cmp_len -= len;
> > @@ -503,6 +506,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
> > unsigned int nlbas = cmd->t_task_nolb;
> > unsigned int block_size = dev->dev_attrib.block_size;
> > unsigned int compare_len = (nlbas * block_size);
> > + unsigned int miscmp_off = 0;
> > sense_reason_t ret = TCM_NO_SENSE;
> > int i;
> >
> > @@ -534,8 +538,19 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
> > cmd->t_bidi_data_nents,
> > cmd->t_data_sg,
> > cmd->t_data_nents,
> > - compare_len);
> > - if (ret)
> > + compare_len,
> > + &miscmp_off);
> > + if (ret == TCM_MISCOMPARE_VERIFY) {
> > + /*
> > + * SBC-4 r15: 5.3 COMPARE AND WRITE command
> > + * In the sense data (see 4.18 and SPC-5) the offset from the
> > + * start of the Data-Out Buffer to the first byte of data that
> > + * was not equal shall be reported in the INFORMATION field.
> > + */
> > + WARN_ON(miscmp_off >= compare_len);
>
> I'm not sure how useful this is. If we hit this then we went wild in
> compare_and_write_do_cmp since we only allocate the cmp buffer to be
> compare_len bytes. If we think it's possible to hit this due to a
> incorrectly setup cmd or buffer/sgl or something, I would be more
> defensive in compare_and_write_do_cmp.
I don't think it's possible to hit, but figured it didn't harm
readability. I'll drop it in the next round.
Cheers, David
