[PATCH 0/4] staging: r8188eu: clean up usb_write_port_complete

[PATCH 0/4] staging: r8188eu: clean up usb_write_port_complete

[Martin Kaiser]

Clean up and simplify the usb_write_port_complete callback function. Yet
again, this is based on the previous patches I submitted.

Martin Kaiser (4):
  staging: r8188eu: refactor status handling in usb_write_port_complete
  staging: r8188eu: reformat usb_write_port_complete
  staging: r8188eu: remove unused function parameter
  staging: r8188eu: always process urb status

 .../staging/r8188eu/include/usb_ops_linux.h   |  2 -
 .../staging/r8188eu/os_dep/usb_ops_linux.c    | 46 ++++++++-----------
 2 files changed, 18 insertions(+), 30 deletions(-)

-- 
2.30.2

[PATCH 1/4] staging: r8188eu: refactor status handling in usb_write_port_complete

[Martin Kaiser]

Refactor the satus handling in usb_write_port_complete. Make it clearer
what happens for each status and avoid all the goto statements.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 .../staging/r8188eu/os_dep/usb_ops_linux.c    | 29 +++++++++----------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 257bcf496012..8494b80a08e5 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -48,21 +48,20 @@ static void usb_write_port_complete(struct urb *purb, struct pt_regs *regs)
 	    padapter->bWritePortCancel)
 		goto check_completion;
 
-	if (purb->status) {
-		if (purb->status == -EINPROGRESS) {
-			goto check_completion;
-		} else if (purb->status == -ENOENT) {
-			goto check_completion;
-		} else if (purb->status == -ECONNRESET) {
-			goto check_completion;
-		} else if (purb->status == -ESHUTDOWN) {
-			padapter->bDriverStopped = true;
-			goto check_completion;
-		} else if ((purb->status != -EPIPE) && (purb->status != -EPROTO)) {
-			padapter->bSurpriseRemoved = true;
-
-			goto check_completion;
-		}
+	switch (purb->status) {
+	case 0:
+	case -EINPROGRESS:
+	case -ENOENT:
+	case -ECONNRESET:
+	case -EPIPE:
+	case -EPROTO:
+		break;
+	case -ESHUTDOWN:
+		padapter->bDriverStopped = true;
+		break;
+	default:
+		padapter->bSurpriseRemoved = true;
+		break;
 	}
 
 check_completion:
-- 
2.30.2

[PATCH 2/4] staging: r8188eu: reformat usb_write_port_complete

[Martin Kaiser]

This trivial patch reformats the usb_write_port_complete function.
Hopefully, this makes the code a bit easier to read.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/os_dep/usb_ops_linux.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 8494b80a08e5..7da13b87d726 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -38,14 +38,13 @@ void rtw_read_port_cancel(struct adapter *padapter)
 static void usb_write_port_complete(struct urb *purb, struct pt_regs *regs)
 {
 	struct xmit_buf *pxmitbuf = (struct xmit_buf *)purb->context;
-	struct adapter	*padapter = pxmitbuf->padapter;
-	struct xmit_priv	*pxmitpriv = &padapter->xmitpriv;
+	struct adapter *padapter = pxmitbuf->padapter;
+	struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
 
 	if (pxmitbuf->flags == HIGH_QUEUE_INX)
 		rtw_chk_hi_queue_cmd(padapter);
 
-	if (padapter->bSurpriseRemoved || padapter->bDriverStopped ||
-	    padapter->bWritePortCancel)
+	if (padapter->bSurpriseRemoved || padapter->bDriverStopped || padapter->bWritePortCancel)
 		goto check_completion;
 
 	switch (purb->status) {
@@ -66,13 +65,9 @@ static void usb_write_port_complete(struct urb *purb, struct pt_regs *regs)
 
 check_completion:
 	rtw_sctx_done_err(&pxmitbuf->sctx,
-			  purb->status ? RTW_SCTX_DONE_WRITE_PORT_ERR :
-			  RTW_SCTX_DONE_SUCCESS);
-
+			  purb->status ? RTW_SCTX_DONE_WRITE_PORT_ERR : RTW_SCTX_DONE_SUCCESS);
 	rtw_free_xmitbuf(pxmitpriv, pxmitbuf);
-
 	tasklet_hi_schedule(&pxmitpriv->xmit_tasklet);
-
 }
 
 u32 rtw_write_port(struct adapter *padapter, u32 addr, u32 cnt, u8 *wmem)
-- 
2.30.2

[PATCH 3/4] staging: r8188eu: remove unused function parameter

[Martin Kaiser]

The regs parameter of the usb_write_port_complete function is not used. We
can remove it.

We can also remove the macro to hide the regs parameter when
usb_write_port_complete is used as callback function for an urb transfer.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/include/usb_ops_linux.h | 2 --
 drivers/staging/r8188eu/os_dep/usb_ops_linux.c  | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/include/usb_ops_linux.h b/drivers/staging/r8188eu/include/usb_ops_linux.h
index 966688eedf66..e406a1fccda7 100644
--- a/drivers/staging/r8188eu/include/usb_ops_linux.h
+++ b/drivers/staging/r8188eu/include/usb_ops_linux.h
@@ -19,8 +19,6 @@
 	usb_bulkout_zero_complete(purb)
 #define usb_write_mem_complete(purb, regs)		\
 	usb_write_mem_complete(purb)
-#define usb_write_port_complete(purb, regs)		\
-	usb_write_port_complete(purb)
 #define usb_read_port_complete(purb, regs)		\
 	usb_read_port_complete(purb)
 #define usb_read_interrupt_complete(purb, regs)		\
diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 7da13b87d726..3fd080091340 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -35,7 +35,7 @@ void rtw_read_port_cancel(struct adapter *padapter)
 	}
 }
 
