[PATCH v3 1/3] mt76usb: allow mt76u_bulk_msg be used for reads

[PATCH v3 1/3] mt76usb: allow mt76u_bulk_msg be used for reads

[Stanislaw Gruszka]

Extend mt76u_bulk_msg() such it can be used for synchronous bulk reads.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
v2: pass NULL actual_len to usb_bulk_msg

 drivers/net/wireless/mediatek/mt76/mt76.h            | 12 ++++++++----
 drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c |  4 ++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index f55dc621e060..6092646014c4 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -729,16 +729,20 @@ static inline u8 q2ep(u8 qid)
 }
 
 static inline int
-mt76u_bulk_msg(struct mt76_dev *dev, void *data, int len, int timeout)
+mt76u_bulk_msg(struct mt76_dev *dev, void *data, int len, int *actual_len,
+	       int timeout)
 {
 	struct usb_interface *intf = to_usb_interface(dev->dev);
 	struct usb_device *udev = interface_to_usbdev(intf);
 	struct mt76_usb *usb = &dev->usb;
 	unsigned int pipe;
-	int sent;
 
-	pipe = usb_sndbulkpipe(udev, usb->out_ep[MT_EP_OUT_INBAND_CMD]);
-	return usb_bulk_msg(udev, pipe, data, len, &sent, timeout);
+	if (actual_len)
+		pipe = usb_rcvbulkpipe(udev, usb->in_ep[MT_EP_IN_CMD_RESP]);
+	else
+		pipe = usb_sndbulkpipe(udev, usb->out_ep[MT_EP_OUT_INBAND_CMD]);
+
+	return usb_bulk_msg(udev, pipe, data, len, actual_len, timeout);
 }
 
 int mt76u_vendor_request(struct mt76_dev *dev, u8 req,
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
index e469e383cb88..f497c8e4332a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -126,7 +126,7 @@ __mt76x02u_mcu_send_msg(struct mt76_dev *dev, struct sk_buff *skb,
 	if (ret)
 		return ret;
 
-	ret = mt76u_bulk_msg(dev, skb->data, skb->len, 500);
+	ret = mt76u_bulk_msg(dev, skb->data, skb->len, NULL, 500);
 	if (ret)
 		return ret;
 
@@ -271,7 +271,7 @@ __mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, u8 *data,
 
 	data_len = MT_CMD_HDR_LEN + len + sizeof(info);
 
-	err = mt76u_bulk_msg(&dev->mt76, data, data_len, 1000);
+	err = mt76u_bulk_msg(&dev->mt76, data, data_len, NULL, 1000);
 	if (err) {
 		dev_err(dev->mt76.dev, "firmware upload failed: %d\n", err);
 		return err;
-- 
2.7.5

[PATCH v3 2/3] mt76usb: use synchronous msg for mcu command responses

[Stanislaw Gruszka]

Use usb_bulk_msg for reading MCU command responses. This simplify code
a lot.

Together with 97a3005759c ("mt76usb: allow mt76u_bulk_msg be used
for reads") it also fix possible problems with rx data buffers
not being aligned and contained within single page. After doing
page_frag_alloc(1024) consecutive page_frag_alloc(PAGE_SIZE) will
alloc PAGE_SIZE buffer at PAGE_SIZE - 1024 offset.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76.h          |  3 +-
 drivers/net/wireless/mediatek/mt76/mt76x0/usb.c    | 11 --------
 .../net/wireless/mediatek/mt76/mt76x02_usb_mcu.c   | 32 +++++++---------------
 drivers/net/wireless/mediatek/mt76/mt76x2/usb.c    | 11 --------
 drivers/net/wireless/mediatek/mt76/usb.c           |  1 -
 drivers/net/wireless/mediatek/mt76/usb_mcu.c       | 31 +++------------------
 6 files changed, 15 insertions(+), 74 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 6092646014c4..c9b5eb9b0582 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -384,8 +384,7 @@ struct mt76_usb {
 
 	struct mt76u_mcu {
 		struct mutex mutex;
-		struct completion cmpl;
-		struct mt76u_buf res;
+		u8 *data;
 		u32 msg_seq;
 
 		/* multiple reads */
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index da9d05f6074d..f0c33890f1a5 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -311,13 +311,11 @@ static int __maybe_unused mt76x0_suspend(struct usb_interface *usb_intf,
 					 pm_message_t state)
 {
 	struct mt76x02_dev *dev = usb_get_intfdata(usb_intf);
-	struct mt76_usb *usb = &dev->mt76.usb;
 
 	mt76u_stop_queues(&dev->mt76);
 	mt76x0u_mac_stop(dev);
 	clear_bit(MT76_STATE_MCU_RUNNING, &dev->mt76.state);
 	mt76x0_chip_onoff(dev, false, false);
-	usb_kill_urb(usb->mcu.res.urb);
 
 	return 0;
 }
@@ -328,15 +326,6 @@ static int __maybe_unused mt76x0_resume(struct usb_interface *usb_intf)
 	struct mt76_usb *usb = &dev->mt76.usb;
 	int ret;
 
-	reinit_completion(&usb->mcu.cmpl);
-	ret = mt76u_submit_buf(&dev->mt76, USB_DIR_IN,
-			       MT_EP_IN_CMD_RESP,
-			       &usb->mcu.res, GFP_KERNEL,
-			       mt76u_mcu_complete_urb,
-			       &usb->mcu.cmpl);
-	if (ret < 0)
-		goto err;
-
 	ret = mt76u_submit_rx_buffers(&dev->mt76);
 	if (ret < 0)
 		goto err;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
index f497c8e4332a..0cb8751321a1 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -61,33 +61,21 @@ mt76x02u_multiple_mcu_reads(struct mt76_dev *dev, u8 *data, int len)
 static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
 {
 	struct mt76_usb *usb = &dev->usb;
-	struct mt76u_buf *buf = &usb->mcu.res;
-	struct urb *urb = buf->urb;
-	u8 *data = buf->buf;
-	int i, ret;
+	u8 *data = usb->mcu.data;
+	int i, len, ret;
 	u32 rxfce;
 
 	for (i = 0; i < 5; i++) {
-		if (!wait_for_completion_timeout(&usb->mcu.cmpl,
-						 msecs_to_jiffies(300)))
+		ret = mt76u_bulk_msg(dev, data, MCU_RESP_URB_SIZE, &len, 300);
+		if (ret == -ETIMEDOUT)
 			continue;
-
-		if (urb->status)
-			return -EIO;
+		if (ret)
+			goto out;
 
 		if (usb->mcu.rp)
-			mt76x02u_multiple_mcu_reads(dev, data + 4,
-						    urb->actual_length - 8);
+			mt76x02u_multiple_mcu_reads(dev, data + 4, len - 8);
 
 		rxfce = get_unaligned_le32(data);
-		ret = mt76u_submit_buf(dev, USB_DIR_IN,
-				       MT_EP_IN_CMD_RESP,
-				       buf, GFP_KERNEL,
-				       mt76u_mcu_complete_urb,
-				       &usb->mcu.cmpl);
-		if (ret)
-			return ret;
-
 		if (seq == FIELD_GET(MT_RX_FCE_INFO_CMD_SEQ, rxfce) &&
 		    FIELD_GET(MT_RX_FCE_INFO_EVT_TYPE, rxfce) == EVT_CMD_DONE)
 			return 0;
@@ -96,9 +84,9 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
 			FIELD_GET(MT_RX_FCE_INFO_EVT_TYPE, rxfce),
 			seq, FIELD_GET(MT_RX_FCE_INFO_CMD_SEQ, rxfce));
 	}
-
-	dev_err(dev->dev, "error: %s timed out\n", __func__);
-	return -ETIMEDOUT;
+out:
+	dev_err(dev->dev, "error: %s failed with %d\n", __func__, ret);
+	return ret;
 }
 
 static int
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
index f81a85e96922..ddb6b2c48e01 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
@@ -100,11 +100,9 @@ static int __maybe_unused mt76x2u_suspend(struct usb_interface *intf,
 					  pm_message_t state)
 {
 	struct mt76x02_dev *dev = usb_get_intfdata(intf);
-	struct mt76_usb *usb = &dev->mt76.usb;
 
 	mt76u_stop_queues(&dev->mt76);
 	mt76x2u_stop_hw(dev);
-	usb_kill_urb(usb->mcu.res.urb);
 
 	return 0;
 }
@@ -115,15 +113,6 @@ static int __maybe_unused mt76x2u_resume(struct usb_interface *intf)
 	struct mt76_usb *usb = &dev->mt76.usb;
 	int err;
 
-	reinit_completion(&usb->mcu.cmpl);
-	err = mt76u_submit_buf(&dev->mt76, USB_DIR_IN,
-			       MT_EP_IN_CMD_RESP,
-			       &usb->mcu.res, GFP_KERNEL,
-			       mt76u_mcu_complete_urb,
-			       &usb->mcu.cmpl);
-	if (err < 0)
-		goto err;
-
 	err = mt76u_submit_rx_buffers(&dev->mt76);
 	if (err < 0)
 		goto err;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 78191968b4fa..5c3b7f735aae 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -933,7 +933,6 @@ int mt76u_init(struct mt76_dev *dev,
 	INIT_DELAYED_WORK(&usb->stat_work, mt76u_tx_status_data);
 	skb_queue_head_init(&dev->rx_skb[MT_RXQ_MAIN]);
 
-	init_completion(&usb->mcu.cmpl);
 	mutex_init(&usb->mcu.mutex);
 
 	mutex_init(&usb->usb_ctrl_mtx);
diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
index 72c8607da4b4..747231edc57d 100644
--- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
@@ -16,42 +16,19 @@
 
 #include "mt76.h"
 
-void mt76u_mcu_complete_urb(struct urb *urb)
-{
-	struct completion *cmpl = urb->context;
-
-	complete(cmpl);
-}
-EXPORT_SYMBOL_GPL(mt76u_mcu_complete_urb);
-
 int mt76u_mcu_init_rx(struct mt76_dev *dev)
 {
 	struct mt76_usb *usb = &dev->usb;
-	int err;
 
-	err = mt76u_buf_alloc(dev, &usb->mcu.res, MCU_RESP_URB_SIZE,
-			      MCU_RESP_URB_SIZE, GFP_KERNEL);
-	if (err < 0)
-		return err;
-
-	err = mt76u_submit_buf(dev, USB_DIR_IN, MT_EP_IN_CMD_RESP,
-			       &usb->mcu.res, GFP_KERNEL,
-			       mt76u_mcu_complete_urb,
-			       &usb->mcu.cmpl);
-	if (err < 0)
-		mt76u_buf_free(&usb->mcu.res);
-
-	return err;
+	usb->mcu.data = kmalloc(MCU_RESP_URB_SIZE, GFP_KERNEL);
+	return usb->mcu.data ? 0 : -ENOMEM;
 }
 EXPORT_SYMBOL_GPL(mt76u_mcu_init_rx);
 
 void mt76u_mcu_deinit(struct mt76_dev *dev)
 {
-	struct mt76u_buf *buf = &dev->usb.mcu.res;
+	struct mt76_usb *usb = &dev->usb;
 
-	if (buf->urb) {
-		usb_kill_urb(buf->urb);
-		mt76u_buf_free(buf);
-	}
+	kfree(usb->mcu.data);
 }
 EXPORT_SYMBOL_GPL(mt76u_mcu_deinit);
-- 
2.7.5

Re: [PATCH v3 2/3] mt76usb: use synchronous msg for mcu command responses

[Lorenzo Bianconi]

> Use usb_bulk_msg for reading MCU command responses. This simplify code
> a lot.
> 
> Together with 97a3005759c ("mt76usb: allow mt76u_bulk_msg be used
> for reads") it also fix possible problems with rx data buffers
> not being aligned and contained within single page. After doing
> page_frag_alloc(1024) consecutive page_frag_alloc(PAGE_SIZE) will
> alloc PAGE_SIZE buffer at PAGE_SIZE - 1024 offset.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/mediatek/mt76/mt76.h          |  3 +-
>  drivers/net/wireless/mediatek/mt76/mt76x0/usb.c    | 11 --------
>  .../net/wireless/mediatek/mt76/mt76x02_usb_mcu.c   | 32 +++++++---------------
>  drivers/net/wireless/mediatek/mt76/mt76x2/usb.c    | 11 --------
>  drivers/net/wireless/mediatek/mt76/usb.c           |  1 -
>  drivers/net/wireless/mediatek/mt76/usb_mcu.c       | 31 +++------------------
>  6 files changed, 15 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 6092646014c4..c9b5eb9b0582 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -384,8 +384,7 @@ struct mt76_usb {
>  
>  	struct mt76u_mcu {
>  		struct mutex mutex;
> -		struct completion cmpl;

Hi Stanislaw,

I was reviewing this approach and I guess it is a little bit racey since now we
are not sure that when the device is removed or suspended the pending mcu commands
are terminated and we do not have any api to stop usb transactions.
Are we sure when we access mt76x02_dev/mt76_dev structure it has not been
already removed?
Maybe we need to maintain the completion in mt76u_mcu and use it to wait the mcu
commands are terminated.

Regards,
Lorenzo

> -		struct mt76u_buf res;
> +		u8 *data;
>  		u32 msg_seq;
>  
>  		/* multiple reads */
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> index da9d05f6074d..f0c33890f1a5 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> @@ -311,13 +311,11 @@ static int __maybe_unused mt76x0_suspend(struct usb_interface *usb_intf,
>  					 pm_message_t state)
>  {
>  	struct mt76x02_dev *dev = usb_get_intfdata(usb_intf);
> -	struct mt76_usb *usb = &dev->mt76.usb;
>  
>  	mt76u_stop_queues(&dev->mt76);
>  	mt76x0u_mac_stop(dev);
>  	clear_bit(MT76_STATE_MCU_RUNNING, &dev->mt76.state);
>  	mt76x0_chip_onoff(dev, false, false);
> -	usb_kill_urb(usb->mcu.res.urb);
>  
>  	return 0;
>  }
> @@ -328,15 +326,6 @@ static int __maybe_unused mt76x0_resume(struct usb_interface *usb_intf)
>  	struct mt76_usb *usb = &dev->mt76.usb;
>  	int ret;
>  
> -	reinit_completion(&usb->mcu.cmpl);
> -	ret = mt76u_submit_buf(&dev->mt76, USB_DIR_IN,
> -			       MT_EP_IN_CMD_RESP,
> -			       &usb->mcu.res, GFP_KERNEL,
> -			       mt76u_mcu_complete_urb,
> -			       &usb->mcu.cmpl);
> -	if (ret < 0)
> -		goto err;
> -
>  	ret = mt76u_submit_rx_buffers(&dev->mt76);
>  	if (ret < 0)
>  		goto err;
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
> index f497c8e4332a..0cb8751321a1 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
> @@ -61,33 +61,21 @@ mt76x02u_multiple_mcu_reads(struct mt76_dev *dev, u8 *data, int len)
>  static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
>  {
>  	struct mt76_usb *usb = &dev->usb;
> -	struct mt76u_buf *buf = &usb->mcu.res;
> -	struct urb *urb = buf->urb;
> -	u8 *data = buf->buf;
> -	int i, ret;
> +	u8 *data = usb->mcu.data;
> +	int i, len, ret;
>  	u32 rxfce;
>  
>  	for (i = 0; i < 5; i++) {
> -		if (!wait_for_completion_timeout(&usb->mcu.cmpl,
> -						 msecs_to_jiffies(300)))
> +		ret = mt76u_bulk_msg(dev, data, MCU_RESP_URB_SIZE, &len, 300);
> +		if (ret == -ETIMEDOUT)
>  			continue;
> -
> -		if (urb->status)
> -			return -EIO;
> +		if (ret)
> +			goto out;
>  
>  		if (usb->mcu.rp)
> -			mt76x02u_multiple_mcu_reads(dev, data + 4,
> -						    urb->actual_length - 8);
> +			mt76x02u_multiple_mcu_reads(dev, data + 4, len - 8);
>  
>  		rxfce = get_unaligned_le32(data);
> -		ret = mt76u_submit_buf(dev, USB_DIR_IN,
> -				       MT_EP_IN_CMD_RESP,
> -				       buf, GFP_KERNEL,
> -				       mt76u_mcu_complete_urb,
> -				       &usb->mcu.cmpl);
> -		if (ret)
> -			return ret;
> -
>  		if (seq == FIELD_GET(MT_RX_FCE_INFO_CMD_SEQ, rxfce) &&
>  		    FIELD_GET(MT_RX_FCE_INFO_EVT_TYPE, rxfce) == EVT_CMD_DONE)
>  			return 0;
> @@ -96,9 +84,9 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
>  			FIELD_GET(MT_RX_FCE_INFO_EVT_TYPE, rxfce),
>  			seq, FIELD_GET(MT_RX_FCE_INFO_CMD_SEQ, rxfce));
>  	}
> -
> -	dev_err(dev->dev, "error: %s timed out\n", __func__);
> -	return -ETIMEDOUT;
> +out:
> +	dev_err(dev->dev, "error: %s failed with %d\n", __func__, ret);
> +	return ret;
>  }
>  
>  static int
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
> index f81a85e96922..ddb6b2c48e01 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
> @@ -100,11 +100,9 @@ static int __maybe_unused mt76x2u_suspend(struct usb_interface *intf,
>  					  pm_message_t state)
>  {
>  	struct mt76x02_dev *dev = usb_get_intfdata(intf);
> -	struct mt76_usb *usb = &dev->mt76.usb;
>  
>  	mt76u_stop_queues(&dev->mt76);
>  	mt76x2u_stop_hw(dev);
> -	usb_kill_urb(usb->mcu.res.urb);
>  
>  	return 0;
>  }
> @@ -115,15 +113,6 @@ static int __maybe_unused mt76x2u_resume(struct usb_interface *intf)
>  	struct mt76_usb *usb = &dev->mt76.usb;
>  	int err;
>  
> -	reinit_completion(&usb->mcu.cmpl);
> -	err = mt76u_submit_buf(&dev->mt76, USB_DIR_IN,
> -			       MT_EP_IN_CMD_RESP,
> -			       &usb->mcu.res, GFP_KERNEL,
> -			       mt76u_mcu_complete_urb,
> -			       &usb->mcu.cmpl);
> -	if (err < 0)
> -		goto err;
> -
>  	err = mt76u_submit_rx_buffers(&dev->mt76);
>  	if (err < 0)
>  		goto err;
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index 78191968b4fa..5c3b7f735aae 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -933,7 +933,6 @@ int mt76u_init(struct mt76_dev *dev,
>  	INIT_DELAYED_WORK(&usb->stat_work, mt76u_tx_status_data);
>  	skb_queue_head_init(&dev->rx_skb[MT_RXQ_MAIN]);
>  
> -	init_completion(&usb->mcu.cmpl);
>  	mutex_init(&usb->mcu.mutex);
>  
>  	mutex_init(&usb->usb_ctrl_mtx);
> diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
> index 72c8607da4b4..747231edc57d 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
> @@ -16,42 +16,19 @@
>  
>  #include "mt76.h"
>  
> -void mt76u_mcu_complete_urb(struct urb *urb)
> -{
> -	struct completion *cmpl = urb->context;
> -
> -	complete(cmpl);
> -}
> -EXPORT_SYMBOL_GPL(mt76u_mcu_complete_urb);
> -
>  int mt76u_mcu_init_rx(struct mt76_dev *dev)
>  {
>  	struct mt76_usb *usb = &dev->usb;
> -	int err;
>  
> -	err = mt76u_buf_alloc(dev, &usb->mcu.res, MCU_RESP_URB_SIZE,
> -			      MCU_RESP_URB_SIZE, GFP_KERNEL);
> -	if (err < 0)
> -		return err;
> -
> -	err = mt76u_submit_buf(dev, USB_DIR_IN, MT_EP_IN_CMD_RESP,
> -			       &usb->mcu.res, GFP_KERNEL,
> -			       mt76u_mcu_complete_urb,
> -			       &usb->mcu.cmpl);
> -	if (err < 0)
> -		mt76u_buf_free(&usb->mcu.res);
> -
> -	return err;
> +	usb->mcu.data = kmalloc(MCU_RESP_URB_SIZE, GFP_KERNEL);
> +	return usb->mcu.data ? 0 : -ENOMEM;
>  }
>  EXPORT_SYMBOL_GPL(mt76u_mcu_init_rx);
>  
>  void mt76u_mcu_deinit(struct mt76_dev *dev)
>  {
> -	struct mt76u_buf *buf = &dev->usb.mcu.res;
> +	struct mt76_usb *usb = &dev->usb;
>  
> -	if (buf->urb) {
> -		usb_kill_urb(buf->urb);
> -		mt76u_buf_free(buf);
> -	}
> +	kfree(usb->mcu.data);
>  }
>  EXPORT_SYMBOL_GPL(mt76u_mcu_deinit);
> -- 
> 2.7.5
> 

Re: [PATCH v3 2/3] mt76usb: use synchronous msg for mcu command responses

[Stanislaw Gruszka]

On Fri, Feb 22, 2019 at 10:53:19AM +0100, Lorenzo Bianconi wrote:
> > Use usb_bulk_msg for reading MCU command responses. This simplify code
> > a lot.
> > 
> > Together with 97a3005759c ("mt76usb: allow mt76u_bulk_msg be used
> > for reads") it also fix possible problems with rx data buffers
> > not being aligned and contained within single page. After doing
> > page_frag_alloc(1024) consecutive page_frag_alloc(PAGE_SIZE) will
> > alloc PAGE_SIZE buffer at PAGE_SIZE - 1024 offset.
> > 
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> >  drivers/net/wireless/mediatek/mt76/mt76.h          |  3 +-
> >  drivers/net/wireless/mediatek/mt76/mt76x0/usb.c    | 11 --------
> >  .../net/wireless/mediatek/mt76/mt76x02_usb_mcu.c   | 32 +++++++---------------
> >  drivers/net/wireless/mediatek/mt76/mt76x2/usb.c    | 11 --------
> >  drivers/net/wireless/mediatek/mt76/usb.c           |  1 -
> >  drivers/net/wireless/mediatek/mt76/usb_mcu.c       | 31 +++------------------
> >  6 files changed, 15 insertions(+), 74 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> > index 6092646014c4..c9b5eb9b0582 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> > @@ -384,8 +384,7 @@ struct mt76_usb {
> >  
> >  	struct mt76u_mcu {
> >  		struct mutex mutex;
> > -		struct completion cmpl;
> 
> Hi Stanislaw,
> 
> I was reviewing this approach and I guess it is a little bit racey since now we
> are not sure that when the device is removed or suspended the pending mcu commands
> are terminated and we do not have any api to stop usb transactions.
> Are we sure when we access mt76x02_dev/mt76_dev structure it has not been
> already removed?
> Maybe we need to maintain the completion in mt76u_mcu and use it to wait the mcu
> commands are terminated.

I don't think so. On suspend we do mt76xxu_mac_stop() which access registers
using mcu commands, all should be synchronized by mcu->mutex . All other
process that could use mcu command should be already stopped. Actually suspend
can be simplified.

Device remove should be properly handled by -ENODEV error returned by
usb_bulk_msg.

Stanislaw

Re: [PATCH v3 2/3] mt76usb: use synchronous msg for mcu command responses

[Lorenzo Bianconi]

> On Fri, Feb 22, 2019 at 10:53:19AM +0100, Lorenzo Bianconi wrote:
> > > Use usb_bulk_msg for reading MCU command responses. This simplify code
> > > a lot.
> > > 
> > > Together with 97a3005759c ("mt76usb: allow mt76u_bulk_msg be used
> > > for reads") it also fix possible problems with rx data buffers
> > > not being aligned and contained within single page. After doing
> > > page_frag_alloc(1024) consecutive page_frag_alloc(PAGE_SIZE) will
> > > alloc PAGE_SIZE buffer at PAGE_SIZE - 1024 offset.
> > > 
> > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > > ---
> > >  drivers/net/wireless/mediatek/mt76/mt76.h          |  3 +-
> > >  drivers/net/wireless/mediatek/mt76/mt76x0/usb.c    | 11 --------
> > >  .../net/wireless/mediatek/mt76/mt76x02_usb_mcu.c   | 32 +++++++---------------
> > >  drivers/net/wireless/mediatek/mt76/mt76x2/usb.c    | 11 --------
> > >  drivers/net/wireless/mediatek/mt76/usb.c           |  1 -
> > >  drivers/net/wireless/mediatek/mt76/usb_mcu.c       | 31 +++------------------
> > >  6 files changed, 15 insertions(+), 74 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > index 6092646014c4..c9b5eb9b0582 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > @@ -384,8 +384,7 @@ struct mt76_usb {
> > >  
> > >  	struct mt76u_mcu {
> > >  		struct mutex mutex;
> > > -		struct completion cmpl;
> > 
> > Hi Stanislaw,
> > 
> > I was reviewing this approach and I guess it is a little bit racey since now we
> > are not sure that when the device is removed or suspended the pending mcu commands
> > are terminated and we do not have any api to stop usb transactions.
> > Are we sure when we access mt76x02_dev/mt76_dev structure it has not been
> > already removed?
> > Maybe we need to maintain the completion in mt76u_mcu and use it to wait the mcu
> > commands are terminated.
> 
> I don't think so. On suspend we do mt76xxu_mac_stop() which access registers
> using mcu commands, all should be synchronized by mcu->mutex . All other
> process that could use mcu command should be already stopped. Actually suspend
> can be simplified.

ack, I missed mcu->mutex. Thx.

Regards,
Lorenzo

> 
> Device remove should be properly handled by -ENODEV error returned by
> usb_bulk_msg.
> 
> Stanislaw

[PATCH v3 3/3] mt76usb: remove usb_mcu.c

[Stanislaw Gruszka]

Don't need separate file just for kmalloc/kfree.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
v2: use dev_kmalloc()
v3: fix Makefile

 drivers/net/wireless/mediatek/mt76/Makefile        |  2 +-
 drivers/net/wireless/mediatek/mt76/mt76.h          |  2 --
 drivers/net/wireless/mediatek/mt76/mt76x0/usb.c    |  5 ----
 .../net/wireless/mediatek/mt76/mt76x2/usb_init.c   |  5 ----
 drivers/net/wireless/mediatek/mt76/usb.c           |  5 ++++
 drivers/net/wireless/mediatek/mt76/usb_mcu.c       | 34 ----------------------
 6 files changed, 6 insertions(+), 47 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt76/usb_mcu.c

diff --git a/drivers/net/wireless/mediatek/mt76/Makefile b/drivers/net/wireless/mediatek/mt76/Makefile
index fa7a44edd02d..0d6a76f6e2ba 100644
--- a/drivers/net/wireless/mediatek/mt76/Makefile
+++ b/drivers/net/wireless/mediatek/mt76/Makefile
@@ -7,7 +7,7 @@ mt76-y := \
 	mmio.o util.o trace.o dma.o mac80211.o debugfs.o eeprom.o \
 	tx.o agg-rx.o mcu.o
 
-mt76-usb-y := usb.o usb_trace.o usb_mcu.o
+mt76-usb-y := usb.o usb_trace.o
 
 CFLAGS_trace.o := -I$(src)
 CFLAGS_usb_trace.o := -I$(src)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index c9b5eb9b0582..a689af307a71 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -771,7 +771,5 @@ struct sk_buff *mt76_mcu_get_response(struct mt76_dev *dev,
 				      unsigned long expires);
 
 void mt76u_mcu_complete_urb(struct urb *urb);
-int mt76u_mcu_init_rx(struct mt76_dev *dev);
-void mt76u_mcu_deinit(struct mt76_dev *dev);
 
 #endif
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index f0c33890f1a5..91718647da02 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -79,7 +79,6 @@ static void mt76x0u_cleanup(struct mt76x02_dev *dev)
 	clear_bit(MT76_STATE_INITIALIZED, &dev->mt76.state);
 	mt76x0_chip_onoff(dev, false, false);
 	mt76u_queues_deinit(&dev->mt76);
-	mt76u_mcu_deinit(&dev->mt76);
 }
 
 static void mt76x0u_mac_stop(struct mt76x02_dev *dev)
@@ -193,10 +192,6 @@ static int mt76x0u_register_device(struct mt76x02_dev *dev)
 	if (err < 0)
 		goto out_err;
 
-	err = mt76u_mcu_init_rx(&dev->mt76);
-	if (err < 0)
-		goto out_err;
-
 	err = mt76x0u_init_hardware(dev);
 	if (err < 0)
 		goto out_err;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
index 090aaf71b3ef..1da90e58d942 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
@@ -214,10 +214,6 @@ int mt76x2u_register_device(struct mt76x02_dev *dev)
 	if (err < 0)
 		goto fail;
 
-	err = mt76u_mcu_init_rx(&dev->mt76);
-	if (err < 0)
-		goto fail;
-
 	err = mt76x2u_init_hardware(dev);
 	if (err < 0)
 		goto fail;
@@ -259,5 +255,4 @@ void mt76x2u_cleanup(struct mt76x02_dev *dev)
 	mt76x02_mcu_set_radio_state(dev, false);
 	mt76x2u_stop_hw(dev);
 	mt76u_queues_deinit(&dev->mt76);
-	mt76u_mcu_deinit(&dev->mt76);
 }
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 5c3b7f735aae..792f5012f1b1 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -577,9 +577,14 @@ EXPORT_SYMBOL_GPL(mt76u_submit_rx_buffers);
 
 static int mt76u_alloc_rx(struct mt76_dev *dev)
 {
+	struct mt76_usb *usb = &dev->usb;
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
 	int i, err;
 
+	usb->mcu.data = devm_kmalloc(dev->dev, MCU_RESP_URB_SIZE, GFP_KERNEL);
+	if (!usb->mcu.data)
+		return -ENOMEM;
+
 	spin_lock_init(&q->rx_page_lock);
 	spin_lock_init(&q->lock);
 	q->entry = devm_kcalloc(dev->dev,
diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
deleted file mode 100644
index 747231edc57d..000000000000
--- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * Copyright (C) 2018 Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
- *
- * Permission to use, copy, modify, and/or distribute this software for any
- * purpose with or without fee is hereby granted, provided that the above
- * copyright notice and this permission notice appear in all copies.
- *
- * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
- * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
- * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
- * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
- * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
- * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
- */
-
-#include "mt76.h"
-
-int mt76u_mcu_init_rx(struct mt76_dev *dev)
-{
-	struct mt76_usb *usb = &dev->usb;
-
-	usb->mcu.data = kmalloc(MCU_RESP_URB_SIZE, GFP_KERNEL);
-	return usb->mcu.data ? 0 : -ENOMEM;
-}
-EXPORT_SYMBOL_GPL(mt76u_mcu_init_rx);
-
-void mt76u_mcu_deinit(struct mt76_dev *dev)
-{
-	struct mt76_usb *usb = &dev->usb;
-
-	kfree(usb->mcu.data);
-}
-EXPORT_SYMBOL_GPL(mt76u_mcu_deinit);
-- 
2.7.5

Re: [PATCH v3 1/3] mt76usb: allow mt76u_bulk_msg be used for reads

[Felix Fietkau]

On 2019-02-20 17:15, Stanislaw Gruszka wrote:
> Extend mt76u_bulk_msg() such it can be used for synchronous bulk reads.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Series applied, thanks.

- Felix