[PATCH 0/3] mt76: usb: alignment and endianes improvements

[PATCH 0/3] mt76: usb: alignment and endianes improvements

[Stanislaw Gruszka]

Fix endian bug and do some minor optimizations in mt76u_{copy,rr,wr} .

I do not mark cc stable endian fix as noboby reported this issue,
i.e. use the driver on big endian machine, but make it a separate patch,
so it can be backported if needed.

This is on top of:
[PATCH] mt76: round up length on mt76_wr_copy 

Stanislaw Gruszka (3):
  mt76: usb: fix endian in mt76u_copy
  mt76: usb: remove unneeded {put,get}_unaligned
  mt76: usb: use full intermediate buffer in mt76u_copy

 drivers/net/wireless/mediatek/mt76/mt76.h |  5 ++++-
 drivers/net/wireless/mediatek/mt76/usb.c  | 27 ++++++++++++++++-----------
 2 files changed, 20 insertions(+), 12 deletions(-)

-- 
1.9.3

[PATCH 1/3] mt76: usb: fix endian in mt76u_copy

[Stanislaw Gruszka]

In contrast to mt76_wr() which we use to program registers,
on mt76_wr_copy() we should not change endian of the data.

Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 87ecbe290f99..db90ec6b8775 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -165,11 +165,11 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
 
 	mutex_lock(&usb->usb_ctrl_mtx);
 	for (i = 0; i < DIV_ROUND_UP(len, 4); i++) {
-		put_unaligned_le32(val[i], usb->data);
+		put_unaligned(val[i], usb->data);
 		ret = __mt76u_vendor_request(dev, MT_VEND_MULTI_WRITE,
 					     USB_DIR_OUT | USB_TYPE_VENDOR,
 					     0, offset + i * 4, usb->data,
-					     sizeof(__le32));
+					     sizeof(u32));
 		if (ret < 0)
 			break;
 	}
-- 
1.9.3

Re: [PATCH 1/3] mt76: usb: fix endian in mt76u_copy

[Stanislaw Gruszka]

On Tue, Jul 02, 2019 at 05:05:59PM +0200, Stanislaw Gruszka wrote:
> In contrast to mt76_wr() which we use to program registers,
> on mt76_wr_copy() we should not change endian of the data.
> 
> Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index 87ecbe290f99..db90ec6b8775 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -165,11 +165,11 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
>  
>  	mutex_lock(&usb->usb_ctrl_mtx);
>  	for (i = 0; i < DIV_ROUND_UP(len, 4); i++) {
> -		put_unaligned_le32(val[i], usb->data);
> +		put_unaligned(val[i], usb->data);

This is bug as put_unaligned() use size based second argument
pointer type, correct version should looks like this:

		put_unaligned(val[i], (u32 *) usb->data);

Stanislaw

[PATCH 2/3] mt76: usb: remove unneeded {put,get}_unaligned

[Stanislaw Gruszka]

Compiler give us guaranties on variables alignment, so use
an variable as buffer when read/write registers and remove
unneeded {put,get}_unaligned.

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

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 56bf93a8988e..094e6e543542 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -389,7 +389,10 @@ enum mt76u_out_ep {
 #define MCU_RESP_URB_SIZE	1024
 struct mt76_usb {
 	struct mutex usb_ctrl_mtx;
-	u8 data[32];
+	union {
+		u8 data[32];
+		__le32 reg_val;
+	};
 
 	struct tasklet_struct rx_tasklet;
 	struct delayed_work stat_work;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index db90ec6b8775..a61bb8171557 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -95,9 +95,9 @@ static u32 __mt76u_rr(struct mt76_dev *dev, u32 addr)
 
 	ret = __mt76u_vendor_request(dev, req,
 				     USB_DIR_IN | USB_TYPE_VENDOR,
-				     0, offset, usb->data, sizeof(__le32));
+				     0, offset, &usb->reg_val, sizeof(__le32));
 	if (ret == sizeof(__le32))
-		data = get_unaligned_le32(usb->data);
+		data = le32_to_cpu(usb->reg_val);
 	trace_usb_reg_rr(dev, addr, data);
 
 	return data;
@@ -131,10 +131,10 @@ static void __mt76u_wr(struct mt76_dev *dev, u32 addr, u32 val)
 	}
 	offset = addr & ~MT_VEND_TYPE_MASK;
 
-	put_unaligned_le32(val, usb->data);
+	usb->reg_val = cpu_to_le32(val);
 	__mt76u_vendor_request(dev, req,
 			       USB_DIR_OUT | USB_TYPE_VENDOR, 0,
-			       offset, usb->data, sizeof(__le32));
+			       offset, &usb->reg_val, sizeof(__le32));
 	trace_usb_reg_wr(dev, addr, val);
 }
 
