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

From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: linux-wireless@vger.kernel.org, Felix Fietkau <nbd@nbd.name>,
	Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>,
	Ryder Lee <ryder.lee@mediatek.com>, Roy Luo <royluo@google.com>,
	Markus Theil <markus.theil@tu-ilmenau.de>
Subject: Re: [PATCH 1/3] mt76: usb: use max packet length for m76u_copy
Date: Fri, 29 Nov 2019 14:47:44 +0200	[thread overview]
Message-ID: <20191129124744.GE32696@localhost.localdomain> (raw)
In-Reply-To: <1575030748-2218-2-git-send-email-sgruszka@redhat.com>

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