[PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[Phillip Potter]

This small patch set cleans up the c2h_handler code in the HAL layer
of the driver. In r8188eu, this field of struct hal_ops, is not even
used, so dependent code has always returned _FAIL. For this reason, we
should remove this function pointer field, and the wrapper function
which checks it. This is done in stages by this set, and helps get
the driver closer to the pointer where the HAL layer is
deleted/integrated as necessary and no longer a separate entity.

Phillip Potter (3):
  staging: r8188eu: remove c2h_handler field from struct hal_ops
  staging: r8188eu: simplify c2h_evt_hdl function
  staging: r8188eu: remove rtw_hal_c2h_handler function

 drivers/staging/r8188eu/core/rtw_cmd.c     | 23 +++-------------------
 drivers/staging/r8188eu/hal/hal_intf.c     |  9 ---------
 drivers/staging/r8188eu/include/hal_intf.h |  4 ----
 3 files changed, 3 insertions(+), 33 deletions(-)

-- 
2.31.1

[PATCH 1/3] staging: r8188eu: remove c2h_handler field from struct hal_ops

[Phillip Potter]

Remove c2h_handler function pointer field from struct hal_ops in
include/hal_intf.h, as it is never set in this driver, and remove
the check for a non-NULL value in the rtw_hal_c2h_handler wrapper
function in hal/hal_intf.c as well. As the function always returns
_FAIL anyway, just modify it to do this unconditionally.

The motivation for removing this field is that it is more code from
the unwanted HAL layer that can be stripped out.

Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 drivers/staging/r8188eu/hal/hal_intf.c     | 6 +-----
 drivers/staging/r8188eu/include/hal_intf.h | 2 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/hal_intf.c b/drivers/staging/r8188eu/hal/hal_intf.c
index a6d589e89aeb..0c835f9cd181 100644
--- a/drivers/staging/r8188eu/hal/hal_intf.c
+++ b/drivers/staging/r8188eu/hal/hal_intf.c
@@ -428,11 +428,7 @@ void rtw_hal_reset_security_engine(struct adapter *adapter)
 
 s32 rtw_hal_c2h_handler(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt)
 {
-	s32 ret = _FAIL;
-
-	if (adapter->HalFunc.c2h_handler)
-		ret = adapter->HalFunc.c2h_handler(adapter, c2h_evt);
-	return ret;
+	return _FAIL;
 }
 
 c2h_id_filter rtw_hal_c2h_id_filter_ccx(struct adapter *adapter)
diff --git a/drivers/staging/r8188eu/include/hal_intf.h b/drivers/staging/r8188eu/include/hal_intf.h
index fa252540e596..4603f9212030 100644
--- a/drivers/staging/r8188eu/include/hal_intf.h
+++ b/drivers/staging/r8188eu/include/hal_intf.h
@@ -251,8 +251,6 @@ struct hal_ops {
 
 	void (*hal_notch_filter)(struct adapter *adapter, bool enable);
 	void (*hal_reset_security_engine)(struct adapter *adapter);
-	s32 (*c2h_handler)(struct adapter *padapter,
-			   struct c2h_evt_hdr *c2h_evt);
 	c2h_id_filter c2h_id_filter_ccx;
 };
 
-- 
2.31.1

[PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Phillip Potter]

Simplify c2h_evt_hdl function by removing majority of its code. The
function always returns _FAIL anyway, due to the wrapper function it
calls always returning _FAIL. For this reason, it is better to just
return _FAIL directly.

Leave the call to c2h_evt_read in place, as without it, event handling
semantics of the driver would be changed, despite nothing actually being
done with the event.

Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 drivers/staging/r8188eu/core/rtw_cmd.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index ce73ac7cf973..b520c6b43c03 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -1854,27 +1854,12 @@ u8 rtw_c2h_wk_cmd(struct adapter *padapter, u8 *c2h_evt)
 
 static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
 {
-	s32 ret = _FAIL;
 	u8 buf[16];
 
-	if (!c2h_evt) {
-		/* No c2h event in cmd_obj, read c2h event before handling*/
-		if (c2h_evt_read(adapter, buf) == _SUCCESS) {
-			c2h_evt = (struct c2h_evt_hdr *)buf;
+	if (!c2h_evt)
+		c2h_evt_read(adapter, buf);
 
-			if (filter && !filter(c2h_evt->id))
-				goto exit;
-
-			ret = rtw_hal_c2h_handler(adapter, c2h_evt);
-		}
-	} else {
-		if (filter && !filter(c2h_evt->id))
-			goto exit;
-
-		ret = rtw_hal_c2h_handler(adapter, c2h_evt);
-	}
-exit:
-	return ret;
+	return _FAIL;
 }
 
 static void c2h_wk_callback(struct work_struct *work)
-- 
2.31.1

Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Fabio M. De Francesco]