-static void usb_write_port_complete(struct urb *purb, struct pt_regs *regs)
+static void usb_write_port_complete(struct urb *purb)
 {
 	struct xmit_buf *pxmitbuf = (struct xmit_buf *)purb->context;
 	struct adapter *padapter = pxmitbuf->padapter;
-- 
2.30.2

[PATCH 4/4] staging: r8188eu: always process urb status

[Martin Kaiser]

Remove the if clause in usb_write_port_complete and process the urb
status regardless of bSurpriseRemoved, bDriverStopped and
bWritePortCancel.

The only possible results of urb status processing are updates to
bSurpriseRemoved and bDriverStopped. All of the three status variable are
set to true only if the whole USB processing has to be stopped (when the
driver is unloaded or when the system goes to sleep).

It's no problem if one of the "stop everything" variables is already set
and the urb status processing sets another one.

This patch removes the last goto in usb_write_port_complete. It's also
part of the ongoing effort to limit the use of the "stop everything"
variables.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/os_dep/usb_ops_linux.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
index 3fd080091340..62106d2f82ad 100644
--- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
@@ -44,9 +44,6 @@ static void usb_write_port_complete(struct urb *purb)
 	if (pxmitbuf->flags == HIGH_QUEUE_INX)
 		rtw_chk_hi_queue_cmd(padapter);
 
-	if (padapter->bSurpriseRemoved || padapter->bDriverStopped || padapter->bWritePortCancel)
-		goto check_completion;
-
 	switch (purb->status) {
 	case 0:
 	case -EINPROGRESS:
@@ -63,7 +60,6 @@ static void usb_write_port_complete(struct urb *purb)
 		break;
 	}
 
-check_completion:
 	rtw_sctx_done_err(&pxmitbuf->sctx,
 			  purb->status ? RTW_SCTX_DONE_WRITE_PORT_ERR : RTW_SCTX_DONE_SUCCESS);
 	rtw_free_xmitbuf(pxmitpriv, pxmitbuf);
-- 
2.30.2

Re: [PATCH 0/4] staging: r8188eu: clean up usb_write_port_complete

[Philipp Hortmann]

On 1/10/23 21:56, Martin Kaiser wrote:
> Clean up and simplify the usb_write_port_complete callback function. Yet
> again, this is based on the previous patches I submitted.
> 
> Martin Kaiser (4):
>    staging: r8188eu: refactor status handling in usb_write_port_complete
>    staging: r8188eu: reformat usb_write_port_complete
>    staging: r8188eu: remove unused function parameter
>    staging: r8188eu: always process urb status
> 
>   .../staging/r8188eu/include/usb_ops_linux.h   |  2 -
>   .../staging/r8188eu/os_dep/usb_ops_linux.c    | 46 ++++++++-----------
>   2 files changed, 18 insertions(+), 30 deletions(-)
> 


Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150

Re: [PATCH 4/4] staging: r8188eu: always process urb status

[Pavel Skripkin]

Hi Martin,

Martin Kaiser <martin@kaiser.cx> says:
> Remove the if clause in usb_write_port_complete and process the urb
> status regardless of bSurpriseRemoved, bDriverStopped and
> bWritePortCancel.
> 
> The only possible results of urb status processing are updates to
> bSurpriseRemoved and bDriverStopped. All of the three status variable are
> set to true only if the whole USB processing has to be stopped (when the
> driver is unloaded or when the system goes to sleep).
> 

Not sure if it matters but we still have that weird rule that after 5 
failed usb read/writes bSurpriseRemoved will be set to true

Maybe also worth removing above logic?


> It's no problem if one of the "stop everything" variables is already set
> and the urb status processing sets another one.
> 
> This patch removes the last goto in usb_write_port_complete. It's also
> part of the ongoing effort to limit the use of the "stop everything"
> variables.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   drivers/staging/r8188eu/os_dep/usb_ops_linux.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
> index 3fd080091340..62106d2f82ad 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_ops_linux.c
> @@ -44,9 +44,6 @@ static void usb_write_port_complete(struct urb *purb)
>   	if (pxmitbuf->flags == HIGH_QUEUE_INX)
>   		rtw_chk_hi_queue_cmd(padapter);
>   
> -	if (padapter->bSurpriseRemoved || padapter->bDriverStopped || padapter->bWritePortCancel)
> -		goto check_completion;
> -
>   	switch (purb->status) {
>   	case 0:
>   	case -EINPROGRESS:
> @@ -63,7 +60,6 @@ static void usb_write_port_complete(struct urb *purb)
>   		break;
>   	}
>   
> -check_completion:
>   	rtw_sctx_done_err(&pxmitbuf->sctx,
>   			  purb->status ? RTW_SCTX_DONE_WRITE_PORT_ERR : RTW_SCTX_DONE_SUCCESS);
>   	rtw_free_xmitbuf(pxmitpriv, pxmitbuf);





With regards,
Pavel Skripkin

Re: [PATCH 4/4] staging: r8188eu: always process urb status

[Martin Kaiser]

Hi Pavel,

Thus wrote Pavel Skripkin (paskripkin@gmail.com):

> Not sure if it matters but we still have that weird rule that after 5 failed
> usb read/writes bSurpriseRemoved will be set to true

> Maybe also worth removing above logic?

thanks for making we aware of this retry counter. It does really look
strange. Still, I'm not sure that I understand this well enough to
submit a patch for removal. I'll play around with this as time
permits...

Best regards,
Martin