-- 
1.9.3

[PATCH 3/3] mt76: usb: use full intermediate buffer in mt76u_copy

[Stanislaw Gruszka]

Instead of use several 4 bytes usb requests, use full 32 bytes buffer
to copy data to device. With the change we use less requests and copy
exact data size to the device regardless size is multiple of 4 or not.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index a61bb8171557..7c564cc68c7c 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -160,19 +160,24 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
 		       const void *data, int len)
 {
 	struct mt76_usb *usb = &dev->usb;
-	const u32 *val = data;
-	int i, ret;
+	int ret, req_len;
 
 	mutex_lock(&usb->usb_ctrl_mtx);
-	for (i = 0; i < DIV_ROUND_UP(len, 4); i++) {
-		put_unaligned(val[i], usb->data);
+	do {
+		req_len = min_t(int, len, sizeof(usb->data));
+
+		memcpy(usb->data, data, req_len);
+
 		ret = __mt76u_vendor_request(dev, MT_VEND_MULTI_WRITE,
 					     USB_DIR_OUT | USB_TYPE_VENDOR,
-					     0, offset + i * 4, usb->data,
-					     sizeof(u32));
+					     0, offset, usb->data, req_len);
 		if (ret < 0)
 			break;
-	}
+
+		data += req_len;
+		offset += req_len;
+		len -= req_len;
+	} while (len > 0);
 	mutex_unlock(&usb->usb_ctrl_mtx);
 }
 
-- 
1.9.3

Re: [PATCH 3/3] mt76: usb: use full intermediate buffer in mt76u_copy

[Stanislaw Gruszka]

On Tue, Jul 02, 2019 at 05:06:01PM +0200, Stanislaw Gruszka wrote:
> Instead of use several 4 bytes usb requests, use full 32 bytes buffer
> to copy data to device. With the change we use less requests and copy
> exact data size to the device regardless size is multiple of 4 or not.

And this does not work correctly on some usb hosts, request which
are not multiple of 4 do not ended being written to hardware, 
what results in original problem of having last part of beacon
corrupted.

I would prefer to drop this set - and I would post 2 patches
(first patch fixed and third dropped). But since this is now in
Felix's wireless tree I guess I need to post fixes on top of this
set?

Stanislaw

Re: [PATCH 3/3] mt76: usb: use full intermediate buffer in mt76u_copy

[Felix Fietkau]

On 2019-07-09 12:12, Stanislaw Gruszka wrote:
> On Tue, Jul 02, 2019 at 05:06:01PM +0200, Stanislaw Gruszka wrote:
>> Instead of use several 4 bytes usb requests, use full 32 bytes buffer
>> to copy data to device. With the change we use less requests and copy
>> exact data size to the device regardless size is multiple of 4 or not.
> 
> And this does not work correctly on some usb hosts, request which
> are not multiple of 4 do not ended being written to hardware, 
> what results in original problem of having last part of beacon
> corrupted.
> 
> I would prefer to drop this set - and I would post 2 patches
> (first patch fixed and third dropped). But since this is now in
> Felix's wireless tree I guess I need to post fixes on top of this
> set?
I haven't sent out a pull request yet, so I can still drop those patches
and apply replacements.

- Felix

Re: [PATCH 0/3] mt76: usb: alignment and endianes improvements

[Lorenzo Bianconi]

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

> Fix endian bug and do some minor optimizations in mt76u_{copy,rr,wr} .
> 
> I do not mark cc stable endian fix as noboby reported this issue,
> i.e. use the driver on big endian machine, but make it a separate patch,
> so it can be backported if needed.
> 

Tested-by: Lorenzo Bianconi <lorenzo@kernel.org>

> This is on top of:
> [PATCH] mt76: round up length on mt76_wr_copy 
> 
> Stanislaw Gruszka (3):
>   mt76: usb: fix endian in mt76u_copy
>   mt76: usb: remove unneeded {put,get}_unaligned
>   mt76: usb: use full intermediate buffer in mt76u_copy
> 
>  drivers/net/wireless/mediatek/mt76/mt76.h |  5 ++++-
>  drivers/net/wireless/mediatek/mt76/usb.c  | 27 ++++++++++++++++-----------
>  2 files changed, 20 insertions(+), 12 deletions(-)
> 
> -- 
> 1.9.3
> 

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