Matthew Wilcox wrote:
> On Sun, Jun 22, 2008 at 03:41:56PM +0300, Boaz Harrosh wrote:
>>> + rbuf[4] = args->id[217] >> 8;
>>> + rbuf[5] = args->id[217];
>> args->id of struct ata_scsi_args are defined as u16.
>> Are they actually SWABed at this point? Are they LE? BE?
>
> They're cpu-endian at this point, I believe. See ata_dev_read_id() in
> libata-core.c where it calls swap_buf_le16().
>
>> if args->id are actually __be16 then above should be
>> + put_unaligned(args->id[217], &rbuf[4]);
>> else
>> + put_unaligned_be16(args->id[217], &rbuf[4]);
>
> But the buf isn't unaligned, nor the offset within it. And I
> get confused by all these macros anyway. If we had something like
> store_scsi_u16(unsigned char *, u16), I'd use that, but put_unaligned_be16
> just doesn't make sense.
>
If you look at the code _unaligned here means to fetch a byte pointer
char by char and swab by shifts, as opposed to SWABX instruction by
CPU. So what I proposed is code equivalent to what you did, and optimized
for BE systems.
If you actually calculated that they are aligned and want the extra CPU
help you will need a typecast as
*((__be16 *)&rbuf[4]) = cpu_to_be16(args->id[217]);
> I actually wrote my own accessors for scsi_ram. If they were to be more
> generic, they ought to be renamed, but this is what I found useful:
>
But all this work is done. No need to reinvent the wheel, which is my
point. Just change below scsi_xxx to beXX_ and your set.
> /*
> * SCSI requires quantities to be written MSB. They're frequently misaligned,
> * so don't mess about with cpu_to_beN, just access it byte-wise
> */
> static void scsi_ram_put_u32(unsigned char *addr, unsigned int data)
> {
> addr[0] = data >> 24;
> addr[1] = data >> 16;
> addr[2] = data >> 8;
> addr[3] = data;
> }
>
exactly same code as put_unaligned_be32 on LE, not optimized for BE.
(actually suboptimal for some LE systems as well)
> static unsigned int scsi_ram_get_u16(unsigned char *addr)
> {
> unsigned int data;
> data = addr[0] << 8;
> data |= addr[1];
>
> return data;
> }
>
dito get_unaligned_be16
> static unsigned int scsi_ram_get_u24(unsigned char *addr)
> {
> unsigned int data;
> data = addr[0] << 16;
> data |= addr[1] << 8;
> data |= addr[2];
>
> return data;
> }
>
OK You got me here
> static unsigned int scsi_ram_get_u32(unsigned char *addr)
> {
> unsigned int data;
> data = addr[0] << 24;
> data |= addr[1] << 16;
> data |= addr[2] << 8;
> data |= addr[3];
>
> return data;
> }
>
dito get_unaligned_be32
> I'm sure a more fully-featured set would include put_u24 and put_u16 too.
> For the curious, u24 is useful for implementing read6/write6.
>
Kernel's is much nicer then those "ntoul..." which make my headache. All we
need to remember is that SCSI is __bexx, and they are pretty intuitive,
I think.
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
