zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
PATCH 1/1 zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
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.
--- drivers/net/wireless/zd1211rw/zd_usb.c | 62 +++++++++++++++++++++++--------- drivers/net/wireless/zd1211rw/zd_usb.h | 1 - 2 files changed, 44 insertions(+), 19 deletions(-)
drivers/net/wireless/zd1211rw/zd_usb.c +44 -18
@@ -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),
drivers/net/wireless/zd1211rw/zd_usb.h +0 -1
@@ -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; };
PATCH 1/1 zd1211rw: change endpoint types of EP_REGS_OUT and EP_INT_IN from interrupt to bulk
Quoted 4 lines · Jussi Kivilinna
drivers/net/wireless/zd1211rw/zd_usb.c +4
-0+static int usb_endpoint_int_to_bulk (struct usb_device *udev,unsigned int pipe) +{ +struct usb_host_endpoint *ep; +
drivers/net/wireless/zd1211rw/zd_usb.c static int eject_installer(struct usb_interface *intf) · line 1264
Quoted 10 lines
drivers/net/wireless/zd1211rw/zd_usb.c +10
-0+ 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
Quoting Sujith <[email protected]>:
Quoted 22 lines
Quoted 5 lines · Jussi Kivilinna
+static int
usb_endpoint_int_to_bulk(struct usb_device *udev, unsigned int pipe)drivers/net/wireless/zd1211rw/zd_usb.c +3
-0+{ +struct usb_host_endpoint *ep; +drivers/net/wireless/zd1211rw/zd_usb.c static int eject_installer(struct usb_interface *intf) · line 1264
Quoted 10 lines
drivers/net/wireless/zd1211rw/zd_usb.c +10
-0+ 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_htcdoes the same thing (seeath/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).
Quoted 3 lines
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.
Quoted 8 lines · Jussi Kivilinna
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 <[email protected]>
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
Signature · 2 lines
John W. Linville Someday the world will need a hero, and you [email protected] might be all we have. Be ready.
Quoting "John W. Linville" <[email protected]>:
Quoted 10 lines · Jussi Kivilinna
Quoted 8 lines
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 <[email protected]>
Quoted 8 lines
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