On Friday 02 February 2007 19:05, you wrote:
> On Fri, 2 Feb 2007 17:32:24 -0800, Inaky Perez-Gonzalez
<[EMAIL PROTECTED]> wrote:
> > usb: descriptor structures have to be packed
> >
> > Many of the Wireless USB decriptors added to usb_ch9.h don't have the
> > __attribute__((packed)) tag, and thus, they don't reflect the wire
> > size. This patch fixes that.
>
> I am not cool with this.
>
> ...
>
> Byte sized members only, attribute is unnecessary.
Paranoia mode on, if sometime gcc decides to change its behaviour, we
would not have to worry about it.
> > @@ -523,7 +523,7 @@ struct usb_connection_context {
> > __u8 CHID[16]; /* persistent host id */
> > __u8 CDID[16]; /* device id (unique w/in host context) */
> > __u8 CK[16]; /* connection key */
> > -};
> > +} __attribute__((packed));
>
> Byte sized members only, attribute is unnecessary.
Well, those arrays could be aligned by the compiler differently (ok,
they are sized 16, which happens to be nice and dandy for the matter).
In any case, same as above.
> So, this leaves usb_security_descriptor, usb_bos_descriptor,
> usb_wireless_cap_descriptor, and usb_dev_cap_header. IMHO, our original
> setup was the correct one. The code should have used sizes such as
> USB_DT_CONFIG_SIZE instead of sizeof().
I have a problem with that, and is maintenance. Is not that I don't like
DT_CONFIG_SIZE at all (that I don't). I prefer to keep things centralized,
that being the descriptor definition, mapping the specification definition.
> It's elementary. The use of
> __attribute__((packed)) adds nontrivial costs... on some architectures.
It sure does, but does it offset the non-trivial costs we'd incur by
extracting the data manually? Or maybe is the compiler the one that is
broken by not being able to do automatically what we can do by hand?
[btw, I truly have little idea about which are those specific costs,
out of professional curiosity, got any pointers?]
If we are talking about misaligned access exceptions...ain't that a
compiler bug? compiler-generated asm that is accessing a packed
struct should take that into account, and unfold it as neccessary to
avoid misalignments.
Here is my point: it's way easier and more maintenable to do
struct some_descriptor {
__le16 foo;
__u8 bar;
} __attribute__((packed));
...
struct some_descriptor descr;
...
read_from_wire(&descr, sizeof(descr));
do_something_with_values(&descr->foo, &descr->bar)
than
u8 buffer[SOME_DESCRIPTOR_DT_SIZE], foo;
le16 bar;
read_from_wire(buffer, SOME_DESCRIPTOR_DT_SIZE);
bar = le16_to_cpu((__le16)buffer[1] << 8 | buffer[0]);
foo = buffer[2];
do_something_with_values(foo, bar)
The compiler should be able to do that translation and deal with
the alignment issues if any.
Now, overhead we'll have in every arch when accessing bytes from the
wire. Do we want to do that overhead by hand and make it a pain to
maintain or do we want the compiler to do that job? Frankly, method
#2 gives me the chills. It's ugly with a small descriptor, imagine
with a huge one.
Is not that I don't care about other arches (hey, I am a sw guy, I
like'em all to run well and my stock options won't climb again to
$130 no matter how many Intel CPUs you buy).
> I think the funniest part of this is that the architecture which suffers
> most from the abuse of __attribute__((packed)) is ia64 (with sparc being
> the second). And yet, Inaky works for Intel. Do we have to take this as
> a sign that Intel is abandoning Itanium? Poor, poor SGI. ^_^
Did I say that?
> I am not going to raise a stink myself, but I would like to point out
> the obvious. The packed attributes were added by people who do not
> care about non-x86, because the cost of __attribute__((packed)) is
> miniscule there (although, not zero either). The matter went so far
> that even usb_config_descriptor has the attribute now, even though
> it always had a perfectly useable size define.
How do suggest we tackle this issue?
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
