[PATCH] Revert "staging: r8188eu: simplify rtw_get_ff_hwaddr"

[PATCH] Revert "staging: r8188eu: simplify rtw_get_ff_hwaddr"

[Martin Kaiser]

This reverts commit fd48124e09825797bdc8ff0120f2401030c618ee.

The cleanup in this commit removes the qsel to addr mappings in
rtw_get_ff_hwaddr. The underlying assumption is that rtw_write_port
uses its addr parameter only for the high_queue check.

This is obviously incorrect as rtw_write_port calls
ffaddr2pipehdl(pdvobj, addr);
where addr is mapped to a usb bulk endpoint.

Unfortunately, testing did not show any problems. The Edimax V2 on which I
tested has two bulk out endpoints. I guess that with the incorrect patch,
addr could only be 0 (no high queue) or 6 (high queue), both of which were
mapped to the first bulk out endpoint. Data transfers did still work...

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---

Hello Greg,

sorry for introducing a regression in commit fd48124e0982 ("staging:
r8188eu: simplify rtw_get_ff_hwaddr").

Could you take this revert before the 6.3 pull request?

Thanks,
Martin

 drivers/staging/r8188eu/core/rtw_xmit.c    | 32 +++++++++++++++++++++-
 drivers/staging/r8188eu/include/rtw_xmit.h |  7 +++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index d0761ac2e695..df88b3e29e77 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -1568,9 +1568,39 @@ static int rtw_br_client_tx(struct adapter *padapter, struct sk_buff **pskb)
 
 u32 rtw_get_ff_hwaddr(struct xmit_frame *pxmitframe)
 {
+	u32 addr;
 	struct pkt_attrib *pattrib = &pxmitframe->attrib;
 
-	return pattrib->qsel == 0x11 ? HIGH_QUEUE_INX : 0;
+	switch (pattrib->qsel) {
+	case 0:
+	case 3:
+		addr = BE_QUEUE_INX;
+		break;
+	case 1:
+	case 2:
+		addr = BK_QUEUE_INX;
+		break;
+	case 4:
+	case 5:
+		addr = VI_QUEUE_INX;
+		break;
+	case 6:
+	case 7:
+		addr = VO_QUEUE_INX;
+		break;
+	case 0x10:
+		addr = BCN_QUEUE_INX;
+		break;
+	case 0x11:/* BC/MC in PS (HIQ) */
+		addr = HIGH_QUEUE_INX;
+		break;
+	case 0x12:
+	default:
+		addr = MGT_QUEUE_INX;
+		break;
+	}
+
+	return addr;
 }
 
 /*
diff --git a/drivers/staging/r8188eu/include/rtw_xmit.h b/drivers/staging/r8188eu/include/rtw_xmit.h
index e4e5af198eee..feeac85aedb0 100644
--- a/drivers/staging/r8188eu/include/rtw_xmit.h
+++ b/drivers/staging/r8188eu/include/rtw_xmit.h
@@ -26,7 +26,14 @@
 #define XMIT_BE_QUEUE		(2)
 #define XMIT_BK_QUEUE		(3)
 
+#define VO_QUEUE_INX		0
+#define VI_QUEUE_INX		1
+#define BE_QUEUE_INX		2
+#define BK_QUEUE_INX		3
+#define BCN_QUEUE_INX		4
+#define MGT_QUEUE_INX		5
 #define HIGH_QUEUE_INX		6
+#define TXCMD_QUEUE_INX		7
 
 #define HW_QUEUE_ENTRY		8
 
-- 
2.30.2

Re: [PATCH] Revert "staging: r8188eu: simplify rtw_get_ff_hwaddr"

[Dan Carpenter]

On Sat, Feb 11, 2023 at 07:32:05PM +0100, Martin Kaiser wrote:
> This reverts commit fd48124e09825797bdc8ff0120f2401030c618ee.
> 
> The cleanup in this commit removes the qsel to addr mappings in
> rtw_get_ff_hwaddr. The underlying assumption is that rtw_write_port
> uses its addr parameter only for the high_queue check.
> 
> This is obviously incorrect as rtw_write_port calls
> ffaddr2pipehdl(pdvobj, addr);
> where addr is mapped to a usb bulk endpoint.
> 
> Unfortunately, testing did not show any problems. The Edimax V2 on which I
> tested has two bulk out endpoints. I guess that with the incorrect patch,
> addr could only be 0 (no high queue) or 6 (high queue), both of which were
> mapped to the first bulk out endpoint. Data transfers did still work...
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> 
> Hello Greg,
> 
> sorry for introducing a regression in commit fd48124e0982 ("staging:
> r8188eu: simplify rtw_get_ff_hwaddr").
> 
> Could you take this revert before the 6.3 pull request?
> 
> Thanks,
> Martin
> 

I feel like the ancient `git revert` script is not at all in line with
current standards and sets people up for failure.  This one at least
has a commit message.  But
1) The subject doesn't have a correct patch prefix.
2) "commit fd48124e09825797bdc8ff0120f2401030c618ee" is not human
   readable or how we describe commits these days with a 12 char hash.
3) There is no fixes tag.

regards,
dan carpenter

[PATCH v2] staging: r8188eu: Revert "staging: r8188eu: simplify rtw_get_ff_hwaddr"

[Martin Kaiser]

This reverts commit fd48124e0982 ("staging: r8188eu: simplify
rtw_get_ff_hwaddr").

The cleanup in this commit removes the qsel to addr mappings in
rtw_get_ff_hwaddr. The underlying assumption is that rtw_write_port
uses its addr parameter only for the high_queue check.

This is obviously incorrect as rtw_write_port calls
ffaddr2pipehdl(pdvobj, addr);
where addr is mapped to a usb bulk endpoint.

Unfortunately, testing did not show any problems. The Edimax V2 on which I
tested has two bulk out endpoints. I guess that with the incorrect patch,
addr could only be 0 (no high queue) or 6 (high queue), both of which were
mapped to the first bulk out endpoint. Data transfers did still work...

Fixes: fd48124e0982 ("staging: r8188eu: simplify rtw_get_ff_hwaddr")
Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - use correct patch prefix
 - add a Fixes tag
 - use 12-digit hash to refer to the reverted commit

 drivers/staging/r8188eu/core/rtw_xmit.c    | 32 +++++++++++++++++++++-
 drivers/staging/r8188eu/include/rtw_xmit.h |  7 +++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index d0761ac2e695..df88b3e29e77 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -1568,9 +1568,39 @@ static int rtw_br_client_tx(struct adapter *padapter, struct sk_buff **pskb)
 
 u32 rtw_get_ff_hwaddr(struct xmit_frame *pxmitframe)
 {
+	u32 addr;
 	struct pkt_attrib *pattrib = &pxmitframe->attrib;
 
-	return pattrib->qsel == 0x11 ? HIGH_QUEUE_INX : 0;
+	switch (pattrib->qsel) {
+	case 0:
+	case 3:
+		addr = BE_QUEUE_INX;
+		break;
+	case 1:
+	case 2:
+		addr = BK_QUEUE_INX;
+		break;
+	case 4:
+	case 5:
+		addr = VI_QUEUE_INX;
+		break;
+	case 6:
+	case 7:
+		addr = VO_QUEUE_INX;
+		break;
+	case 0x10:
+		addr = BCN_QUEUE_INX;
+		break;
+	case 0x11:/* BC/MC in PS (HIQ) */
+		addr = HIGH_QUEUE_INX;
+		break;
+	case 0x12:
+	default:
+		addr = MGT_QUEUE_INX;
+		break;
+	}
+
+	return addr;
 }
 
 /*
diff --git a/drivers/staging/r8188eu/include/rtw_xmit.h b/drivers/staging/r8188eu/include/rtw_xmit.h
index e4e5af198eee..feeac85aedb0 100644
--- a/drivers/staging/r8188eu/include/rtw_xmit.h
+++ b/drivers/staging/r8188eu/include/rtw_xmit.h
@@ -26,7 +26,14 @@
 #define XMIT_BE_QUEUE		(2)
 #define XMIT_BK_QUEUE		(3)
 
+#define VO_QUEUE_INX		0
+#define VI_QUEUE_INX		1
+#define BE_QUEUE_INX		2
+#define BK_QUEUE_INX		3
+#define BCN_QUEUE_INX		4
+#define MGT_QUEUE_INX		5
 #define HIGH_QUEUE_INX		6
+#define TXCMD_QUEUE_INX		7
 
 #define HW_QUEUE_ENTRY		8
 
-- 
2.30.2

Re: [PATCH] Revert "staging: r8188eu: simplify rtw_get_ff_hwaddr"

[Martin Kaiser]

Thus wrote Dan Carpenter (error27@gmail.com):

> On Sat, Feb 11, 2023 at 07:32:05PM +0100, Martin Kaiser wrote:
> > This reverts commit fd48124e09825797bdc8ff0120f2401030c618ee.

> > The cleanup in this commit removes the qsel to addr mappings in
> > rtw_get_ff_hwaddr. The underlying assumption is that rtw_write_port
> > uses its addr parameter only for the high_queue check.

> > This is obviously incorrect as rtw_write_port calls
> > ffaddr2pipehdl(pdvobj, addr);
> > where addr is mapped to a usb bulk endpoint.

> > Unfortunately, testing did not show any problems. The Edimax V2 on which I
> > tested has two bulk out endpoints. I guess that with the incorrect patch,
> > addr could only be 0 (no high queue) or 6 (high queue), both of which were
> > mapped to the first bulk out endpoint. Data transfers did still work...

> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---

> > Hello Greg,

> > sorry for introducing a regression in commit fd48124e0982 ("staging:
> > r8188eu: simplify rtw_get_ff_hwaddr").

> > Could you take this revert before the 6.3 pull request?

> > Thanks,
> > Martin


> I feel like the ancient `git revert` script is not at all in line with
> current standards and sets people up for failure.  This one at least
> has a commit message.  But
> 1) The subject doesn't have a correct patch prefix.
> 2) "commit fd48124e09825797bdc8ff0120f2401030c618ee" is not human
>    readable or how we describe commits these days with a 12 char hash.
> 3) There is no fixes tag.

Thanks, I fixed these points in v2.

Best regards,
Martin

Re: [PATCH v2] staging: r8188eu: Revert "staging: r8188eu: simplify rtw_get_ff_hwaddr"

[Dan Carpenter]

On Mon, Feb 13, 2023 at 08:54:08PM +0100, Martin Kaiser wrote:
> This reverts commit fd48124e0982 ("staging: r8188eu: simplify
> rtw_get_ff_hwaddr").
> 
> The cleanup in this commit removes the qsel to addr mappings in
> rtw_get_ff_hwaddr. The underlying assumption is that rtw_write_port
> uses its addr parameter only for the high_queue check.
> 
> This is obviously incorrect as rtw_write_port calls
> ffaddr2pipehdl(pdvobj, addr);
> where addr is mapped to a usb bulk endpoint.
> 
> Unfortunately, testing did not show any problems. The Edimax V2 on which I
> tested has two bulk out endpoints. I guess that with the incorrect patch,
> addr could only be 0 (no high queue) or 6 (high queue), both of which were
> mapped to the first bulk out endpoint. Data transfers did still work...
> 
> Fixes: fd48124e0982 ("staging: r8188eu: simplify rtw_get_ff_hwaddr")
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> v2:
>  - use correct patch prefix
>  - add a Fixes tag
>  - use 12-digit hash to refer to the reverted commit

Thanks, Martin!

regards,
dan carpenter