On Saturday, August 28, 2021 11:24:52 PM CEST Phillip Potter wrote:
> Simplify c2h_evt_hdl function by removing majority of its code. The
> function always returns _FAIL anyway, due to the wrapper function it
> calls always returning _FAIL. For this reason, it is better to just
> return _FAIL directly.
> 
> Leave the call to c2h_evt_read in place, as without it, event handling
> semantics of the driver would be changed, despite nothing actually being
> done with the event.
> 
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> ---
>  drivers/staging/r8188eu/core/rtw_cmd.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
> index ce73ac7cf973..b520c6b43c03 100644
> --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> @@ -1854,27 +1854,12 @@ u8 rtw_c2h_wk_cmd(struct adapter *padapter, u8 *c2h_evt)
>  
>  static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
>  {
> -	s32 ret = _FAIL;
>  	u8 buf[16];
>  
> -	if (!c2h_evt) {
> -		/* No c2h event in cmd_obj, read c2h event before handling*/
> -		if (c2h_evt_read(adapter, buf) == _SUCCESS) {
> -			c2h_evt = (struct c2h_evt_hdr *)buf;

Dear Philip,

Not related to your patch, but what kind of odd assignment is it? c2h_evt takes
the address of a local variable and therefore it crashes the kernel whenever
someone decides to dereference it after this function returns and unwinds 
the stack...

> +	if (!c2h_evt)
> +		c2h_evt_read(adapter, buf);

Having said that, I strongly doubt that this path is ever taken. I didn't check the call
chain, but it may be that the function in never called or, if it is called, it always
has a valid c2h_evt argument. 

Actually I don't mean to suggest something specific. It simply looks odd, so I'd check 
and if this happens to be the case, I'd remove the whole c2h_evt_hdl().

Regards,

Fabio
>  
> -			if (filter && !filter(c2h_evt->id))
> -				goto exit;
> -
> -			ret = rtw_hal_c2h_handler(adapter, c2h_evt);
> -		}
> -	} else {
> -		if (filter && !filter(c2h_evt->id))
> -			goto exit;
> -
> -		ret = rtw_hal_c2h_handler(adapter, c2h_evt);
> -	}
> -exit:
> -	return ret;
> +	return _FAIL;
>  }
>  
>  static void c2h_wk_callback(struct work_struct *work)
> -- 
> 2.31.1
> 
> 

Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Phillip Potter]

