On Tue, Aug 23, 2011 at 7:13 PM, Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
>
> Bcc:
> Subject: Re: [RFC][PATCH] usb: use usb_endpoint_maxp() instead of
> le16_to_cpu()
> Reply-To:
> In-Reply-To: <87mxf0mo7s.wl%kuninori.morimoto.gx@xxxxxxxxxxx>
> X-Key-Id: 97C4700B
> X-Key-Fingerprint: 09E2 D1F3 9A3A FF13 C3D3 961C 0688 1C1E 97C4 700B
>
> [cut some people from cc ]
>
> * Kuninori Morimoto | 2011-08-23 03:12:03 [-0700]:
>
> >index 0149c09..d956965 100644
> >--- a/drivers/usb/core/devices.c
> >+++ b/drivers/usb/core/devices.c
> >@@ -190,7 +190,7 @@ static char *usb_dump_endpoint_descriptor(int speed, char *start, char *end,
> > dir = usb_endpoint_dir_in(desc) ? 'I' : 'O';
> >
> > if (speed == USB_SPEED_HIGH) {
> >- switch (le16_to_cpu(desc->wMaxPacketSize) & (0x03 << 11)) {
> >+ switch (usb_endpoint_maxp(desc) & (0x03 << 11)) {
> > case 1 << 11:
> > bandwidth = 2; break;
> > case 2 << 11:
> >@@ -240,7 +240,7 @@ static char *usb_dump_endpoint_descriptor(int speed, char *start, char *end,
> >
> > start += sprintf(start, format_endpt, desc->bEndpointAddress, dir,
> > desc->bmAttributes, type,
> >- (le16_to_cpu(desc->wMaxPacketSize) & 0x07ff) *
> >+ (usb_endpoint_maxp(desc) & 0x07ff) *
> > bandwidth,
> > interval, unit);
> > return start;
> >diff --git a/drivers/usb/gadget/net2272.c b/drivers/usb/gadget/net2272.c
> >index ab98ea9..6fef1c0 100644
> >--- a/drivers/usb/gadget/net2272.c
> >+++ b/drivers/usb/gadget/net2272.c
> >@@ -204,7 +204,7 @@ net2272_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
> > if (!dev->driver || dev->gadget.speed == USB_SPEED_UNKNOWN)
> > return -ESHUTDOWN;
> >
> >- max = le16_to_cpu(desc->wMaxPacketSize) & 0x1fff;
> >+ max = usb_endpoint_maxp(desc) & 0x1fff;
> >
> > spin_lock_irqsave(&dev->lock, flags);
> > _ep->maxpacket = max & 0x7fff;
> >diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> >index d446886..d873a03 100644
> >--- a/drivers/usb/host/xhci-mem.c
> >+++ b/drivers/usb/host/xhci-mem.c
> >@@ -1141,8 +1141,8 @@ static u32 xhci_get_max_esit_payload(struct xhci_hcd *xhci,
> > if (udev->speed == USB_SPEED_SUPER)
> > return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
> >
> >- max_packet = GET_MAX_PACKET(le16_to_cpu(ep->desc.wMaxPacketSize));
> >- max_burst = (le16_to_cpu(ep->desc.wMaxPacketSize) & 0x1800) >> 11;
> >+ max_packet = GET_MAX_PACKET(usb_endpoint_maxp(&ep->desc));
> >+ max_burst = (usb_endpoint_maxp(&ep->desc) & 0x1800) >> 11;
> > /* A 0 in max burst means 1 transfer per ESIT */
> > return max_packet * (max_burst + 1);
> > }
>
> We have three masks here which are common:
> - 0x07ff: only max packet size
> - 0x1fff: max packet size + additional microframes
> - 0x1800: only the additional microframes
>
> I don't know if net2272 really meant to get both fields or only one.
> From the context I see that it tried to set ->maxpacket to the maxpacket
> but it got one f too much.
> From looking at the xhci code it seems to make sense to include that
> GET_MAX_PACKET() macro into usb_endpoint_maxp() and add another inline
> function for the "0x1800 >> 11" magic. So the switch case would not
> require that "1 << 11".
> Any comments?
>
The GET_MAX_PACKET() in xhci is introduced to fix a bug (originally masked with
0x03ff, not 0x07ff). I suggest introducing two macros to deal with
these 0x07ff and
0x1800 >> 11 things in usb subsystem so many drivers can benefit: usbcore, ehci
all use mask 0x07ff directly.
Thanks,
Andiry
--
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
