Sujith <m.sujith@...> writes:
> Jussi Kivilinna wrote:
> > +static int usb_endpoint_int_to_bulk(struct usb_device *udev, unsigned int pipe)
> > +{
> > + struct usb_host_endpoint *ep;
> > +
> > + ep = usb_pipe_endpoint(udev, pipe);
> > + if (!ep)
> > + return -EPIPE;
> > +
> > + switch (ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
> > + case USB_ENDPOINT_XFER_INT:
> > + ep->desc.bmAttributes &= ~USB_ENDPOINT_XFERTYPE_MASK;
> > + ep->desc.bmAttributes |= USB_ENDPOINT_XFER_BULK;
> > + ep->desc.bInterval = 0;
> > + /* passthru */
Interesting, I know that interrupt and bulk endpoints share a lot of
common logic so I can see how this should work. But is it really
possible [and more to the point: sane?!] to override the endpoint
descriptors?
I know that usb_submit_urb checks [when config_usb_debug is enabled]
if the xfertype of the submitted urb matches the one of the endpoint,
so I wonder I started to wonder if we are fooling "just" the code
here, or ourselves in the process?
> ath9k_htc does the same thing (see ath/ath9k/hif_usb.c).
> It does seem to be working properly but I am wondering if the
> USB subsystem has to be updated after the driver messes around with bmAttributes.
> (lsusb would still show the endpoint type as INT).
>
> You have a usb_reset_configuration() call before changing
> the endpoint type, why is that necessary ? And would that
> reset the endpoints as well ?
Huh, has this already catched on?
--
Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir
belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de
On Sun, 6 Feb 2011, Sujith wrote:
> Martin Schleier wrote:
> > Interesting, I know that interrupt and bulk endpoints share a lot of
> > common logic so I can see how this should work. But is it really
> > possible [and more to the point: sane?!] to override the endpoint
> > descriptors?
> >
> > I know that usb_submit_urb checks [when config_usb_debug is enabled]
> > if the xfertype of the submitted urb matches the one of the endpoint,
> > so I wonder I started to wonder if we are fooling "just" the code
> > here, or ourselves in the process?
> >
> > > ath9k_htc does the same thing (see ath/ath9k/hif_usb.c).
> > > It does seem to be working properly but I am wondering if the
> > > USB subsystem has to be updated after the driver messes around with bmAttributes.
> > > (lsusb would still show the endpoint type as INT).
> > >
> > > You have a usb_reset_configuration() call before changing
> > > the endpoint type, why is that necessary ? And would that
> > > reset the endpoints as well ?
> > Huh, has this already catched on?
>
> Well, for ath9k_htc, the FW changes the descriptor type after it is
> transferred to the target. The USB devs suggested that usb_device_reset()
> should be called by the driver in such cases, but in ath9k_htc's case,
> the device is disconnected and the error message "device firmware changed"
> is displayed.
>
> Agreed, this is a dirty hack - but is there any way for a driver to make the USB
> subsystem re-configure the endpoints ?
Why do you want to? Why does changing the endpoint type from interrupt
to bulk improve performance? Yes, the description says that this
causes CPU usage to go from 10% to below 1%, but _why_ does it do that?
Maybe the same effect can be achieved simply by changing the interrupt
interval instead.
Alan Stern
On Sun, 6 Feb 2011, Sujith wrote:
> Alan Stern wrote:
> > Why do you want to? Why does changing the endpoint type from interrupt
> > to bulk improve performance? Yes, the description says that this
> > causes CPU usage to go from 10% to below 1%, but _why_ does it do that?
> >
> > Maybe the same effect can be achieved simply by changing the interrupt
> > interval instead.
>
> Well, when the device is plugged in, the USB core recognizes the endpoints
> as interrupt type. Later, in the probe() callback, the FW is uploaded to the
> target and it patches the endpoints' type to bulk. Please correct me if am
> wrong, but I think this mismatch between the host and the target would be
> the cause for the high CPU usage, when the transmission load for that
> endpoint is high.
(Actually, bulk transfers generally cause a higher CPU load than
interrupt transfers. But never mind that...)
The problem is that the kernel doesn't know the firmware has been
changed. The kernel has to be told about this, one way or another.
Typically the device would disconnect itself from the USB bus briefly,
when starting to run the new firmware. Or the kernel would be told to
reset the device after the firmware was transferred, after which the
device would start running the new firmware.
Apparently the driver needs to call usb_reset_device() when the
firmware has been transferred. Following the reset, the kernel will
see that the descriptors have changed and it will reload all of them.
Then it will know that the endpoint is bulk rather than interrupt.
Alan Stern
Quoting Alan Stern <[email protected]>:
>> but I know that vendor driver uses these
>> endpoints with bulk urbs and using those solve/workaround CPU usage
>> problem. Increasing interrupt interval from 1 to 255 didn't help.
>>
>> I think zd1211rw have been trying to use bulk urbs too, but
>> usb_bulk_msg() silently switches to interrupt mode. I don't know if
>> editing endpoint descriptors is right way solve this, or why CPU usage
>> is higher.
>>
>> CPU usage comes from writing beacon frame to device, this is done
>> usually every 100ms on AP-mode. On Atom writing beacon (multiple
>> usb_bulk_msg() calls) with interrupt endpoints usually takes ~20msec
>> and this is when CPU usage goes up. By changing endpoints to bulk,
>> time to write beacon drops to 0~3msec.
>
> That's part of the problem -- the driver shouldn't use usb_bulk_msg().
> Create a bunch of URBs and submit them explicitly all at once; don't
> wait for one to finish before submitting the next. Setting the
> URB_NO_INTERRUPT flag on all but the last could also help. This might
> use even less CPU time than you see currently.
>
I had been experimenting with making just that, bunch of URBs, not
waiting until end. Now I tested with URB_NO_INTERRUPT on all except
last urb and CPU usage dropped from 10% to 2%. Thanks :)
-Jussi
Martin Schleier wrote:
> Interesting, I know that interrupt and bulk endpoints share a lot of
> common logic so I can see how this should work. But is it really
> possible [and more to the point: sane?!] to override the endpoint
> descriptors?
>
> I know that usb_submit_urb checks [when config_usb_debug is enabled]
> if the xfertype of the submitted urb matches the one of the endpoint,
> so I wonder I started to wonder if we are fooling "just" the code
> here, or ourselves in the process?
>
> > ath9k_htc does the same thing (see ath/ath9k/hif_usb.c).
> > It does seem to be working properly but I am wondering if the
> > USB subsystem has to be updated after the driver messes around with bmAttributes.
> > (lsusb would still show the endpoint type as INT).
> >
> > You have a usb_reset_configuration() call before changing
> > the endpoint type, why is that necessary ? And would that
> > reset the endpoints as well ?
> Huh, has this already catched on?
Well, for ath9k_htc, the FW changes the descriptor type after it is
transferred to the target. The USB devs suggested that usb_device_reset()
should be called by the driver in such cases, but in ath9k_htc's case,
the device is disconnected and the error message "device firmware changed"
is displayed.
Agreed, this is a dirty hack - but is there any way for a driver to make the USB
subsystem re-configure the endpoints ?
Sujith
On Tue, 8 Feb 2011, Jussi Kivilinna wrote:
> Atleast with zd1211rw, after usb_reset_device() endpoints still show
> as interrupt type and device requires firmware to be transferred
> again.
Ah, this is one of those bogus devices which has no way of telling the
host when its descriptors change. Have you tried testing this device
across hibernation?
> I cannot tell if zd1211 firmware actually changes endpoint
> types (as ath9k_htc)
Use "lsusb -v".
> but I know that vendor driver uses these
> endpoints with bulk urbs and using those solve/workaround CPU usage
> problem. Increasing interrupt interval from 1 to 255 didn't help.
>
> I think zd1211rw have been trying to use bulk urbs too, but
> usb_bulk_msg() silently switches to interrupt mode. I don't know if
> editing endpoint descriptors is right way solve this, or why CPU usage
> is higher.
>
> CPU usage comes from writing beacon frame to device, this is done
> usually every 100ms on AP-mode. On Atom writing beacon (multiple
> usb_bulk_msg() calls) with interrupt endpoints usually takes ~20msec
> and this is when CPU usage goes up. By changing endpoints to bulk,
> time to write beacon drops to 0~3msec.
That's part of the problem -- the driver shouldn't use usb_bulk_msg().
Create a bunch of URBs and submit them explicitly all at once; don't
wait for one to finish before submitting the next. Setting the
URB_NO_INTERRUPT flag on all but the last could also help. This might
use even less CPU time than you see currently.
> Endpoint descriptors appear to be edited by two drivers,
> drivers/media/IR/mceusb.c (bulk to intr) and
> /drivers/net/wireless/ath/ath9k/hif_usb.c (intr to bulk). Also bulk
> endpoints are changed to interrupt type for buggy low-speed devices in
> drivers/usb/core/config.c.
>
> So what is right way to proceed? Is problem in usb core or with
> device? Deal with buggy device in driver and override endpoint as
> patch does?
There are at least three problems that I can see. First, the device
doesn't disconnect from and reconnect to the USB bus when it starts
using new firmware. Second, the device loses its firmware when it is
reset. Third, the driver wastes too much CPU time writing beacon
frames.
The first two problems are the device's fault, and the third is the
driver's fault. None of them are the fault of usbcore.
Alan Stern
Alan Stern wrote:
> There are at least three problems that I can see. First, the device
> doesn't disconnect from and reconnect to the USB bus when it starts
> using new firmware.
Yes, ath9k_htc has this problem.
> Second, the device loses its firmware when it is
> reset.
Agreed again, this is a problem with ath9k_htc.
> The first two problems are the device's fault, and the third is the
> driver's fault. None of them are the fault of usbcore.
Indeed, which is why this dirty hack is done in the driver.
And it was done to fix a similar issue - high CPU usage when scanning.
But do you have any suggestion for such broken devices ?
The FW changes the type from Interrupt to Bulk, but can the host
driver continue to use Interrupt URBs and use the URB_NO_INTERRUPT trick
that you have mentioned ?
Sujith
On Wed, 9 Feb 2011, Sujith wrote:
> > The first two problems are the device's fault, and the third is the
> > driver's fault. None of them are the fault of usbcore.
>
> Indeed, which is why this dirty hack is done in the driver.
> And it was done to fix a similar issue - high CPU usage when scanning.
>
> But do you have any suggestion for such broken devices ?
> The FW changes the type from Interrupt to Bulk, but can the host
> driver continue to use Interrupt URBs and use the URB_NO_INTERRUPT trick
> that you have mentioned ?
Yes. And it's not a trick; that is exactly what URB_NO_INTERRUPT was
intended for.
Alan Stern
Alan Stern wrote:
> On Wed, 9 Feb 2011, Sujith wrote:
>
> > > The first two problems are the device's fault, and the third is the
> > > driver's fault. None of them are the fault of usbcore.
> >
> > Indeed, which is why this dirty hack is done in the driver.
> > And it was done to fix a similar issue - high CPU usage when scanning.
> >
> > But do you have any suggestion for such broken devices ?
> > The FW changes the type from Interrupt to Bulk, but can the host
> > driver continue to use Interrupt URBs and use the URB_NO_INTERRUPT trick
> > that you have mentioned ?
>
> Yes. And it's not a trick; that is exactly what URB_NO_INTERRUPT was
> intended for.
Ok, thanks. Will fix the driver then.
Sujith
Alan Stern wrote:
> Why do you want to? Why does changing the endpoint type from interrupt
> to bulk improve performance? Yes, the description says that this
> causes CPU usage to go from 10% to below 1%, but _why_ does it do that?
>
> Maybe the same effect can be achieved simply by changing the interrupt
> interval instead.
Well, when the device is plugged in, the USB core recognizes the endpoints
as interrupt type. Later, in the probe() callback, the FW is uploaded to the
target and it patches the endpoints' type to bulk. Please correct me if am
wrong, but I think this mismatch between the host and the target would be
the cause for the high CPU usage, when the transmission load for that
endpoint is high.
Sujith
Quoting Alan Stern <[email protected]>:
> On Sun, 6 Feb 2011, Sujith wrote:
>
>> Alan Stern wrote:
>> > Why do you want to? Why does changing the endpoint type from interrupt
>> > to bulk improve performance? Yes, the description says that this
>> > causes CPU usage to go from 10% to below 1%, but _why_ does it do that?
>> >
>> > Maybe the same effect can be achieved simply by changing the interrupt
>> > interval instead.
>>
>> Well, when the device is plugged in, the USB core recognizes the endpoints
>> as interrupt type. Later, in the probe() callback, the FW is uploaded to the
>> target and it patches the endpoints' type to bulk. Please correct me if am
>> wrong, but I think this mismatch between the host and the target would be
>> the cause for the high CPU usage, when the transmission load for that
>> endpoint is high.
>
> (Actually, bulk transfers generally cause a higher CPU load than
> interrupt transfers. But never mind that...)
>
> The problem is that the kernel doesn't know the firmware has been
> changed. The kernel has to be told about this, one way or another.
> Typically the device would disconnect itself from the USB bus briefly,
> when starting to run the new firmware. Or the kernel would be told to
> reset the device after the firmware was transferred, after which the
> device would start running the new firmware.
>
> Apparently the driver needs to call usb_reset_device() when the
> firmware has been transferred. Following the reset, the kernel will
> see that the descriptors have changed and it will reload all of them.
> Then it will know that the endpoint is bulk rather than interrupt.
>
Atleast with zd1211rw, after usb_reset_device() endpoints still show
as interrupt type and device requires firmware to be transferred
again. I cannot tell if zd1211 firmware actually changes endpoint
types (as ath9k_htc) but I know that vendor driver uses these
endpoints with bulk urbs and using those solve/workaround CPU usage
problem. Increasing interrupt interval from 1 to 255 didn't help.
I think zd1211rw have been trying to use bulk urbs too, but
usb_bulk_msg() silently switches to interrupt mode. I don't know if
editing endpoint descriptors is right way solve this, or why CPU usage
is higher.
CPU usage comes from writing beacon frame to device, this is done
usually every 100ms on AP-mode. On Atom writing beacon (multiple
usb_bulk_msg() calls) with interrupt endpoints usually takes ~20msec
and this is when CPU usage goes up. By changing endpoints to bulk,
time to write beacon drops to 0~3msec.
Endpoint descriptors appear to be edited by two drivers,
drivers/media/IR/mceusb.c (bulk to intr) and
/drivers/net/wireless/ath/ath9k/hif_usb.c (intr to bulk). Also bulk
endpoints are changed to interrupt type for buggy low-speed devices in
drivers/usb/core/config.c.
So what is right way to proceed? Is problem in usb core or with
device? Deal with buggy device in driver and override endpoint as
patch does?
-Jussi
Quoting Martin Schleier <[email protected]>:
> Sujith <m.sujith@...> writes:
>> Jussi Kivilinna wrote:
>> > +static int usb_endpoint_int_to_bulk(struct usb_device *udev,
>> unsigned int pipe)
>> > +{
>> > + struct usb_host_endpoint *ep;
>> > +
>> > + ep = usb_pipe_endpoint(udev, pipe);
>> > + if (!ep)
>> > + return -EPIPE;
>> > +
>> > + switch (ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
>> > + case USB_ENDPOINT_XFER_INT:
>> > + ep->desc.bmAttributes &= ~USB_ENDPOINT_XFERTYPE_MASK;
>> > + ep->desc.bmAttributes |= USB_ENDPOINT_XFER_BULK;
>> > + ep->desc.bInterval = 0;
>> > + /* passthru */
>
> Interesting, I know that interrupt and bulk endpoints share a lot of
> common logic so I can see how this should work. But is it really
> possible [and more to the point: sane?!] to override the endpoint
> descriptors?
>
> I know that usb_submit_urb checks [when config_usb_debug is enabled]
> if the xfertype of the submitted urb matches the one of the endpoint,
> so I wonder I started to wonder if we are fooling "just" the code
> here, or ourselves in the process?
>
Would USB quirk like 'USB_QUIRK_NEED_BULK_ON_INTR_EP' that avoids USB
debug check make more sense?
-Jussi