* [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
@ 2011-02-06 8:35 Jussi Kivilinna
2011-02-06 9:16 ` Sujith
2011-02-15 18:49 ` John W. Linville
0 siblings, 2 replies; 5+ messages in thread
From: Jussi Kivilinna @ 2011-02-06 8:35 UTC (permalink / raw)
To: linux-wireless; +Cc: Daniel Drake, John W. Linville, Ulrich Kunitz
This fixes high CPU usage when writing beacon frame to hardware. With this
patch CPU usage of kworker on Intel Atom with 100 TU beacon interval goes
from ~10% down to <1%.
Checking vendor driver appears it's also ignoring fact that endpoints are
interrupt type and is using bulk urbs on these.
Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
---
drivers/net/wireless/zd1211rw/zd_usb.c | 62 +++++++++++++++++++++++---------
drivers/net/wireless/zd1211rw/zd_usb.h | 1 -
2 files changed, 44 insertions(+), 19 deletions(-)
diff --git a/drivers/net/wireless/zd1211rw/zd_usb.c b/drivers/net/wireless/zd1211rw/zd_usb.c
index f6df366..65aff96 100644
--- a/drivers/net/wireless/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/zd1211rw/zd_usb.c
@@ -452,19 +452,6 @@ resubmit:
return;
}
-static inline int int_urb_interval(struct usb_device *udev)
-{
- switch (udev->speed) {
- case USB_SPEED_HIGH:
- return 4;
- case USB_SPEED_LOW:
- return 10;
- case USB_SPEED_FULL:
- default:
- return 1;
- }
-}
-
static inline int usb_int_enabled(struct zd_usb *usb)
{
unsigned long flags;
@@ -511,10 +498,9 @@ int zd_usb_enable_int(struct zd_usb *usb)
goto error_set_urb_null;
}
- usb_fill_int_urb(urb, udev, usb_rcvintpipe(udev, EP_INT_IN),
+ usb_fill_bulk_urb(urb, udev, usb_rcvbulkpipe(udev, EP_INT_IN),
intr->buffer, USB_MAX_EP_INT_BUFFER,
- int_urb_complete, usb,
- intr->interval);
+ int_urb_complete, usb);
urb->transfer_dma = intr->buffer_dma;
urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
@@ -1118,7 +1104,6 @@ static inline void init_usb_interrupt(struct zd_usb *usb)
struct zd_usb_interrupt *intr = &usb->intr;
spin_lock_init(&intr->lock);
- intr->interval = int_urb_interval(zd_usb_to_usbdev(usb));
init_completion(&intr->read_regs.completion);
intr->read_regs.cr_int_addr = cpu_to_le16((u16)CR_INTERRUPT);
}
@@ -1263,10 +1248,32 @@ static int eject_installer(struct usb_interface *intf)
return 0;
}
+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 */
+ case USB_ENDPOINT_XFER_BULK:
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
int zd_usb_init_hw(struct zd_usb *usb)
{
int r;
struct zd_mac *mac = zd_usb_to_mac(usb);
+ struct usb_device *udev = zd_usb_to_usbdev(usb);
dev_dbg_f(zd_usb_dev(usb), "\n");
@@ -1277,13 +1284,32 @@ int zd_usb_init_hw(struct zd_usb *usb)
return r;
}
- r = usb_reset_configuration(zd_usb_to_usbdev(usb));
+ r = usb_reset_configuration(udev);
if (r) {
dev_dbg_f(zd_usb_dev(usb),
"couldn't reset configuration. Error number %d\n", r);
return r;
}
+ /* Change EP_REGS_OUT and EP_INT_IN to bulk endpoints. This solves high
+ * CPU usage of zd_mac_beacon_config.
+ */
+ r = usb_endpoint_int_to_bulk(udev, usb_sndintpipe(udev, EP_REGS_OUT));
+ if (r) {
+ dev_dbg_f(zd_usb_dev(usb),
+ "couldn't change EP_REGS_OUT endpoint type to bulk. "
+ "Error number %d\n", r);
+ return r;
+ }
+
+ r = usb_endpoint_int_to_bulk(udev, usb_rcvintpipe(udev, EP_INT_IN));
+ if (r) {
+ dev_dbg_f(zd_usb_dev(usb),
+ "couldn't change EP_INT_IN endpoint type to bulk. "
+ "Error number %d\n", r);
+ return r;
+ }
+
r = zd_mac_init_hw(mac->hw);
if (r) {
dev_dbg_f(zd_usb_dev(usb),
diff --git a/drivers/net/wireless/zd1211rw/zd_usb.h b/drivers/net/wireless/zd1211rw/zd_usb.h
index 2d688f4..333bb84 100644
--- a/drivers/net/wireless/zd1211rw/zd_usb.h
+++ b/drivers/net/wireless/zd1211rw/zd_usb.h
@@ -168,7 +168,6 @@ struct zd_usb_interrupt {
struct urb *urb;
void *buffer;
dma_addr_t buffer_dma;
- int interval;
u8 read_regs_enabled:1;
};
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
2011-02-06 8:35 [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk Jussi Kivilinna
@ 2011-02-06 9:16 ` Sujith
2011-02-06 11:35 ` Jussi Kivilinna
2011-02-15 18:49 ` John W. Linville
1 sibling, 1 reply; 5+ messages in thread
From: Sujith @ 2011-02-06 9:16 UTC (permalink / raw)
To: Jussi Kivilinna
Cc: linux-wireless, Daniel Drake, John W. Linville, Ulrich Kunitz
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 */
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 ?
Sujith
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
2011-02-06 9:16 ` Sujith
@ 2011-02-06 11:35 ` Jussi Kivilinna
0 siblings, 0 replies; 5+ messages in thread
From: Jussi Kivilinna @ 2011-02-06 11:35 UTC (permalink / raw)
To: Sujith; +Cc: linux-wireless, Daniel Drake, John W. Linville, Ulrich Kunitz
Quoting Sujith <m.sujith@gmail.com>:
> 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 */
>
> 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 ?
Choosed to place intr->bulk change after reset as that seemed less
likely to wrong. Tested in the other order and that works too.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
2011-02-06 8:35 [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk Jussi Kivilinna
2011-02-06 9:16 ` Sujith
@ 2011-02-15 18:49 ` John W. Linville
2011-02-16 1:08 ` Jussi Kivilinna
1 sibling, 1 reply; 5+ messages in thread
From: John W. Linville @ 2011-02-15 18:49 UTC (permalink / raw)
To: Jussi Kivilinna; +Cc: linux-wireless, Daniel Drake, Ulrich Kunitz
On Sun, Feb 06, 2011 at 10:35:22AM +0200, Jussi Kivilinna wrote:
> This fixes high CPU usage when writing beacon frame to hardware. With this
> patch CPU usage of kworker on Intel Atom with 100 TU beacon interval goes
> from ~10% down to <1%.
>
> Checking vendor driver appears it's also ignoring fact that endpoints are
> interrupt type and is using bulk urbs on these.
>
> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
This patch led to a lengthy thread. I'm not 100% clear on the
outcome, but it seems like the conclusion was that the driver should
be doing something else -- at least that seemed to be the conclusion
for ath9k_htc.
So, is this patch still wanted?
John
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
2011-02-15 18:49 ` John W. Linville
@ 2011-02-16 1:08 ` Jussi Kivilinna
0 siblings, 0 replies; 5+ messages in thread
From: Jussi Kivilinna @ 2011-02-16 1:08 UTC (permalink / raw)
To: John W. Linville; +Cc: linux-wireless, Daniel Drake, Ulrich Kunitz
Quoting "John W. Linville" <linville@tuxdriver.com>:
> On Sun, Feb 06, 2011 at 10:35:22AM +0200, Jussi Kivilinna wrote:
>> This fixes high CPU usage when writing beacon frame to hardware. With this
>> patch CPU usage of kworker on Intel Atom with 100 TU beacon interval goes
>> from ~10% down to <1%.
>>
>> Checking vendor driver appears it's also ignoring fact that endpoints are
>> interrupt type and is using bulk urbs on these.
>>
>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>
> This patch led to a lengthy thread. I'm not 100% clear on the
> outcome, but it seems like the conclusion was that the driver should
> be doing something else -- at least that seemed to be the conclusion
> for ath9k_htc.
>
> So, is this patch still wanted?
>
> John
It's not needed. The patchset [1] that made write command use
asynchronous URBs lowers CPU usage the other way.
-Jussi
[1] http://marc.info/?l=linux-wireless&m=129753620103065&w=2
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-02-16 1:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-06 8:35 [PATCH] zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk Jussi Kivilinna
2011-02-06 9:16 ` Sujith
2011-02-06 11:35 ` Jussi Kivilinna
2011-02-15 18:49 ` John W. Linville
2011-02-16 1:08 ` Jussi Kivilinna
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).