On Sun, 29 Aug 2021 at 09:52, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> >  static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
> >  {
> > -     s32 ret = _FAIL;
> >       u8 buf[16];
> >
> > -     if (!c2h_evt) {
> > -             /* No c2h event in cmd_obj, read c2h event before handling*/
> > -             if (c2h_evt_read(adapter, buf) == _SUCCESS) {
> > -                     c2h_evt = (struct c2h_evt_hdr *)buf;
>
> Dear Philip,
>
> Not related to your patch, but what kind of odd assignment is it? c2h_evt takes
> the address of a local variable and therefore it crashes the kernel whenever
> someone decides to dereference it after this function returns and unwinds
> the stack...

Dear Fabio,

Thank you for taking a look firstly, really appreciate it :-) As for the line:
c2h_evt = (struct c2h_evt_hdr *)buf;

in the original code before I removed it, bear in mind that this
pointer assignment is
happening into the parameter variable c2h_evt, which is a copy of the
passed in argument,
as C is pass-by-value. Therefore, after the c2h_evt_hdl function
returns, the value passed
in as the argument for this parameter would still have its original
pointer value (or NULL).

It would not therefore be possible to deference the pointer to this
stack-allocated memory
from outside the function, even in the original code. I agree though,
its purpose is dubious.
Originally, the wrapper function rtw_hal_c2h_handler would have passed
it through to the
function assigned to the c2h_handler function pointer, but there was
no such function in
this driver, so it was never executed.

>
> > +     if (!c2h_evt)
> > +             c2h_evt_read(adapter, buf);
>
> Having said that, I strongly doubt that this path is ever taken. I didn't check the call
> chain, but it may be that the function in never called or, if it is called, it always
> has a valid c2h_evt argument.
>
> Actually I don't mean to suggest something specific. It simply looks odd, so I'd check
> and if this happens to be the case, I'd remove the whole c2h_evt_hdl().
>
> Regards,
>
> Fabio

As alluded to, removing the whole of c2h_evt_hdl would lead to
c2h_evt_read no longer
being executed, which would mean the reads from the adapter register
don't happen, and
nor does the clearing by c2h_evt_clear(adapter); - in particular, the
comment there mentions
the FW not updating the next command message if this isn't executed when needed.

This may be perfectly fine, but I thought this approach is safer due
to the above.

Regards,
Phil

Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Phillip Potter]

On Sun, 29 Aug 2021 at 09:52, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> >  static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
> >  {
> > -     s32 ret = _FAIL;
> >       u8 buf[16];
> >
> > -     if (!c2h_evt) {
> > -             /* No c2h event in cmd_obj, read c2h event before handling*/
> > -             if (c2h_evt_read(adapter, buf) == _SUCCESS) {
> > -                     c2h_evt = (struct c2h_evt_hdr *)buf;
>
> Dear Philip,
>
> Not related to your patch, but what kind of odd assignment is it? c2h_evt takes
> the address of a local variable and therefore it crashes the kernel whenever
> someone decides to dereference it after this function returns and unwinds
> the stack...

Dear Fabio,

Thank you for taking a look firstly, really appreciate it :-) As for the line:
c2h_evt = (struct c2h_evt_hdr *)buf;

in the original code before I removed it, bear in mind that this
pointer assignment is
happening into the parameter variable c2h_evt, which is a copy of the
passed in argument,
as C is pass-by-value. Therefore, after the c2h_evt_hdl function
returns, the value passed
in as the argument for this parameter would still have its original
pointer value (or NULL).

It would not therefore be possible to deference the pointer to this
stack-allocated memory
from outside the function, even in the original code. I agree though,
its purpose is dubious.
Originally, the wrapper function rtw_hal_c2h_handler would have passed
it through to the
function assigned to the c2h_handler function pointer, but there was
no such function in
this driver, so it was never executed.

>
> > +     if (!c2h_evt)
> > +             c2h_evt_read(adapter, buf);
>
> Having said that, I strongly doubt that this path is ever taken. I didn't check the call
> chain, but it may be that the function in never called or, if it is called, it always
> has a valid c2h_evt argument.
>
> Actually I don't mean to suggest something specific. It simply looks odd, so I'd check
> and if this happens to be the case, I'd remove the whole c2h_evt_hdl().
>
> Regards,
>
> Fabio

As alluded to, removing the whole of c2h_evt_hdl would lead to
c2h_evt_read no longer
being executed, which would mean the reads from the adapter register
don't happen, and
nor does the clearing by c2h_evt_clear(adapter); - in particular, the
comment there mentions
the FW not updating the next command message if this isn't executed when needed.

This may be perfectly fine, but I thought this approach is safer due
to the above.

Regards,
Phil

Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Fabio M. De Francesco]

On Sunday, August 29, 2021 12:49:42 PM CEST Phillip Potter wrote:
> On Sun, 29 Aug 2021 at 09:52, Fabio M. De Francesco
> <fmdefrancesco@gmail.com> wrote:
> >
> > >  static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
> > >  {
> > > -     s32 ret = _FAIL;
> > >       u8 buf[16];
> > >
> > > -     if (!c2h_evt) {
> > > -             /* No c2h event in cmd_obj, read c2h event before handling*/
> > > -             if (c2h_evt_read(adapter, buf) == _SUCCESS) {
> > > -                     c2h_evt = (struct c2h_evt_hdr *)buf;
> >
> > Dear Philip,
> >
> > Not related to your patch, but what kind of odd assignment is it? c2h_evt takes
> > the address of a local variable and therefore it crashes the kernel whenever
> > someone decides to dereference it after this function returns and unwinds
> > the stack...
> 
> Dear Fabio,
> 
> Thank you for taking a look firstly, really appreciate it :-) As for the line:
> c2h_evt = (struct c2h_evt_hdr *)buf;
> 
> in the original code before I removed it, bear in mind that this
> pointer assignment is
> happening into the parameter variable c2h_evt, which is a copy of the
> passed in argument,
> as C is pass-by-value. 

You're right! Sorry. For sure I was still sleeping when I wrote that message. 
The assignment is to the argument itself, not to the storage 
location it points to... for a moment I forgot how arguments are passed
(by value) and how pointers work :(

> Therefore, after the c2h_evt_hdl function
> returns, the value passed
> in as the argument for this parameter would still have its original
> pointer value (or NULL).
> 
> It would not therefore be possible to deference the pointer to this
> stack-allocated memory
> from outside the function, even in the original code. I agree though,
> its purpose is dubious.
> Originally, the wrapper function rtw_hal_c2h_handler would have passed
> it through to the
> function assigned to the c2h_handler function pointer, but there was
> no such function in
> this driver, so it was never executed.
> 
> >
> > > +     if (!c2h_evt)
> > > +             c2h_evt_read(adapter, buf);
> >
> > Having said that, I strongly doubt that this path is ever taken. I didn't check the call
> > chain, but it may be that the function in never called or, if it is called, it always
> > has a valid c2h_evt argument.
> >
> > Actually I don't mean to suggest something specific. It simply looks odd, so I'd check
> > and if this happens to be the case, I'd remove the whole c2h_evt_hdl().
> >
> > Regards,
> >
> > Fabio
> 
> As alluded to, removing the whole of c2h_evt_hdl would lead to
> c2h_evt_read no longer
> being executed, 

Yes, correct. I see that the only caller of c2h_evt_hdl() is at line 1971 of  core/rtw_cmd.c:
"c2h_evt_hdl(padapter, (struct c2h_evt_hdr *)pdrvextra_cmd->pbuf, NULL);".
Actually I cannot say whether or not pdrvextra_cmd->pbuf is properly initialized at
that specific point.

Maybe that staying on the safe side and checking for !c2h_evt is the most reasonable
thing to do...

Some time ago, Greg wrote that even reads can have side effects on some (broken?) 
hardware. So, you're on the safe side if you leave that read where it is. Well done. 

> which would mean the reads from the adapter register
> don't happen, and
> nor does the clearing by c2h_evt_clear(adapter); - in particular, the
> comment there mentions
> the FW not updating the next command message if this isn't executed when needed.
> 
> This may be perfectly fine, but I thought this approach is safer due
> to the above.

Yes, I must agree with you :)

Thanks for making me notice that detail about arguments. Maybe that that odd 
assignment made me forget for a moment how passing arguments works in C.

Thanks,

Fabio

> Regards,
> Phil
> 

Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Pavel Skripkin]

On 8/29/21 12:24 AM, Phillip Potter wrote:
> Simplify c2h_evt_hdl function by removing majority of its code. The
> function always returns _FAIL anyway, due to the wrapper function it
> calls always returning _FAIL. For this reason, it is better to just
> return _FAIL directly.
> 
> Leave the call to c2h_evt_read in place, as without it, event handling
> semantics of the driver would be changed, despite nothing actually being
> done with the event.
> 
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> ---
>   drivers/staging/r8188eu/core/rtw_cmd.c | 21 +++------------------
>   1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
> index ce73ac7cf973..b520c6b43c03 100644
> --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> @@ -1854,27 +1854,12 @@ u8 rtw_c2h_wk_cmd(struct adapter *padapter, u8 *c2h_evt)
>   
>   static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
>   {
> -	s32 ret = _FAIL;
>   	u8 buf[16];
>   
> -	if (!c2h_evt) {
> -		/* No c2h event in cmd_obj, read c2h event before handling*/
> -		if (c2h_evt_read(adapter, buf) == _SUCCESS) {
> -			c2h_evt = (struct c2h_evt_hdr *)buf;
> +	if (!c2h_evt)
> +		c2h_evt_read(adapter, buf);
>   
> -			if (filter && !filter(c2h_evt->id))
> -				goto exit;
> -
> -			ret = rtw_hal_c2h_handler(adapter, c2h_evt);
> -		}
> -	} else {
> -		if (filter && !filter(c2h_evt->id))
> -			goto exit;
> -
> -		ret = rtw_hal_c2h_handler(adapter, c2h_evt);
> -	}
> -exit:
> -	return ret;
> +	return _FAIL;


Hi, Phillip!


Do we really need to return _FAIL every time? The only one caller of 
c2h_evt_hdl() does not rely on it's return value. Shouldn't we make this 
function return void to simplify the code?


BTW, this function does nothing now, as I understand. It reads to local 
buffer and returns. I think, it can be removed



>   }
>   
>   static void c2h_wk_callback(struct work_struct *work)
> 




With regards,
Pavel Skripkin

Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Phillip Potter]

On Sun, Aug 29, 2021 at 02:54:14PM +0300, Pavel Skripkin wrote:
> On 8/29/21 12:24 AM, Phillip Potter wrote:
> > Simplify c2h_evt_hdl function by removing majority of its code. The
> > function always returns _FAIL anyway, due to the wrapper function it
> > calls always returning _FAIL. For this reason, it is better to just
> > return _FAIL directly.
> > 
> > Leave the call to c2h_evt_read in place, as without it, event handling
> > semantics of the driver would be changed, despite nothing actually being
> > done with the event.
> > 
> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> > ---
> >   drivers/staging/r8188eu/core/rtw_cmd.c | 21 +++------------------
> >   1 file changed, 3 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
> > index ce73ac7cf973..b520c6b43c03 100644
> > --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> > +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> > @@ -1854,27 +1854,12 @@ u8 rtw_c2h_wk_cmd(struct adapter *padapter, u8 *c2h_evt)
> >   static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
> >   {
> > -	s32 ret = _FAIL;
> >   	u8 buf[16];
> > -	if (!c2h_evt) {
> > -		/* No c2h event in cmd_obj, read c2h event before handling*/
> > -		if (c2h_evt_read(adapter, buf) == _SUCCESS) {
> > -			c2h_evt = (struct c2h_evt_hdr *)buf;
> > +	if (!c2h_evt)
> > +		c2h_evt_read(adapter, buf);
> > -			if (filter && !filter(c2h_evt->id))
> > -				goto exit;
> > -
> > -			ret = rtw_hal_c2h_handler(adapter, c2h_evt);
> > -		}
> > -	} else {
> > -		if (filter && !filter(c2h_evt->id))
> > -			goto exit;
> > -
> > -		ret = rtw_hal_c2h_handler(adapter, c2h_evt);
> > -	}
> > -exit:
> > -	return ret;
> > +	return _FAIL;
> 
> 
> Hi, Phillip!
> 
> 
> Do we really need to return _FAIL every time? The only one caller of
> c2h_evt_hdl() does not rely on it's return value. Shouldn't we make this
> function return void to simplify the code?

Dear Pavel,

Thanks for the review. Good point on the return type, I'll publish a v2
series.

> 
> 
> BTW, this function does nothing now, as I understand. It reads to local
> buffer and returns. I think, it can be removed
> 
> 
> 
> >   }
> >   static void c2h_wk_callback(struct work_struct *work)
> > 

Not sure if you're referring to c2h_wk_callback or c2h_evt_hdl, but
either way, they both call (indirectly or otherwise) c2h_evt_read and
c2h_evt_clear as well as rtw_c2h_wk_cmd in the case of c2h_wk_callback.
To just delete them wholesale therefore would be unsafe I think, due to
the effect on event semantics.

Certainly, it is possible to handle this c2h stuff by just reading from
register and ignoring though - another series is a better place for that
though I think. Admittedly, I may be talking nonsense here :-)

Regards,
Phil

Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Pavel Skripkin]

