[related question regarding the SCSI-private endian helper needs at the end]
Currently on the read side, we have (le16 as an example endianness)
le16_to_cpup(__le16 *)
get_unaligned_le16(void *)
And on the write side:
*(__le16)ptr = cpu_to_le16(u16)
put_unaligned_le16(u16, void *);
On the read side, Al said he would have preferred the unaligned version
take the same types as the aligned, rather than void *. AKPM didn't think
the use of get_ was that great as get/put generally implies some kind of reference
taking in the kernel.
As the le16_to_cpup has been around for so long and is more recognizable, let's
make it the same for the unaligned case and typesafe:
le16_to_cpup(__le16 *)
unaligned_le16_to_cpup(__le16 *)
On the write side, the above get/put and type issues are still there, in addition AKPM felt
that the ordering of the put_unaligned parameters was opposite what was intuitive and that
the pointer should come first.
In this case, as there is currently no aligned helper (other than in some drivers defining macros)
define the api thusly:
Aligned:
write_le16(__le16 *ptr, u16 val)
Unaligned:
unaligned_write_le16(__le16 *ptr, u16 val)
The existing get/put_unaligned macros do not necessarily need to be modified. It would
present one oddity that the parameter order was different, but that's not a huge issue.
These could be phased in as they do not collide with the current API and the old api converted
over fairly quickly as not many places use the get/put_unaligned_{endian} helpers yet.
In addition, there are some subsystems (scsi) that are looking into some differently sized
endian helpers (be24) and it may be worthwhile to have some agreement whether it is worth
making them common infrastructure and whether they should present a similar API to the
common byteorder/unaligned API.
Is this a worthwhile direction to take?
Cheers,
Harvey
On Tue, 2008-10-07 at 14:53 -0700, Harvey Harrison wrote:
> [related question regarding the SCSI-private endian helper needs at the end]
>
> Currently on the read side, we have (le16 as an example endianness)
>
> le16_to_cpup(__le16 *)
> get_unaligned_le16(void *)
>
> And on the write side:
>
> *(__le16)ptr = cpu_to_le16(u16)
> put_unaligned_le16(u16, void *);
>
> On the read side, Al said he would have preferred the unaligned version
> take the same types as the aligned, rather than void *. AKPM didn't think
> the use of get_ was that great as get/put generally implies some kind of reference
> taking in the kernel.
>
> As the le16_to_cpup has been around for so long and is more recognizable, let's
> make it the same for the unaligned case and typesafe:
>
> le16_to_cpup(__le16 *)
> unaligned_le16_to_cpup(__le16 *)
>
> On the write side, the above get/put and type issues are still there, in addition AKPM felt
> that the ordering of the put_unaligned parameters was opposite what was intuitive and that
> the pointer should come first.
>
> In this case, as there is currently no aligned helper (other than in some drivers defining macros)
> define the api thusly:
>
> Aligned:
> write_le16(__le16 *ptr, u16 val)
>
> Unaligned:
> unaligned_write_le16(__le16 *ptr, u16 val)
Well, for us it would be even worse since now we'll have to do casting
to __beX just to shut sparse up, which makes the code even more
unreadable.
> The existing get/put_unaligned macros do not necessarily need to be modified. It would
> present one oddity that the parameter order was different, but that's not a huge issue.
>
> These could be phased in as they do not collide with the current API and the old api converted
> over fairly quickly as not many places use the get/put_unaligned_{endian} helpers yet.
>
> In addition, there are some subsystems (scsi) that are looking into some differently sized
> endian helpers (be24) and it may be worthwhile to have some agreement whether it is worth
> making them common infrastructure and whether they should present a similar API to the
> common byteorder/unaligned API.
>
> Is this a worthwhile direction to take?
I really don't think so. The current:
cmd[2] = X >> 24;
cmd[3] = X >> 16;
...
make sense to a storage person because they replicate exactly how the
standards are laid out. They're also clearer than the plethora of
unaligned_write_be32 etc.
The only real one we need an accessor for is 64 bits ... purely because
most people are too lazy to write all eight statements.
The optimisation argument is a bit of a myth ... the number of cycles
taken by these stores is dwarfed by the rest of the code path, even if
the compiler totally screws up the optimisation, which, realistically,
it shouldn't since it should know the platform native endianness in most
cases.
There was a fairly luke warm response to the idea of SCSI accessors
basically showing the level of apathy; certainly generic ones would be a
shade worse than this because now readers have to work out what they're
doing. In general, I'd prefer the SCSI stack just be left alone rather
than converted to accessors like this.
James
On Tue, 2008-10-07 at 18:12 -0400, James Bottomley wrote:
> On Tue, 2008-10-07 at 14:53 -0700, Harvey Harrison wrote:
> > [related question regarding the SCSI-private endian helper needs at the end]
> >
> > Currently on the read side, we have (le16 as an example endianness)
> >
> > le16_to_cpup(__le16 *)
> > get_unaligned_le16(void *)
> >
> > And on the write side:
> >
> > *(__le16)ptr = cpu_to_le16(u16)
> > put_unaligned_le16(u16, void *);
> >
> > On the read side, Al said he would have preferred the unaligned version
> > take the same types as the aligned, rather than void *. AKPM didn't think
> > the use of get_ was that great as get/put generally implies some kind of reference
> > taking in the kernel.
> >
> > As the le16_to_cpup has been around for so long and is more recognizable, let's
> > make it the same for the unaligned case and typesafe:
> >
> > le16_to_cpup(__le16 *)
> > unaligned_le16_to_cpup(__le16 *)
> >
> > On the write side, the above get/put and type issues are still there, in addition AKPM felt
> > that the ordering of the put_unaligned parameters was opposite what was intuitive and that
> > the pointer should come first.
> >
> > In this case, as there is currently no aligned helper (other than in some drivers defining macros)
> > define the api thusly:
> >
> > Aligned:
> > write_le16(__le16 *ptr, u16 val)
> >
> > Unaligned:
> > unaligned_write_le16(__le16 *ptr, u16 val)
>
> Well, for us it would be even worse since now we'll have to do casting
> to __beX just to shut sparse up, which makes the code even more
> unreadable.
Well, there are so many sparse warnings in scsi already, would a few
more really hurt?
To avoid the casts, you could also define some annotated, packed structs
to avoid the casting.
As an example, in the write command handling in achba.c, a patch similar to the following
(assumes the existence of a __be24 type somewhere):
Somewhere in a scsi header:
struct scsi_cmd_write6 {
u8 cmd;
__be24 lba;
u8 count;
} __packed
struct scsi_cmd_write16 {
u16 cmd;
__be64 lba;
__be32 count;
} __packed
struct scsi_cmd_write12 {
u16 cmd;
__be32 lba;
__be32 count;
u16 (harvey doesn't know scsi);
} __packed
struct scsi_cmd_write10 {
u16 cmd;
__be32 lba;
u8 (harvey doesn't know scsi);
__be16 count;
} __packed
ANd then notice that you don't need any casts even if the unaligned helpers
have strict types...that and the code gets a whole lot easier to read.
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index aa4e77c..5363a45 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1786,41 +1786,24 @@ static int aac_synchronize(struct scsi_cmnd *scsicmd)
u32 cmnd_count;
if (cmd->cmnd[0] == WRITE_6) {
- cmnd_lba = ((cmd->cmnd[1] & 0x1F) << 16) |
- (cmd->cmnd[2] << 8) |
- cmd->cmnd[3];
- cmnd_count = cmd->cmnd[4];
+ const struct scsi_cmd_write_6 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be24_to_cpup(&tmp->lba) & 0x001FFFFF;
+ cmnd_count = tmp->count;
+
if (cmnd_count == 0)
cmnd_count = 256;
} else if (cmd->cmnd[0] == WRITE_16) {
- cmnd_lba = ((u64)cmd->cmnd[2] << 56) |
- ((u64)cmd->cmnd[3] << 48) |
- ((u64)cmd->cmnd[4] << 40) |
- ((u64)cmd->cmnd[5] << 32) |
- ((u64)cmd->cmnd[6] << 24) |
- (cmd->cmnd[7] << 16) |
- (cmd->cmnd[8] << 8) |
- cmd->cmnd[9];
- cmnd_count = (cmd->cmnd[10] << 24) |
- (cmd->cmnd[11] << 16) |
- (cmd->cmnd[12] << 8) |
- cmd->cmnd[13];
+ const struct scsi_cmd_write_16 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be64_to_cpup(&tmp->lba);
+ cmnd_count = unaligned_be32_to_cpup(&tmp->count);
} else if (cmd->cmnd[0] == WRITE_12) {
- cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
- (cmd->cmnd[3] << 16) |
- (cmd->cmnd[4] << 8) |
- cmd->cmnd[5];
- cmnd_count = (cmd->cmnd[6] << 24) |
- (cmd->cmnd[7] << 16) |
- (cmd->cmnd[8] << 8) |
- cmd->cmnd[9];
+ const struct scsi_cmd_write_12 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be32_to_cpup(&tmp->lba);
+ cmnd_count = unaligned_be32_to_cpup(&tmp->count);
} else if (cmd->cmnd[0] == WRITE_10) {
- cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
- (cmd->cmnd[3] << 16) |
- (cmd->cmnd[4] << 8) |
- cmd->cmnd[5];
- cmnd_count = (cmd->cmnd[7] << 8) |
- cmd->cmnd[8];
+ const struct scsi_cmd_write_10 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be32_to_cpup(&tmp->lba);
+ cmnd_count = unaligned_be16_to_cpup(&tmp->count);
} else
continue;
if (((cmnd_lba + cmnd_count) < lba) ||
On Tue, Oct 07, 2008 at 02:53:11PM -0700, Harvey Harrison wrote:
> In addition, there are some subsystems (scsi) that are looking into some
> differently sized endian helpers (be24) and it may be worthwhile to have
> some agreement whether it is worth making them common infrastructure and
> whether they should present a similar API to the common byteorder/unaligned
> API.
I still think SCSI should have its own accessors, even if they're
just wrappers around the common BE code.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Tue, Oct 07, 2008 at 03:39:25PM -0700, Harvey Harrison wrote:
> As an example, in the write command handling in achba.c, a patch similar to the following
> (assumes the existence of a __be24 type somewhere):
What type could be defined to be '__be24'? Would
typedef struct {
unsigned char data[3];
} __be24;
do the trick? It's not an integral type, but I'm not sure if that matters.
> + cmnd_lba = unaligned_be24_to_cpup(&tmp->lba) & 0x001FFFFF;
That might be easier to read if you're using a 500-column terminal. For
the rest of us,
cmnd_lba = scsi_get_u24(cmnd + 2) & 0x1FFFFF;
is much easier to read.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Tue, 2008-10-07 at 17:28 -0600, Matthew Wilcox wrote:
> On Tue, Oct 07, 2008 at 02:53:11PM -0700, Harvey Harrison wrote:
> > In addition, there are some subsystems (scsi) that are looking into some
> > differently sized endian helpers (be24) and it may be worthwhile to have
> > some agreement whether it is worth making them common infrastructure and
> > whether they should present a similar API to the common byteorder/unaligned
> > API.
>
> I still think SCSI should have its own accessors, even if they're
> just wrappers around the common BE code.
>
I thought it was generally discouraged that subsystems have trivial
wrappers like that, otherwise you wind up with:
scsi_get_u32
usb_get_u32
v4l_get_u32
... and so on, where as if they all used the common names, people more
used to other areas of the kernel can still recognize what the code
is doing without having the lookup another define.
Just my 2 cents
Harvey
On Tue, 2008-10-07 at 17:33 -0600, Matthew Wilcox wrote:
> On Tue, Oct 07, 2008 at 03:39:25PM -0700, Harvey Harrison wrote:
> > As an example, in the write command handling in achba.c, a patch similar to the following
> > (assumes the existence of a __be24 type somewhere):
>
> What type could be defined to be '__be24'? Would
>
> typedef struct {
> unsigned char data[3];
> } __be24;
__packed
>
> do the trick? It's not an integral type, but I'm not sure if that matters.
>
> > + cmnd_lba = unaligned_be24_to_cpup(&tmp->lba) & 0x001FFFFF;
>
> That might be easier to read if you're using a 500-column terminal. For
> the rest of us,
> cmnd_lba = scsi_get_u24(cmnd + 2) & 0x1FFFFF;
> is much easier to read.
Suggestions welcome on shorter common names for the unaligned helpers.
Cheers,
Harvey
On Tue, Oct 07, 2008 at 04:35:42PM -0700, Harvey Harrison wrote:
> On Tue, 2008-10-07 at 17:28 -0600, Matthew Wilcox wrote:
> > On Tue, Oct 07, 2008 at 02:53:11PM -0700, Harvey Harrison wrote:
> > > In addition, there are some subsystems (scsi) that are looking into some
> > > differently sized endian helpers (be24) and it may be worthwhile to have
> > > some agreement whether it is worth making them common infrastructure and
> > > whether they should present a similar API to the common byteorder/unaligned
> > > API.
> >
> > I still think SCSI should have its own accessors, even if they're
> > just wrappers around the common BE code.
> >
>
> I thought it was generally discouraged that subsystems have trivial
> wrappers like that, otherwise you wind up with:
>
> scsi_get_u32
> usb_get_u32
> v4l_get_u32
>
> ... and so on, where as if they all used the common names, people more
> used to other areas of the kernel can still recognize what the code
> is doing without having the lookup another define.
My point is that they don't have to recognise what the code is doing.
They don't have to know if the protocol is BE or LE, only that USB code
is accessing the data in the appropriate way for USB.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Tue, 2008-10-07 at 17:55 -0600, Matthew Wilcox wrote:
> On Tue, Oct 07, 2008 at 04:35:42PM -0700, Harvey Harrison wrote:
> > On Tue, 2008-10-07 at 17:28 -0600, Matthew Wilcox wrote:
> > > On Tue, Oct 07, 2008 at 02:53:11PM -0700, Harvey Harrison wrote:
> > > > In addition, there are some subsystems (scsi) that are looking into some
> > > > differently sized endian helpers (be24) and it may be worthwhile to have
> > > > some agreement whether it is worth making them common infrastructure and
> > > > whether they should present a similar API to the common byteorder/unaligned
> > > > API.
> > >
> > > I still think SCSI should have its own accessors, even if they're
> > > just wrappers around the common BE code.
> > >
> >
> > I thought it was generally discouraged that subsystems have trivial
> > wrappers like that, otherwise you wind up with:
> >
> > scsi_get_u32
> > usb_get_u32
> > v4l_get_u32
> >
> > ... and so on, where as if they all used the common names, people more
> > used to other areas of the kernel can still recognize what the code
> > is doing without having the lookup another define.
>
> My point is that they don't have to recognise what the code is doing.
> They don't have to know if the protocol is BE or LE, only that USB code
> is accessing the data in the appropriate way for USB.
>
Or you go the packed-struct route and use the common helpers, and sparse tells
you if you use the wrong endianness. Even better, the structs act as decent
documentation of the data being worked on.
YMMV,
Harvey
On Tue, 7 Oct 2008, Harvey Harrison wrote:
> [related question regarding the SCSI-private endian helper needs at the end]
>
> Currently on the read side, we have (le16 as an example endianness)
>
> le16_to_cpup(__le16 *)
> get_unaligned_le16(void *)
>
> And on the write side:
>
> *(__le16)ptr = cpu_to_le16(u16)
> put_unaligned_le16(u16, void *);
>
> On the read side, Al said he would have preferred the unaligned version
> take the same types as the aligned, rather than void *. AKPM didn't think
As I said before, me too (take the same types as the aligned). I like to
rely on sparse for:
struct {
...
__le32 x;
...
} s __attribute__ ((packed));
get_unaligned_le16(&s.x);
> the use of get_ was that great as get/put generally implies some kind of reference
> taking in the kernel.
OK.
> As the le16_to_cpup has been around for so long and is more recognizable, let's
> make it the same for the unaligned case and typesafe:
>
> le16_to_cpup(__le16 *)
> unaligned_le16_to_cpup(__le16 *)
I always hated that naming...
> On the write side, the above get/put and type issues are still there, in addition AKPM felt
> that the ordering of the put_unaligned parameters was opposite what was intuitive and that
> the pointer should come first.
>
> In this case, as there is currently no aligned helper (other than in some drivers defining macros)
> define the api thusly:
>
> Aligned:
> write_le16(__le16 *ptr, u16 val)
>
> Unaligned:
> unaligned_write_le16(__le16 *ptr, u16 val)
Does it write to MMIO I/O space? No? Then please don't use write (like
in writeb()).
What about load_{unaligned_,}le16() and store_{unaligned_,}le16()?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, 7 Oct 2008, Harvey Harrison wrote:
> On Tue, 2008-10-07 at 17:33 -0600, Matthew Wilcox wrote:
> > On Tue, Oct 07, 2008 at 03:39:25PM -0700, Harvey Harrison wrote:
> > > As an example, in the write command handling in achba.c, a patch similar to the following
> > > (assumes the existence of a __be24 type somewhere):
> >
> > What type could be defined to be '__be24'? Would
> >
> > typedef struct {
> > unsigned char data[3];
> > } __be24;
>
> __packed
I don't think __packed would help here, as alignment(struct) ==
max(alignment(struct member)) == alignment(char).
Don't you want the __packed on the structure that embeds the __be24?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Matthew,
> > > On Tue, Oct 07, 2008 at 02:53:11PM -0700, Harvey Harrison wrote:
> > > > In addition, there are some subsystems (scsi) that are looking into some
> > > > differently sized endian helpers (be24) and it may be worthwhile to have
> > > > some agreement whether it is worth making them common infrastructure and
> > > > whether they should present a similar API to the common byteorder/unaligned
> > > > API.
> > >
> > > I still think SCSI should have its own accessors, even if they're
> > > just wrappers around the common BE code.
> > >
> >
> > I thought it was generally discouraged that subsystems have trivial
> > wrappers like that, otherwise you wind up with:
> >
> > scsi_get_u32
> > usb_get_u32
> > v4l_get_u32
> >
> > ... and so on, where as if they all used the common names, people more
> > used to other areas of the kernel can still recognize what the code
> > is doing without having the lookup another define.
>
> My point is that they don't have to recognise what the code is doing.
> They don't have to know if the protocol is BE or LE, only that USB code
> is accessing the data in the appropriate way for USB.
nice idea, but in the real world some stacks mix big and little endian
in their protocols. Take Bluetooth as an example ;)
Regards
Marcel
On Wed, 2008-10-08 at 09:13 +0200, Geert Uytterhoeven wrote:
> On Tue, 7 Oct 2008, Harvey Harrison wrote:
> > [related question regarding the SCSI-private endian helper needs at the end]
> >
> > Currently on the read side, we have (le16 as an example endianness)
> >
> > le16_to_cpup(__le16 *)
> > get_unaligned_le16(void *)
> >
> > And on the write side:
> >
> > *(__le16)ptr = cpu_to_le16(u16)
> > put_unaligned_le16(u16, void *);
> >
> > On the read side, Al said he would have preferred the unaligned version
> > take the same types as the aligned, rather than void *. AKPM didn't think
>
> As I said before, me too (take the same types as the aligned). I like to
> rely on sparse for:
>
> struct {
> ...
> __le32 x;
> ...
> } s __attribute__ ((packed));
>
> get_unaligned_le16(&s.x);
Agreed.
>
> > the use of get_ was that great as get/put generally implies some kind of reference
> > taking in the kernel.
>
> OK.
>
> > As the le16_to_cpup has been around for so long and is more recognizable, let's
> > make it the same for the unaligned case and typesafe:
> >
> > le16_to_cpup(__le16 *)
> > unaligned_le16_to_cpup(__le16 *)
>
> I always hated that naming...
True, but there are already lots of places that use them...and I didn't want to
introduce an identical name for something that already exists, so I worked using
the existing name. I think load_le16/load_unaligned_le16 is the best so far,
but I can see people being unhappy with the duplication of le16_to_cpup.
But it is trivial to move existing users over if that's the way the decision
goes.
>
> > On the write side, the above get/put and type issues are still there, in addition AKPM felt
> > that the ordering of the put_unaligned parameters was opposite what was intuitive and that
> > the pointer should come first.
> >
> > In this case, as there is currently no aligned helper (other than in some drivers defining macros)
> > define the api thusly:
> >
> > Aligned:
> > write_le16(__le16 *ptr, u16 val)
> >
> > Unaligned:
> > unaligned_write_le16(__le16 *ptr, u16 val)
>
> Does it write to MMIO I/O space? No? Then please don't use write (like
> in writeb()).
>
> What about load_{unaligned_,}le16() and store_{unaligned_,}le16()?
OK, will stay away from write as well. I think store looks good, with
load_ there is still a question of duplicating existing functionality.
Thanks for the feedback.
Harvey