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

[Harvey Harrison <harvey.harrison@gmail.com>]

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