On 8/30/21 2:18 AM, Phillip Potter wrote:
> On Sun, Aug 29, 2021 at 02:54:14PM +0300, Pavel Skripkin wrote:
>> On 8/29/21 12:24 AM, Phillip Potter wrote:
>> > Simplify c2h_evt_hdl function by removing majority of its code. The
>> > function always returns _FAIL anyway, due to the wrapper function it
>> > calls always returning _FAIL. For this reason, it is better to just
>> > return _FAIL directly.
>> > 
>> > Leave the call to c2h_evt_read in place, as without it, event handling
>> > semantics of the driver would be changed, despite nothing actually being
>> > done with the event.
>> > 
>> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
>> > ---
>> >   drivers/staging/r8188eu/core/rtw_cmd.c | 21 +++------------------
>> >   1 file changed, 3 insertions(+), 18 deletions(-)
>> > 
>> > diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
>> > index ce73ac7cf973..b520c6b43c03 100644
>> > --- a/drivers/staging/r8188eu/core/rtw_cmd.c
>> > +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
>> > @@ -1854,27 +1854,12 @@ u8 rtw_c2h_wk_cmd(struct adapter *padapter, u8 *c2h_evt)
>> >   static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
>> >   {
>> > -	s32 ret = _FAIL;
>> >   	u8 buf[16];
>> > -	if (!c2h_evt) {
>> > -		/* No c2h event in cmd_obj, read c2h event before handling*/
>> > -		if (c2h_evt_read(adapter, buf) == _SUCCESS) {
>> > -			c2h_evt = (struct c2h_evt_hdr *)buf;
>> > +	if (!c2h_evt)
>> > +		c2h_evt_read(adapter, buf);
>> > -			if (filter && !filter(c2h_evt->id))
>> > -				goto exit;
>> > -
>> > -			ret = rtw_hal_c2h_handler(adapter, c2h_evt);
>> > -		}
>> > -	} else {
>> > -		if (filter && !filter(c2h_evt->id))
>> > -			goto exit;
>> > -
>> > -		ret = rtw_hal_c2h_handler(adapter, c2h_evt);
>> > -	}
>> > -exit:
>> > -	return ret;
>> > +	return _FAIL;
>> 
>> 
>> Hi, Phillip!
>> 
>> 
>> Do we really need to return _FAIL every time? The only one caller of
>> c2h_evt_hdl() does not rely on it's return value. Shouldn't we make this
>> function return void to simplify the code?
> 
> Dear Pavel,
> 
> Thanks for the review. Good point on the return type, I'll publish a v2
> series.
> 
>> 
>> 
>> BTW, this function does nothing now, as I understand. It reads to local
>> buffer and returns. I think, it can be removed
>> 
>> 
>> 
>> >   }
>> >   static void c2h_wk_callback(struct work_struct *work)
>> > 
> 
> Not sure if you're referring to c2h_wk_callback or c2h_evt_hdl, but
> either way, they both call (indirectly or otherwise) c2h_evt_read and
> c2h_evt_clear as well as rtw_c2h_wk_cmd in the case of c2h_wk_callback.
> To just delete them wholesale therefore would be unsafe I think, due to
> the effect on event semantics.
> 
> Certainly, it is possible to handle this c2h stuff by just reading from
> register and ignoring though - another series is a better place for that
> though I think. Admittedly, I may be talking nonsense here :-)
> 

