[PATCH] scsi: sd_dif.c use unaligned access helpers

[PATCH] scsi: sd_dif.c use unaligned access helpers

[Harvey Harrison]

This code should be looked at carefully, while this patch doesn't
change any behaviour, the handling of app_tag, ref_tag seems odd.
The struct defines both as __be16/32 but in these locations it is
read in/written out as cpu-byteorder.

It looks like they should actually be u16/u32.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 drivers/scsi/sd_dif.c |   26 ++++++++------------------
 1 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 4d17f3d..e54711e 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -22,6 +22,7 @@
 
 #include <linux/blkdev.h>
 #include <linux/crc-t10dif.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -142,11 +143,10 @@ static int sd_dif_type1_verify_ip(struct blk_integrity_exchg *bix)
 static void sd_dif_type1_set_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
 	unsigned int i, j;
 
 	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {
-		sdt->app_tag = tag[j] << 8 | tag[j+1];
+		sdt->app_tag = get_unaligned_be16(tag_buf + j);
 		BUG_ON(sdt->app_tag == 0xffff);
 	}
 }
@@ -154,13 +154,10 @@ static void sd_dif_type1_set_tag(void *prot, void *tag_buf, unsigned int sectors
 static void sd_dif_type1_get_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
 	unsigned int i, j;
 
-	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {
-		tag[j] = (sdt->app_tag & 0xff00) >> 8;
-		tag[j+1] = sdt->app_tag & 0xff;
-	}
+	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++)
+		put_unaligned_be16(sdt->app_tag, tag_buf + j);
 }
 
 static struct blk_integrity dif_type1_integrity_crc = {
@@ -256,29 +253,22 @@ static int sd_dif_type3_verify_ip(struct blk_integrity_exchg *bix)
 static void sd_dif_type3_set_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
 	unsigned int i, j;
 
 	for (i = 0, j = 0 ; i < sectors ; i++, j += 6, sdt++) {
-		sdt->app_tag = tag[j] << 8 | tag[j+1];
-		sdt->ref_tag = tag[j+2] << 24 | tag[j+3] << 16 |
-			tag[j+4] << 8 | tag[j+5];
+		sdt->app_tag = get_unaligned_be16(tag_buf + j);
+		sdt->ref_tag = get_unaligned_be32(tag_buf + 2 + j);
 	}
 }
 
 static void sd_dif_type3_get_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
 	unsigned int i, j;
 
 	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {
-		tag[j] = (sdt->app_tag & 0xff00) >> 8;
-		tag[j+1] = sdt->app_tag & 0xff;
-		tag[j+2] = (sdt->ref_tag & 0xff000000) >> 24;
-		tag[j+3] = (sdt->ref_tag & 0xff0000) >> 16;
-		tag[j+4] = (sdt->ref_tag & 0xff00) >> 8;
-		tag[j+5] = sdt->ref_tag & 0xff;
+		put_unaligned_be16(sdt->app_tag, tag_buf + j);
+		put_unaligned_be32(sdt->ref_tag, tag_buf + 2 + j);
 		BUG_ON(sdt->app_tag == 0xffff || sdt->ref_tag == 0xffffffff);
 	}
 }
-- 
1.5.6.4.570.g052e

Re: [PATCH] scsi: sd_dif.c use unaligned access helpers

[Martin K. Petersen]

>>>>> "Harvey" == Harvey Harrison <harvey.harrison@gmail.com> writes:

[Sorry about the delayed response.  I've been traveling for a few
days.]

Harvey> This code should be looked at carefully, while this patch
Harvey> doesn't change any behaviour, the handling of app_tag, ref_tag
Harvey> seems odd.  The struct defines both as __be16/32 but in these
Harvey> locations it is read in/written out as cpu-byteorder.

The opaque tag space (app in Type 1+2, app + ref in Type 3) is
provided for use by filesystems.  The SCSI layer doesn't know anything
about the contents of the tag buffer.  It has no idea whether it
contains bytes, shorts, ints or longs.  So we can't swab on the
assumption that each DIF tuple contains an u16.  Just like we don't
byteswap any other data coming down from above.

Filesystems must prepare tags in an endianness-aware fashion just like
they prepare normal filesystem metadata.  And at the SCSI layer we
treat it as a byte stream.


Harvey> It looks like they should actually be u16/u32.

Guard + ref tags are interpreted by the HBA and storage device and
they are defined to be big endian.

Consequently the DIF tuple tags are defined as __be16 and __be32 to be
in sync with the spec.  But from a protocol perspective the notion of
endianness of the application tag is essentially meaningless.  It is
not interpreted by anybody else than the filesystem.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: sd_dif.c use unaligned access helpers

[Harvey Harrison]

On Sat, 2008-07-26 at 00:22 -0400, Martin K. Petersen wrote:
> >>>>> "Harvey" == Harvey Harrison <harvey.harrison@gmail.com> writes:
> 
> [Sorry about the delayed response.  I've been traveling for a few
> days.]
> 
> Harvey> This code should be looked at carefully, while this patch
> Harvey> doesn't change any behaviour, the handling of app_tag, ref_tag
> Harvey> seems odd.  The struct defines both as __be16/32 but in these
> Harvey> locations it is read in/written out as cpu-byteorder.
> 
> The opaque tag space (app in Type 1+2, app + ref in Type 3) is
> provided for use by filesystems.  The SCSI layer doesn't know anything
> about the contents of the tag buffer.  It has no idea whether it
> contains bytes, shorts, ints or longs.  So we can't swab on the
> assumption that each DIF tuple contains an u16.  Just like we don't
> byteswap any other data coming down from above.
> 
> Filesystems must prepare tags in an endianness-aware fashion just like
> they prepare normal filesystem metadata.  And at the SCSI layer we
> treat it as a byte stream.

