On Thu, Dec 30, 2010 at 08:31:23PM -0800, David Brownell wrote:
> Do you think there's any chance of getting the
> root cause fixed?
>
> As I recall, this was a workaround for a bad fix.
> Some descriptor (hub?) had two adjacent bitfields
> with one bit per port (?), and the bad fix removed
> a moderately clean representation thereof. (Maybe
> this is what a comment from your patch describes:
>
>
> - /* two bitmaps: ports removable, and usb 1.0 legacy PortPwrCtrlMask */
> + /* ports removable, and usb 1.0 legacy PortPwrCtrlMask */
Yes, the USB core defines the hub descriptor like so:
struct usb_hub_descriptor {
__u8 bDescLength;
__u8 bDescriptorType;
__u8 bNbrPorts;
__le16 wHubCharacteristics;
__u8 bPwrOn2PwrGood;
__u8 bHubContrCurrent;
/* add 1 bit for hub status change; round to bytes */
__u8 DeviceRemovable[(USB_MAXCHILDREN + 1 + 7) / 8];
__u8 PortPwrCtrlMask[(USB_MAXCHILDREN + 1 + 7) / 8];
} __attribute__ ((packed));
(Later this will be changed to a union, since USB 3.0 hubs have a
different descriptor.)
The DeviceRemovable and PortPwrCtrlMask fields are defined to be
variable length, depending on how many ports the hub has. Usually
there's only 7 ports, and both fields are only 8 bits long (one bit in
the first u8 is reserved). But if there's, say, 8 ports, the fields
should be 16 bits long, with two u8 DeviceRemovable fields first, then
two u8 PortPwrCtrlMask fields.
> Yes, that #define has always been ugly, but at the
> time it seemed a bit less ugly thanjust ignoring
> therelevant spec and disregarding the bitmaps.
The patch to remove the #define doesn't change the code in the host
controller at all, so it's not disregarding the spec. The #define
doesn't make it any easier to understand what's going on in the drivers,
IMO. It was one of the very confusing things when I was writing the
xHCI driver, which is why I decided to not use it. I can see now why
you would regard the last fields in the hub descriptor as one big
bitmap, but they're really *two* bitmaps with completely different
meanings.
Ideally, the USB core would allocate a variable-length array for the
descriptor, and have a better macro for calculating the offset into the
port information bitmasks. Something like:
struct usb_hub_descriptor {
__u8 bDescLength;
__u8 bDescriptorType;
__u8 bNbrPorts;
__le16 wHubCharacteristics;
__u8 bPwrOn2PwrGood;
__u8 bHubContrCurrent;
/* add 1 bit for hub status change; round to bytes */
__u8 *port_info_bitmask;
} __attribute__ ((packed));
#define USB_HUB_DR_PORT_INDEX(hdev, n) ((n) / 8))
#define USB_HUB_PPCM_PORT_INDEX(hdev, n) (((hdev->children + 1) / 8) + 1 + ((n) / 8)))
Then drivers could access those fields like so:
hub_desc->port_info_bitmask[USB_DR_PORT_INDEX(hdev, n)]
hub_desc->port_info_bitmask[USB_HUB_PPCM_PORT_INDEX(hdev, n)]
But I'm not sure if we the USB core to go to the trouble of allocating
variable-length hub descriptors.
The #define (as-is) really needs to get removed. bitmap wasn't a
good word to use as a #define, and it could hide bugs elsewhere. I'd
like to leave the patch as-is, and then possibly revisit the hub
descriptor representation later.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
