Re: PATCH: usb: descriptor structures have to be packed

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.

> +++ 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.

> @@ -395,7 +395,7 @@ struct usb_security_descriptor {
>  
>  	__le16 wTotalLength;
>  	__u8  bNumEncryptionTypes;
> -};
> +} __attribute__((packed));

This size is odd, but see below.

> @@ -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.

> @@ -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.

> @@ -437,7 +437,7 @@ struct usb_bos_descriptor {
>  
>  	__le16 wTotalLength;
>  	__u8  bNumDeviceCaps;
> -};
> +} __attribute__((packed));

This is the same as usb_security_descriptor.

> @@ -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.

> @@ -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.

> @@ -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.

> @@ -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.

> @@ -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.

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. The use of
__attribute__((packed)) adds nontrivial costs... on some architectures.

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. ^_^

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.

-- Pete

-------------------------------------------------------------------------
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