I was referring to c2h_evt_hdl. I missed, that c2h_evt_read() writes to 
register, sorry :)





With regards,
Pavel Skripkin

[PATCH 3/3] staging: r8188eu: remove rtw_hal_c2h_handler function

[Phillip Potter]

Remove rtw_hal_c2h_handler function from hal/hal_intf.c, as well as its
declaration in include/hal_intf.h, and remove its one remaining caller
within core/rtw_cmd.c.

This function was a wrapper function, then simplified to always return
_FAIL. Since it has no further use, remove it, as part of ongoing
efforts to simplify and remove the HAL layer of the driver.

Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 drivers/staging/r8188eu/core/rtw_cmd.c     | 2 --
 drivers/staging/r8188eu/hal/hal_intf.c     | 5 -----
 drivers/staging/r8188eu/include/hal_intf.h | 2 --
 3 files changed, 9 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index b520c6b43c03..5222863f8d66 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -1898,8 +1898,6 @@ static void c2h_wk_callback(struct work_struct *work)
 		}
 
 		if (ccx_id_filter(c2h_evt->id)) {
-			/* Handle CCX report here */
-			rtw_hal_c2h_handler(adapter, c2h_evt);
 			kfree(c2h_evt);
 		} else {
 #ifdef CONFIG_88EU_P2P
diff --git a/drivers/staging/r8188eu/hal/hal_intf.c b/drivers/staging/r8188eu/hal/hal_intf.c
index 0c835f9cd181..bcc77da06c08 100644
--- a/drivers/staging/r8188eu/hal/hal_intf.c
+++ b/drivers/staging/r8188eu/hal/hal_intf.c
@@ -426,11 +426,6 @@ void rtw_hal_reset_security_engine(struct adapter *adapter)
 		adapter->HalFunc.hal_reset_security_engine(adapter);
 }
 
