On 10/01/2013 11:45 PM, Sarah Sharp wrote:
On Tue, Oct 01, 2013 at 10:01:08PM +0300, Xenia Ragiadakou wrote:
Hi Sarah,
I read the mail on 'possible conflict between xhci_hcd and a patched
usbhid'.
For reference to others:
http://marc.info/?l=linux-usb&m=138064948726038&w=2
http://marc.info/?l=linux-usb&m=138065201426880&w=2
I looked in xhci and the problem arises in xhci_queue_intr_tx() when
if (xhci_interval != ep_interval) {
...
urb->interval = xhci_interval;
}
right?
Yes. The underlying problem is that the xHCI host sets up the endpoint
contexts during the Configure Endpoint command, using the interval from
the device's endpoint descriptors. It also uses the endpoint descriptor
wMaxPacketSize, which can be wrong as well. If the device driver wants
to use a different urb->interval than is in the endpoint descriptor, the
xHCI driver will simply ignore it.
(I'm Ccing the linux-media list, as I've discussed some of these devices
with broken descriptors before.)
When you say a new API, what do you mean? New functions in usbcore
to be used by usb device drivers?
Yes. You would export the function in the USB core, and put a prototype
in a USB include file (probably in include/linux/usb.h). Let's say that
function is called usb_change_ep_bandwidth.
Drivers could call into that function when they needed to change either
the bInterval or wMaxPacketSize of a particular endpoint. This could be
during the driver's probe function, or before switching alternate
interface settings, or even after the alt setting is in place, but
userspace dictates the driver use a different bandwidth.
Drivers should pass usb_change_ep_bandwidth a pointer to the endpoint
they need to change, along with the bInterval and wMaxPacketSize values
they would like the endpoint to have. Those values could be stored as
new values in struct usb_host_endpoint.
usb_change_ep_bandwidth would then call into the xHCI driver to drop the
endpoint, re-add it, and then issue a bandwidth change. The xHCI driver
would have to be changed to look at the new fields in usb_host_endpoint,
and set up the endpoint contexts with the interval and packet size from
those fields, instead of the endpoint descriptor.
We should probably set the new values in usb_host_endpoint to zero after
the driver unbinds from the device. Not sure if they should be reset
after the driver switches interfaces. I would have to see the use cases
in the driver.
Here, it is needed to change the endpoint descriptors with the new
value in urb so that xhci takes the correct value?
I mean the fix should be made in usbcore and xhci shall remain intact?
No, we need to fix both the xHCI driver and the USB core.
I have time to work on that but i 'm not sure that i understood.
Sure. I would actually suggest you first finish up the patch to issue a
configure endpoint if userspace wants to clear a halt, but the endpoint
isn't actually halted. Did your most current patch work? I can't
remember what the status was.
Sarah Sharp
Thanks for the clarification, I understand better now.
As far as concerns the reset endpoint fix, I am not sure if it works I
have to send an RFC so that it can be further tested but I have a lot of
pending RFCs for xhci on the mailing list so i was waiting for those to
be reviewed before sending new ones. Or it is not necessary to wait and
just send the RFC based on current usb-next tree?
regards,
ksenia
--
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