OK.  But everywhere in this file app_tag is treated as host-order,
and causes no end of sparse warnings.

Looking just at type1 (with some comments):

static void sd_dif_type1_set_tag(void *prot, void *tag_buf, unsigned int sectors)
{
	struct sd_dif_tuple *sdt = prot;
	char *tag = tag_buf;
	unsigned int i, j;

	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {
//app_tag is host-order, read in as raw byte in be-order
		sdt->app_tag = tag[j] << 8 | tag[j+1];
		BUG_ON(sdt->app_tag == 0xffff);
	}
}

static void sd_dif_type1_get_tag(void *prot, void *tag_buf, unsigned int sectors)
{
	struct sd_dif_tuple *sdt = prot;
	char *tag = tag_buf;
	unsigned int i, j;

	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {
//app_tag is host order again, read out in be-byteorder
		tag[j] = (sdt->app_tag & 0xff00) >> 8;
		tag[j+1] = sdt->app_tag & 0xff;
	}
}

So who cares what the spec says, the code is treating it as a host-order
u16 everywhere.  Similarly for the ref_tag.  So if you're going to 
manipulate them in host-order, just declare the struct in host order.

Anyways, maybe I'm mistaken here.

Harvey

Re: [PATCH] scsi: sd_dif.c use unaligned access helpers

[Martin K. Petersen]

>>>>> "Harvey" == Harvey Harrison <harvey.harrison@gmail.com> writes:

Harvey> So who cares what the spec says, the code is treating it as a
Harvey> host-order u16 everywhere.  Similarly for the ref_tag.

Hold on.  We don't know the personality of the ref_tag.

This is convoluted and I totally agree it's icky.

Each DIF tuple consists of 8 bytes.  The interpretation of those 8
bytes depends on how the disk is formatted.  For Type 1, 2 and 3 the
first two bytes contain the guard_tag, a big endian CRC of the data
portion of the sector.  That part is easy.

The next two bytes are defined to be for use by the application client
(i.e. OS).  The current SCSI spec doesn't say anything about the
contents or how they should be interpreted.

The last four bytes (reference tag) are defined as a big endian entity
for Type 1 and 2.  But with Type 3 the last four bytes become part of
the opaque tag space.  You can view is as they get combined with the
app tag to provide 6 bytes of space.

IOW, for Type 1 + 2 the ref tag is big endian.  For Type 3 it is
undefined.

Also, for DIF Type 4 that's currently being discussed the application
tag will be used for a hash that's defined to be big endian.

So the problem is that the endianness depends on how the attached disk
is formatted.

For now we can switch the app_tag to be host endian.  The only
workaround I can think of for the ref_tag is to introduce struct
sd_dif_tuple_type12 and struct sd_dif_tuple_type3 which use __be32 and
u32 respectively.

I'll muck with that later.  But first I have to finish my OLS
slides...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: sd_dif.c use unaligned access helpers

[Harvey Harrison]

On Sat, 2008-07-26 at 08:26 -0400, Martin K. Petersen wrote:
> >>>>> "Harvey" == Harvey Harrison <harvey.harrison@gmail.com> writes:
> 
> Harvey> So who cares what the spec says, the code is treating it as a
> Harvey> host-order u16 everywhere.  Similarly for the ref_tag.
> 
> Hold on.  We don't know the personality of the ref_tag.
> 
> This is convoluted and I totally agree it's icky.
> 
> Each DIF tuple consists of 8 bytes.  The interpretation of those 8
> bytes depends on how the disk is formatted.  For Type 1, 2 and 3 the
> first two bytes contain the guard_tag, a big endian CRC of the data
> portion of the sector.  That part is easy.
> 
> The next two bytes are defined to be for use by the application client
> (i.e. OS).  The current SCSI spec doesn't say anything about the
> contents or how they should be interpreted.
> 
> The last four bytes (reference tag) are defined as a big endian entity
> for Type 1 and 2.  But with Type 3 the last four bytes become part of
> the opaque tag space.  You can view is as they get combined with the
> app tag to provide 6 bytes of space.

Ahh, reading what you originally wrote I see what you mean now.

> 
> IOW, for Type 1 + 2 the ref tag is big endian.  For Type 3 it is
> undefined.
> 
> Also, for DIF Type 4 that's currently being discussed the application
> tag will be used for a hash that's defined to be big endian.
> 
> So the problem is that the endianness depends on how the attached disk
> is formatted.
> 
> For now we can switch the app_tag to be host endian.  The only
> workaround I can think of for the ref_tag is to introduce struct
> sd_dif_tuple_type12 and struct sd_dif_tuple_type3 which use __be32 and
> u32 respectively.

That's probably not too bad of an idea.  Thanks for taking the time
to explain.

> 
> I'll muck with that later.  But first I have to finish my OLS
> slides...

Good luck.

Harvey