[PATCH 0/3] mt76: some further beaconing optimizations and cleanups

[PATCH 0/3] mt76: some further beaconing optimizations and cleanups

[Stanislaw Gruszka]

This is based on top:
[PATCH v9 0/6] mt76: channel switch support for USB devices
https://lore.kernel.org/linux-wireless/20191126214704.27297-1-markus.theil@tu-ilmenau.de/T/#t

Stanislaw Gruszka (3):
  mt76: usb: use max packet length for m76u_copy
  mt76: mt76x02u: do not set NULL beacons
  mt76: mt76x02: minor mt76x02_mac_set_beacon optimization

 drivers/net/wireless/mediatek/mt76/mt76.h          |  7 +++---
 .../net/wireless/mediatek/mt76/mt76x02_beacon.c    | 14 ++++-------
 drivers/net/wireless/mediatek/mt76/mt76x02_mac.h   |  2 +-
 .../net/wireless/mediatek/mt76/mt76x02_usb_core.c  |  6 ++---
 drivers/net/wireless/mediatek/mt76/usb.c           | 27 ++++++++++++++--------
 5 files changed, 27 insertions(+), 29 deletions(-)

-- 
1.9.3

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

[Stanislaw Gruszka]

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

[PATCH 2/3] mt76: mt76x02u: do not set NULL beacons

[Stanislaw Gruszka]

With current implementation we do not cleanup beacon memory, so is not
needed to call mt76x02_mac_set_beacon() with NULL skb.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
index 3dc3682d585a..c06d8b5f7995 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
@@ -198,7 +198,7 @@ static void mt76x02u_pre_tbtt_work(struct work_struct *work)
 		container_of(work, struct mt76x02_dev, pre_tbtt_work);
 	struct beacon_bc_data data = {};
 	struct sk_buff *skb;
-	int i, nbeacons;
+	int nbeacons;
 
 	if (!dev->mt76.beacon_mask)
 		return;
@@ -224,10 +224,8 @@ static void mt76x02u_pre_tbtt_work(struct work_struct *work)
 	nbeacons = hweight8(dev->mt76.beacon_mask);
 	mt76x02_enqueue_buffered_bc(dev, &data, N_BCN_SLOTS - nbeacons);
 
-	for (i = nbeacons; i < N_BCN_SLOTS; i++) {
-		skb = __skb_dequeue(&data.q);
+	while ((skb = __skb_dequeue(&data.q)) != NULL)
 		mt76x02_mac_set_beacon(dev, skb);
-	}
 
 out:
 	mt76x02_mac_set_beacon_finish(dev);
-- 
1.9.3

[PATCH 3/3] mt76: mt76x02: minor mt76x02_mac_set_beacon optimization

[Stanislaw Gruszka]

We do not call mt76x02_mac_set_beacon() with NULL skb any longer and
we do not need to return error value.

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

diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
index 68a4f512319e..3cc5226b05c1 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
@@ -67,21 +67,15 @@ void mt76x02_mac_set_beacon_finish(struct mt76x02_dev *dev)
 }
 EXPORT_SYMBOL_GPL(mt76x02_mac_set_beacon_finish);
 
-int mt76x02_mac_set_beacon(struct mt76x02_dev *dev,
-			   struct sk_buff *skb)
+void mt76x02_mac_set_beacon(struct mt76x02_dev *dev,
+			    struct sk_buff *skb)
 {
 	int bcn_len = dev->beacon_ops->slot_size;
 	int bcn_addr = MT_BEACON_BASE + (bcn_len * dev->beacon_data_count);
-	int ret = 0;
-
-	if (skb) {
-		ret = mt76x02_write_beacon(dev, bcn_addr, skb);
-		if (!ret)
-			dev->beacon_data_count++;
-	}
 
+	if (!mt76x02_write_beacon(dev, bcn_addr, skb))
+		dev->beacon_data_count++;
 	dev_kfree_skb(skb);
-	return ret;
 }
 EXPORT_SYMBOL_GPL(mt76x02_mac_set_beacon);
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.h b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.h
index f67f66f65ee0..5d6411ee44ce 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.h
@@ -201,7 +201,7 @@ void mt76x02_tx_complete_skb(struct mt76_dev *mdev, enum mt76_txq_id qid,
 
 void mt76x02_mac_cc_reset(struct mt76x02_dev *dev);
 void mt76x02_mac_set_bssid(struct mt76x02_dev *dev, u8 idx, const u8 *addr);
-int mt76x02_mac_set_beacon(struct mt76x02_dev *dev, struct sk_buff *skb);
+void mt76x02_mac_set_beacon(struct mt76x02_dev *dev, struct sk_buff *skb);
 void mt76x02_mac_set_beacon_enable(struct mt76x02_dev *dev,
 				   struct ieee80211_vif *vif, bool enable);
 void mt76x02_mac_set_beacon_prepare(struct mt76x02_dev *dev);
-- 
1.9.3

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Stanislaw Gruszka]

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 

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]