-s32 rtw_hal_c2h_handler(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt)
-{
-	return _FAIL;
-}
-
 c2h_id_filter rtw_hal_c2h_id_filter_ccx(struct adapter *adapter)
 {
 	return adapter->HalFunc.c2h_id_filter_ccx;
diff --git a/drivers/staging/r8188eu/include/hal_intf.h b/drivers/staging/r8188eu/include/hal_intf.h
index 4603f9212030..954de3ab2613 100644
--- a/drivers/staging/r8188eu/include/hal_intf.h
+++ b/drivers/staging/r8188eu/include/hal_intf.h
@@ -400,8 +400,6 @@ int rtw_hal_iol_cmd(struct adapter  *adapter, struct xmit_frame *xmit_frame,
 void rtw_hal_notch_filter(struct adapter *adapter, bool enable);
 void rtw_hal_reset_security_engine(struct adapter *adapter);
 
-s32 rtw_hal_c2h_handler(struct adapter *adapter,
-			struct c2h_evt_hdr *c2h_evt);
 c2h_id_filter rtw_hal_c2h_id_filter_ccx(struct adapter *adapter);
 void indicate_wx_scan_complete_event(struct adapter *padapter);
 u8 rtw_do_join(struct adapter *padapter);
-- 
2.31.1

Re: [PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[Fabio M. De Francesco]

On Saturday, August 28, 2021 11:24:50 PM CEST Phillip Potter wrote:
> This small patch set cleans up the c2h_handler code in the HAL layer
> of the driver. In r8188eu, this field of struct hal_ops, is not even
> used, so dependent code has always returned _FAIL. For this reason, we
> should remove this function pointer field, and the wrapper function
> which checks it. This is done in stages by this set, and helps get
> the driver closer to the pointer where the HAL layer is
> deleted/integrated as necessary and no longer a separate entity.
> 
> Phillip Potter (3):
>   staging: r8188eu: remove c2h_handler field from struct hal_ops
>   staging: r8188eu: simplify c2h_evt_hdl function
>   staging: r8188eu: remove rtw_hal_c2h_handler function
> 
>  drivers/staging/r8188eu/core/rtw_cmd.c     | 23 +++-------------------
>  drivers/staging/r8188eu/hal/hal_intf.c     |  9 ---------
>  drivers/staging/r8188eu/include/hal_intf.h |  4 ----
>  3 files changed, 3 insertions(+), 33 deletions(-)
> 
> -- 
> 2.31.1
> 
Dear Philip,

You work looks good (especially after having clarified a couple of minor doubts 
I had expressed in another message). So, the entire series is...

Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Regards,

Fabio

Re: [PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[Phillip Potter]

On Sun, 29 Aug 2021 at 13:48, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On Saturday, August 28, 2021 11:24:50 PM CEST Phillip Potter wrote:
> > This small patch set cleans up the c2h_handler code in the HAL layer
> > of the driver. In r8188eu, this field of struct hal_ops, is not even
> > used, so dependent code has always returned _FAIL. For this reason, we
> > should remove this function pointer field, and the wrapper function
> > which checks it. This is done in stages by this set, and helps get
> > the driver closer to the pointer where the HAL layer is
> > deleted/integrated as necessary and no longer a separate entity.
> >
> > Phillip Potter (3):
> >   staging: r8188eu: remove c2h_handler field from struct hal_ops
> >   staging: r8188eu: simplify c2h_evt_hdl function
> >   staging: r8188eu: remove rtw_hal_c2h_handler function
> >
> >  drivers/staging/r8188eu/core/rtw_cmd.c     | 23 +++-------------------
> >  drivers/staging/r8188eu/hal/hal_intf.c     |  9 ---------
> >  drivers/staging/r8188eu/include/hal_intf.h |  4 ----
> >  3 files changed, 3 insertions(+), 33 deletions(-)
> >
> > --
> > 2.31.1
> >
> Dear Philip,
>
> You work looks good (especially after having clarified a couple of minor doubts
> I had expressed in another message). So, the entire series is...
>
> Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>
> Regards,
>
> Fabio
>

Dear Fabio,

Thanks very much for the review and Acked-by, much appreciated.

Regards,
Phil

Re: [PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[Phillip Potter]

On Sun, 29 Aug 2021 at 13:48, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On Saturday, August 28, 2021 11:24:50 PM CEST Phillip Potter wrote:
> > This small patch set cleans up the c2h_handler code in the HAL layer
> > of the driver. In r8188eu, this field of struct hal_ops, is not even
> > used, so dependent code has always returned _FAIL. For this reason, we
> > should remove this function pointer field, and the wrapper function
> > which checks it. This is done in stages by this set, and helps get
> > the driver closer to the pointer where the HAL layer is
> > deleted/integrated as necessary and no longer a separate entity.
> >
> > Phillip Potter (3):
> >   staging: r8188eu: remove c2h_handler field from struct hal_ops
> >   staging: r8188eu: simplify c2h_evt_hdl function
> >   staging: r8188eu: remove rtw_hal_c2h_handler function
> >
> >  drivers/staging/r8188eu/core/rtw_cmd.c     | 23 +++-------------------
> >  drivers/staging/r8188eu/hal/hal_intf.c     |  9 ---------
> >  drivers/staging/r8188eu/include/hal_intf.h |  4 ----
> >  3 files changed, 3 insertions(+), 33 deletions(-)
> >
> > --
> > 2.31.1
> >
> Dear Philip,
>
> You work looks good (especially after having clarified a couple of minor doubts
> I had expressed in another message). So, the entire series is...
>
> Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>
> Regards,
>
> Fabio
>

Dear Fabio,

Thanks very much for the review and Acked-by, much appreciated.

Regards,
Phil

Re: [PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[Michael Straube]

On 8/28/21 23:24, Phillip Potter wrote:
> This small patch set cleans up the c2h_handler code in the HAL layer
> of the driver. In r8188eu, this field of struct hal_ops, is not even
> used, so dependent code has always returned _FAIL. For this reason, we
> should remove this function pointer field, and the wrapper function
> which checks it. This is done in stages by this set, and helps get
> the driver closer to the pointer where the HAL layer is
> deleted/integrated as necessary and no longer a separate entity.
> 
> Phillip Potter (3):
>    staging: r8188eu: remove c2h_handler field from struct hal_ops
>    staging: r8188eu: simplify c2h_evt_hdl function
>    staging: r8188eu: remove rtw_hal_c2h_handler function
> 
>   drivers/staging/r8188eu/core/rtw_cmd.c     | 23 +++-------------------
>   drivers/staging/r8188eu/hal/hal_intf.c     |  9 ---------
>   drivers/staging/r8188eu/include/hal_intf.h |  4 ----
>   3 files changed, 3 insertions(+), 33 deletions(-)
> 

Thanks for your patches Phillip. Looks good to me and I've built and
runtime tested this series with Inter-Tech DMG-02, so...

Acked-by: Michael Straube <straube.linux@gmail.com>

Thanks,
Michael

Re: [PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[Phillip Potter]

On Sun, 29 Aug 2021 at 16:04, Michael Straube <straube.linux@gmail.com> wrote:
>
> On 8/28/21 23:24, Phillip Potter wrote:
> > This small patch set cleans up the c2h_handler code in the HAL layer
> > of the driver. In r8188eu, this field of struct hal_ops, is not even
> > used, so dependent code has always returned _FAIL. For this reason, we
> > should remove this function pointer field, and the wrapper function
> > which checks it. This is done in stages by this set, and helps get
> > the driver closer to the pointer where the HAL layer is
> > deleted/integrated as necessary and no longer a separate entity.
> >
> > Phillip Potter (3):
> >    staging: r8188eu: remove c2h_handler field from struct hal_ops
> >    staging: r8188eu: simplify c2h_evt_hdl function
> >    staging: r8188eu: remove rtw_hal_c2h_handler function
> >
> >   drivers/staging/r8188eu/core/rtw_cmd.c     | 23 +++-------------------
> >   drivers/staging/r8188eu/hal/hal_intf.c     |  9 ---------
> >   drivers/staging/r8188eu/include/hal_intf.h |  4 ----
> >   3 files changed, 3 insertions(+), 33 deletions(-)
> >
>
> Thanks for your patches Phillip. Looks good to me and I've built and
> runtime tested this series with Inter-Tech DMG-02, so...
>
> Acked-by: Michael Straube <straube.linux@gmail.com>
>
> Thanks,
> Michael

Thanks very much Michael, appreciate it.

Regards,
Phil

Re: [PATCH 0/3] staging: r8188eu: cleanup c2h_handler code

[Phillip Potter]

On Sun, 29 Aug 2021 at 16:04, Michael Straube <straube.linux@gmail.com> wrote:
>
> On 8/28/21 23:24, Phillip Potter wrote:
> > This small patch set cleans up the c2h_handler code in the HAL layer
> > of the driver. In r8188eu, this field of struct hal_ops, is not even
> > used, so dependent code has always returned _FAIL. For this reason, we
> > should remove this function pointer field, and the wrapper function
> > which checks it. This is done in stages by this set, and helps get
> > the driver closer to the pointer where the HAL layer is
> > deleted/integrated as necessary and no longer a separate entity.
> >
> > Phillip Potter (3):
> >    staging: r8188eu: remove c2h_handler field from struct hal_ops
> >    staging: r8188eu: simplify c2h_evt_hdl function
> >    staging: r8188eu: remove rtw_hal_c2h_handler function
> >
> >   drivers/staging/r8188eu/core/rtw_cmd.c     | 23 +++-------------------
> >   drivers/staging/r8188eu/hal/hal_intf.c     |  9 ---------
> >   drivers/staging/r8188eu/include/hal_intf.h |  4 ----
> >   3 files changed, 3 insertions(+), 33 deletions(-)
> >
>
> Thanks for your patches Phillip. Looks good to me and I've built and
> runtime tested this series with Inter-Tech DMG-02, so...
>
> Acked-by: Michael Straube <straube.linux@gmail.com>
>
> Thanks,
> Michael

Thanks very much Michael, appreciate it.

Regards,
Phil