On Friday 02 February 2007 7:05 pm, Pete Zaitcev 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.
Actually I'm fine with this overdue patch. The only issue with it is
that it conflicts with the patch moving that file to <linux/usb/ch9.h>;
easily resolved.
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
However: Inaky, if you refresh this patch to go after the "move ch9.h"
patch, I'd suggest you add a short comment to the top explaining that
all the descriptors use "packed" so that [a] they never get padded,
either internally (USB spec writers probably handled that) or externally;
[b] so that accessing bigger-than-a-bytes fields will never generate
bus errors on any platform, even when the location of its descriptor
inside a bundle isn't "naturally aligned", and [c] for consistency,
removing all doubt even when it appears to someone that the two other
points are non-issues for that particular descriptor type.
> > +++ b/include/linux/usb_ch9.h Fri Feb 02 17:17:09 2007 -0800
> > @@ -366,7 +366,7 @@ struct usb_debug_descriptor {
> > /* bulk endpoints with 8 byte maxpacket */
> > __u8 bDebugInEndpoint;
> > __u8 bDebugOutEndpoint;
> > -};
> > +} __attribute__((packed));
>
> This is a 4-byte structure. The attribute is unnecessary.
So "doesn't matter here" except for [c] consistency;
it's inconsistent _not_ to have it, and people
would waste timer wondering why.
> > @@ -395,7 +395,7 @@ struct usb_security_descriptor {
> >
> > __le16 wTotalLength;
> > __u8 bNumEncryptionTypes;
> > -};
> > +} __attribute__((packed));
>
> This size is odd, but see below.
Right, so finding a pointer to one of these inside a group of
other descriptors means that one of the adjacent descriptors
is not "naturally aligned" ... and therefor that without the
annotation of "packed", accessing members of the non-aligned
struct **will oops** on various platforms [b].
> > @@ -409,7 +409,7 @@ struct usb_key_descriptor {
> > __u8 tTKID[3];
> > __u8 bReserved;
> > __u8 bKeyData[0];
> > -};
> > +} __attribute__((packed));
>
> This structure has no members larger than a byte, so the attribute
> is unnecessary.
Except for consistency [c]. I'd certainly wonder what the criteria
were if I found a random collection of descriptors missing such
annotations.
> > @@ -425,7 +425,7 @@ struct usb_encryption_descriptor {
> > #define USB_ENC_TYPE_RSA_1 3 /* rsa3072/sha1 auth */
> > __u8 bEncryptionValue; /* use in SET_ENCRYPTION */
> > __u8 bAuthKeyIndex;
> > -};
> > +} __attribute__((packed));
>
> Ditto.
Ditto; it's stil the Right Thing To Do [c].
> > @@ -437,7 +437,7 @@ struct usb_bos_descriptor {
> >
> > __le16 wTotalLength;
> > __u8 bNumDeviceCaps;
> > -};
> > +} __attribute__((packed));
>
> This is the same as usb_security_descriptor.
Sure looks like a different type to me. The two types are
not interchangeable. The spec has both, for good reason:
they serve distinct roles.
Oh you mean "same _issue_ as..."? So [b] applies; oopsing bad.
> > @@ -446,7 +446,7 @@ struct usb_dev_cap_header {
> > __u8 bLength;
> > __u8 bDescriptorType;
> > __u8 bDevCapabilityType;
> > -};
> > +} __attribute__((packed));
>
> This one might need the attribute because it's smaller than 4 bytes.
You misunderstand the point of having the attribute. It's not
purely about size. It's also about alignment [b] of all descriptors
in a bundle.
> > @@ -474,7 +474,7 @@ struct usb_wireless_cap_descriptor { /*
> > __u8 bmFFITXPowerInfo; /* FFI power levels */
> > __le16 bmBandGroup;
> > __u8 bReserved;
> > -};
> > +} __attribute__((packed));
>
> This is the same as usb_security_descriptor.
Hardly. Only one has a bmFFITXPowerInfo field, for starters.
Also [b] applies; oopsing bad.
> > @@ -495,7 +495,7 @@ struct usb_wireless_ep_comp_descriptor {
> > #define USB_ENDPOINT_SWITCH_NO 0
> > #define USB_ENDPOINT_SWITCH_SWITCH 1
> > #define USB_ENDPOINT_SWITCH_SCALE 2
> > -};
> > +} __attribute__((packed));
>
> This descriptor is naturally aligned, attribute is unnecessary.
In the middle of a buffer full of descriptors, there's no way
to guarantee that it will actually be aligned. The reason to
have a "packed" annotation is [b] so that the compiler will generate
the appropriate code ... e.g. accessing wOverTheAirPacketSize
will never oops, even on CPUs that require __le16 values always
live on 2-byte boundaries.
> > @@ -511,7 +511,7 @@ struct usb_handshake {
> > __u8 CDID[16];
> > __u8 nonce[16];
> > __u8 MIC[8];
> > -};
> > +} __attribute__((packed));
>
> Byte sized members only, attribute is unnecessary.
... and couldn't hurt either; [c]. Might even help, if some
compilers try to "know" that all those fields live on
particular boundaries and optimize accesses accordingly.
> > @@ -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.
... and thus couldn't hurt [c].
> 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(). It's elementary.
You persist in mis-understanding the point of using "packed". It's
elementary, see the list at the top of this response. You continue
to assume only reason [a] matters. But [b] is for example the reason
we don't have to audit every single frelling line of code touching
any kind of USB descriptor bundle, seeking out oopsable idioms. And
[c] is what gives us the assurance that [a] and [b] are non-issues
pretty much universally.
> The use of
> __attribute__((packed)) adds nontrivial costs... on some architectures.
Think about what those costs are. Byte access? No cost. But for
values of type "__le16" or "__le32", that cost is nothing more than
a compiler-generated version of an unaligned access ... rather than
having an explicit one (macro or open-coded) in the source code.
Now, in the tradeoff between in the one case auditing every line of
code in the Linux kernel (and to some extent in userspace) for whether
they use the right unaligned access idiom -- and then patching every
line that's wrong, then deploying those patches -- or, in the other
case, just not having to worry about the issue. Let me suggest that
the former case is Just Not Gonna Happen.
> 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.
I've explained this all before, you still refuse to understand.
Struct size is only one of several factors.
And the non-x86 folk care a lot more [1] about those other factors,
since they're a lot more likely to run on CPUs which don't do
hardware fixups for unaligned access ... the ones prevented by
using the "packed" attribute that way.
- Dave
[1] Speaking of which ... I know a company that needs some GCC
work done to handle various issues, including one seemingly
relaated to bugs in "packed" for their arch. For anyone
interested and capable: please drop me a email privately.
-------------------------------------------------------------------------
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
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
