Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Martin Schleier]

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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Jussi Kivilinna]

Quoting Martin Schleier <drahemmaps@gmx.net>:

> 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

Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Sujith]

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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Alan Stern]

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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Jussi Kivilinna]

Quoting Alan Stern <stern@rowland.harvard.edu>:

> 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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Alan Stern]

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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Jussi Kivilinna]

Quoting Alan Stern <stern@rowland.harvard.edu>:

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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Sujith]

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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Alan Stern]

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

Re: Override endpoint attributes (was: RE: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk)

[Sujith]

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