diff mbox series

[1/3] mt76: usb: use max packet length for m76u_copy

Message ID 1575030748-2218-2-git-send-email-sgruszka@redhat.com (mailing list archive)
State Accepted
Delegated to: Felix Fietkau
Headers show
Series mt76: some further beaconing optimizations and cleanups | expand

Commit Message

Stanislaw Gruszka Nov. 29, 2019, 12:32 p.m. UTC 
For transferring data over USB the optimal size is endpoint maxpacket.
For my hardware maxpaket for control endpoint is 64 bytes and changing
to this value from 128 bytes further shorten TBTT work time from
3ms to 1ms.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76.h |  7 +++----
 drivers/net/wireless/mediatek/mt76/usb.c  | 29 +++++++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

Comments

Lorenzo Bianconi Nov. 29, 2019, 12:47 p.m. UTC | #1 
> For transferring data over USB the optimal size is endpoint maxpacket.
> For my hardware maxpaket for control endpoint is 64 bytes and changing
> to this value from 128 bytes further shorten TBTT work time from
> 3ms to 1ms.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/mediatek/mt76/mt76.h |  7 +++----
>  drivers/net/wireless/mediatek/mt76/usb.c  | 29 +++++++++++++++++++----------
>  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 1981912de1f9..c268c3d76b3d 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -381,10 +381,9 @@ enum mt76u_out_ep {
>  #define MCU_RESP_URB_SIZE	1024
>  struct mt76_usb {
>  	struct mutex usb_ctrl_mtx;
> -	union {
> -		u8 data[128];
> -		__le32 reg_val;
> -	};
> +	__le32 reg_val;
> +	u8 *data;
> +	u16 data_len;
>  
>  	struct tasklet_struct rx_tasklet;
>  	struct workqueue_struct *stat_wq;
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index 97b263ce3872..a9ff2bd62fc9 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -163,7 +163,7 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
>  
>  	mutex_lock(&usb->usb_ctrl_mtx);
>  	while (i < len) {
> -		current_batch_size = min_t(int, sizeof(usb->data), len - i);
> +		current_batch_size = min_t(int, usb->data_len, len - i);
>  		memcpy(usb->data, val + i, current_batch_size);
>  		ret = __mt76u_vendor_request(dev, MT_VEND_MULTI_WRITE,
>  					     USB_DIR_OUT | USB_TYPE_VENDOR,
> @@ -950,6 +950,15 @@ int mt76u_alloc_queues(struct mt76_dev *dev)
>  	.kick = mt76u_tx_kick,
>  };
>  
> +void mt76u_deinit(struct mt76_dev *dev)
> +{
> +	if (dev->usb.stat_wq) {
> +		destroy_workqueue(dev->usb.stat_wq);
> +		dev->usb.stat_wq = NULL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(mt76u_deinit);
> +
>  int mt76u_init(struct mt76_dev *dev,
>  	       struct usb_interface *intf)
>  {
> @@ -974,6 +983,15 @@ int mt76u_init(struct mt76_dev *dev,
>  	if (!usb->stat_wq)
>  		return -ENOMEM;
>  
> +	usb->data_len = usb_maxpacket(udev, usb_sndctrlpipe(udev, 0), 1);
> +	if (usb->data_len < 32)
> +		usb->data_len = 32;

Hi Stanislaw,

	usb->data_len = max_t(u16, 32,
			      usb_maxpacket(udev, usb_sndctrlpipe(udev, 0), 1));

Moreover are you sure using ctrl endpoint 0 is fine for all devices?

Regards,
Lorenzo

> +	usb->data = devm_kmalloc(dev->dev, usb->data_len, GFP_KERNEL);
> +	if (!usb->data) {
> +		mt76u_deinit(dev);
> +		return -ENOMEM;
> +	}
> +
>  	mutex_init(&usb->mcu.mutex);
>  
>  	mutex_init(&usb->usb_ctrl_mtx);
> @@ -988,14 +1006,5 @@ int mt76u_init(struct mt76_dev *dev,
>  }
>  EXPORT_SYMBOL_GPL(mt76u_init);
>  
> -void mt76u_deinit(struct mt76_dev *dev)
> -{
> -	if (dev->usb.stat_wq) {
> -		destroy_workqueue(dev->usb.stat_wq);
> -		dev->usb.stat_wq = NULL;
> -	}
> -}
> -EXPORT_SYMBOL_GPL(mt76u_deinit);
> -
>  MODULE_AUTHOR("Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>");
>  MODULE_LICENSE("Dual BSD/GPL");
> -- 
> 1.9.3
>
Stanislaw Gruszka Nov. 29, 2019, 1:06 p.m. UTC | #2 
On Fri, Nov 29, 2019 at 02:47:44PM +0200, Lorenzo Bianconi wrote:
> > +	usb->data_len = usb_maxpacket(udev, usb_sndctrlpipe(udev, 0), 1);
> > +	if (usb->data_len < 32)
> > +		usb->data_len = 32;
> 
> Hi Stanislaw,
> 
> 	usb->data_len = max_t(u16, 32,
> 			      usb_maxpacket(udev, usb_sndctrlpipe(udev, 0), 1));
> 

Why this is better ?

> Moreover are you sure using ctrl endpoint 0 is fine for all devices?

usb_sndctrlpipe(udev, 0) is used in __mt76u_vendor_request() for all 
out requests i.e. on all cases were usb->data is used.

Stanislaw
Lorenzo Bianconi Nov. 29, 2019, 1:11 p.m. UTC | #3 
> On Fri, Nov 29, 2019 at 02:47:44PM +0200, Lorenzo Bianconi wrote:
> > > +	usb->data_len = usb_maxpacket(udev, usb_sndctrlpipe(udev, 0), 1);
> > > +	if (usb->data_len < 32)
> > > +		usb->data_len = 32;
> > 
> > Hi Stanislaw,
> > 
> > 	usb->data_len = max_t(u16, 32,
> > 			      usb_maxpacket(udev, usb_sndctrlpipe(udev, 0), 1));
> > 
> 
> Why this is better ?

More readable for my point of view

> 
> > Moreover are you sure using ctrl endpoint 0 is fine for all devices?
> 
> usb_sndctrlpipe(udev, 0) is used in __mt76u_vendor_request() for all 
> out requests i.e. on all cases were usb->data is used.

ops, right :) Thx for for pointing this out.

Regards,
Lorenzo

> 
> Stanislaw 
>
diff mbox series

Patch

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 1981912de1f9..c268c3d76b3d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -381,10 +381,9 @@  enum mt76u_out_ep {
 #define MCU_RESP_URB_SIZE	1024
 struct mt76_usb {
 	struct mutex usb_ctrl_mtx;
-	union {
-		u8 data[128];
-		__le32 reg_val;
-	};
+	__le32 reg_val;
+	u8 *data;
+	u16 data_len;
 
 	struct tasklet_struct rx_tasklet;
 	struct workqueue_struct *stat_wq;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 97b263ce3872..a9ff2bd62fc9 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -163,7 +163,7 @@  static void mt76u_copy(struct mt76_dev *dev, u32 offset,
 
 	mutex_lock(&usb->usb_ctrl_mtx);
 	while (i < len) {
-		current_batch_size = min_t(int, sizeof(usb->data), len - i);
+		current_batch_size = min_t(int, usb->data_len, len - i);
 		memcpy(usb->data, val + i, current_batch_size);
 		ret = __mt76u_vendor_request(dev, MT_VEND_MULTI_WRITE,
 					     USB_DIR_OUT | USB_TYPE_VENDOR,
@@ -950,6 +950,15 @@  int mt76u_alloc_queues(struct mt76_dev *dev)
 	.kick = mt76u_tx_kick,
 };
 
+void mt76u_deinit(struct mt76_dev *dev)
+{
+	if (dev->usb.stat_wq) {
+		destroy_workqueue(dev->usb.stat_wq);
+		dev->usb.stat_wq = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(mt76u_deinit);
+
 int mt76u_init(struct mt76_dev *dev,
 	       struct usb_interface *intf)
 {
@@ -974,6 +983,15 @@  int mt76u_init(struct mt76_dev *dev,
 	if (!usb->stat_wq)
 		return -ENOMEM;
 
+	usb->data_len = usb_maxpacket(udev, usb_sndctrlpipe(udev, 0), 1);
+	if (usb->data_len < 32)
+		usb->data_len = 32;
+	usb->data = devm_kmalloc(dev->dev, usb->data_len, GFP_KERNEL);
+	if (!usb->data) {
+		mt76u_deinit(dev);
+		return -ENOMEM;
+	}
+
 	mutex_init(&usb->mcu.mutex);
 
 	mutex_init(&usb->usb_ctrl_mtx);
@@ -988,14 +1006,5 @@  int mt76u_init(struct mt76_dev *dev,
 }
 EXPORT_SYMBOL_GPL(mt76u_init);
 
-void mt76u_deinit(struct mt76_dev *dev)
-{
-	if (dev->usb.stat_wq) {
-		destroy_workqueue(dev->usb.stat_wq);
-		dev->usb.stat_wq = NULL;
-	}
-}
-EXPORT_SYMBOL_GPL(mt76u_deinit);
-
 MODULE_AUTHOR("Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>");
 MODULE_LICENSE("Dual BSD/GPL");