[PATCH] staging: r8188eu: remove local BIT macro

[PATCH] staging: r8188eu: remove local BIT macro

[Martin Kaiser]

The r8188eu driver defines a local BIT(x) macro. Remove this local macro
and use the one from include/linux/bits.h.

The global BIT macro returns an unsigned long value, the removed local
BIT macro used a signed int.

DYNAMIC_BB_DYNAMIC_TXPWR is defined as BIT(2), ~DYNAMIC_BB_DYNAMIC_TXPWR
is passed to Switch_DM_Func as a u32 parameter. We need a cast in this
case as ~DYNAMIC_BB_DYNAMIC_TXPWR is a 64-bit value on x86_64 systems.

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

Dear all,

this is the next attempt to get this right. I'm not sure if it should be
labeled v3. Greg accepted all other patches of the "some rx cleanups"
series, so I'm sending this as a single patch, starting at v1 again.

Thanks to Greg and Dan for the feedback, I see that ((u32)BIT(2)) is not
different from BIT(2) without the cast. Making the cast where
DYNAMIC_BB_DYNAMIC_TXPWR is used is the better option.

It seems that (u32)(~DYNAMIC_BB_DYNAMIC_TXPWR) and
~(u32)DYNAMIC_BB_DYNAMIC_TXPWR do the same thing and it's ok to invert
first and to cast afterwards. According to the C99 standard, a 64-bit
unsigned int is cast to u32 by repeatedly adding U32_MAX + 1 until the
result fits into a u32 - or simply by cutting off the upper 32bit.

BTW the previous rtl8188eu driver used
Switch_DM_Func(padapter, (u32)(~DYNAMIC_BB_DYNAMIC_TXPWR), false);
as well.

Thanks,
Martin

 drivers/staging/r8188eu/core/rtw_wlan_util.c | 4 ++--
 drivers/staging/r8188eu/include/wifi.h       | 7 +------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
index 665b077190bc..f32401deae9a 100644
--- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
+++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
@@ -1276,13 +1276,13 @@ void update_IOT_info(struct adapter *padapter)
 		pmlmeinfo->turboMode_cts2self = 0;
 		pmlmeinfo->turboMode_rtsen = 1;
 		/* disable high power */
-		Switch_DM_Func(padapter, (~DYNAMIC_BB_DYNAMIC_TXPWR), false);
+		Switch_DM_Func(padapter, (u32)(~DYNAMIC_BB_DYNAMIC_TXPWR), false);
 		break;
 	case HT_IOT_PEER_REALTEK:
 		/* rtw_write16(padapter, 0x4cc, 0xffff); */
 		/* rtw_write16(padapter, 0x546, 0x01c0); */
 		/* disable high power */
-		Switch_DM_Func(padapter, (~DYNAMIC_BB_DYNAMIC_TXPWR), false);
+		Switch_DM_Func(padapter, (u32)(~DYNAMIC_BB_DYNAMIC_TXPWR), false);
 		break;
 	default:
 		pmlmeinfo->turboMode_cts2self = 0;
diff --git a/drivers/staging/r8188eu/include/wifi.h b/drivers/staging/r8188eu/include/wifi.h
index c331be19ff83..299553351246 100644
--- a/drivers/staging/r8188eu/include/wifi.h
+++ b/drivers/staging/r8188eu/include/wifi.h
@@ -4,14 +4,9 @@
 #ifndef _WIFI_H_
 #define _WIFI_H_
 
+#include <linux/bits.h>
 #include <linux/ieee80211.h>
 
-#ifdef BIT
-/* error	"BIT define occurred earlier elsewhere!\n" */
-#undef BIT
-#endif
-#define BIT(x)	(1 << (x))
-
 #define WLAN_ETHHDR_LEN		14
 #define WLAN_HDR_A3_LEN		24
 #define WLAN_HDR_A3_QOS_LEN	26
-- 
2.30.2

RE: [PATCH] staging: r8188eu: remove local BIT macro

[David Laight]

From: Martin Kaiser <martin@kaiser.cx>
> Sent: 19 March 2022 18:04
> 
> The r8188eu driver defines a local BIT(x) macro. Remove this local macro
> and use the one from include/linux/bits.h.
> 
> The global BIT macro returns an unsigned long value, the removed local
> BIT macro used a signed int.
> 
> DYNAMIC_BB_DYNAMIC_TXPWR is defined as BIT(2), ~DYNAMIC_BB_DYNAMIC_TXPWR
> is passed to Switch_DM_Func as a u32 parameter. We need a cast in this
> case as ~DYNAMIC_BB_DYNAMIC_TXPWR is a 64-bit value on x86_64 systems.

Hmmm....
Why not fix the called function so that the caller doesn't
need to do the invert.

...
> b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> index 665b077190bc..f32401deae9a 100644
> --- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
> +++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> @@ -1276,13 +1276,13 @@ void update_IOT_info(struct adapter *padapter)
>  		pmlmeinfo->turboMode_cts2self = 0;
>  		pmlmeinfo->turboMode_rtsen = 1;
>  		/* disable high power */
> -		Switch_DM_Func(padapter, (~DYNAMIC_BB_DYNAMIC_TXPWR), false);
> +		Switch_DM_Func(padapter, (u32)(~DYNAMIC_BB_DYNAMIC_TXPWR), false);

The function is defined as a real function:
Even though all the callers either pass 'true' or 'false' for enable.

void Switch_DM_Func(struct adapter *padapter, u32 mode, u8 enable)
{
	if (enable)
		SetHwReg8188EU(padapter, HW_VAR_DM_FUNC_SET, (u8 *)(&mode));
	else
		SetHwReg8188EU(padapter, HW_VAR_DM_FUNC_CLR, (u8 *)(&mode));
}

That (u8 *)&mode cast is at best dubious.

Searching for the callers also gives:
	Switch_DM_Func(padapter, DYNAMIC_FUNC_DISABLE, false)

Should that have an invert?
Or is the other call wrong?
They don't both look right.
Or is DYNAMIC_FUNC_DISABLE just zero?

SetHwReg8188EU() is basically a big switch statement on the
'probably mostly constant' second argument.
The two relevant switch cases are:

	case HW_VAR_DM_FUNC_SET:
		if (*((u32 *)val) == DYNAMIC_ALL_FUNC_ENABLE) {
			podmpriv->SupportAbility =	pdmpriv->InitODMFlag;
		} else {
			podmpriv->SupportAbility |= *((u32 *)val);
		}
		break;
	case HW_VAR_DM_FUNC_CLR:
		podmpriv->SupportAbility &= *((u32 *)val);
		break;

So the ~ should probably be moved to the final statement.

OTOH this code is a big pile of poo.
Abstraction functions gone mad.

If you have a function that does two different things based on
a parameter that is always a constant you really should have
two different functions.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: r8188eu: remove local BIT macro

[Dan Carpenter]

This version is better, thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

What the GCC devs should have done is run their checker on real code and
silence the common false positives.  There are a few ways to do this.
You could make ~ a special case.  They already ignore truncated sign
extension so they could equally well ignore truncation when all it's a
32 bits which are all set to 1.  Probably the best answer is to do both.

Sure, you might miss some bugs that way, but anyone relying on GCC to
find *all* their bugs is a moron.

regards,
dan carpenter

RE: [PATCH] staging: r8188eu: remove local BIT macro

[David Laight]

From: Dan Carpenter
> Sent: 21 March 2022 08:16
> 
> This version is better, thanks!
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> What the GCC devs should have done is run their checker on real code and
> silence the common false positives.  There are a few ways to do this.
> You could make ~ a special case.  They already ignore truncated sign
> extension so they could equally well ignore truncation when all it's a
> 32 bits which are all set to 1.  Probably the best answer is to do both.

Or start using -(BIT_VALUE + 1) instead of ~BIT_VALUE :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: r8188eu: remove local BIT macro

[Dan Carpenter]

On Sat, Mar 19, 2022 at 10:35:58PM +0000, David Laight wrote:
> OTOH this code is a big pile of poo.
> Abstraction functions gone mad.

Yeah.

I wrote an email similar to yours and I even wrote some sample code.
But then I deleted it because I don't pay Martin anything so I'm just
grateful for what he sends and can't reasonably ask for more.

This code constantly amazes me with how many abstractions there are.
Martin keeps deleting them, and I think he's going to run out but so
far that hasn't happened.

Anyway here is the diff just for laughs since you brought it up.  Not
something that's necessarry and definitely not a priority.

I don't really like enable/disable functions that do opposite things
depending on if true/false is passed as a parameter.  They're normally
more readable split apart.

Ideally there would be adapter_to_pdbm() and adapter_to_podm() helper
functions.

diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c b/drivers/staging/r8188eu/core/rtw_wlan_util.c
index 665b077190bc..d973cf341031 100644
--- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
+++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
@@ -276,12 +276,23 @@ void Restore_DM_Func_Flag(struct adapter *padapter)
 	SetHwReg8188EU(padapter, HW_VAR_DM_FUNC_OP, (u8 *)(&saveflag));
 }
 
+void enable_dm_func(struct adapter *padapter, u32 mode)
+ {
+	struct dm_pri *pdmpriv = adapter_to_pdbm(padapter);
+	struct odm_dm_struct *podmpriv = adapter_to_pod(padapter);
+
+	if (mode == DYNAMIC_ALL_FUNC_ENABLE) {
+		podmpriv->SupportAbility = pdmpriv->InitODMFlag;
+	} else {
+		podmpriv->SupportAbility |= mode;
+	}
+}
+
+void disable_dm_func(struct adapter *padapter, u32 mode)
+{
+	struct odm_dm_struct *podmpriv = adapter_to_pod(padapter);
+
+	podmpriv->SupportAbility &= mode;
+ }

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: remove local BIT macro

[Dan Carpenter]

On Mon, Mar 21, 2022 at 09:04:46AM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 21 March 2022 08:16
> > 
> > This version is better, thanks!
> > 
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > What the GCC devs should have done is run their checker on real code and
> > silence the common false positives.  There are a few ways to do this.
> > You could make ~ a special case.  They already ignore truncated sign
> > extension so they could equally well ignore truncation when all it's a
> > 32 bits which are all set to 1.  Probably the best answer is to do both.
> 
> Or start using -(BIT_VALUE + 1) instead of ~BIT_VALUE :-)
> 

You are a bad person.  :P

regards,
dan carpenter

RE: [PATCH] staging: r8188eu: remove local BIT macro

[David Laight]

From: Dan Carpenter
> Sent: 21 March 2022 09:07
> 
> On Sat, Mar 19, 2022 at 10:35:58PM +0000, David Laight wrote:
> > OTOH this code is a big pile of poo.
> > Abstraction functions gone mad.
> 
> Yeah.
> 
> I wrote an email similar to yours and I even wrote some sample code.
> But then I deleted it because I don't pay Martin anything so I'm just
> grateful for what he sends and can't reasonably ask for more.
> 
> This code constantly amazes me with how many abstractions there are.
> Martin keeps deleting them, and I think he's going to run out but so
> far that hasn't happened.
> 
> Anyway here is the diff just for laughs since you brought it up.  Not
> something that's necessarry and definitely not a priority.
> 
> I don't really like enable/disable functions that do opposite things
> depending on if true/false is passed as a parameter.  They're normally
> more readable split apart.
> 
> Ideally there would be adapter_to_pdbm() and adapter_to_podm() helper
> functions.
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_wlan_util.c
> b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> index 665b077190bc..d973cf341031 100644
> --- a/drivers/staging/r8188eu/core/rtw_wlan_util.c
> +++ b/drivers/staging/r8188eu/core/rtw_wlan_util.c
> @@ -276,12 +276,23 @@ void Restore_DM_Func_Flag(struct adapter *padapter)
>  	SetHwReg8188EU(padapter, HW_VAR_DM_FUNC_OP, (u8 *)(&saveflag));
>  }
> 
> +void enable_dm_func(struct adapter *padapter, u32 mode)
> + {
> +	struct dm_pri *pdmpriv = adapter_to_pdbm(padapter);
> +	struct odm_dm_struct *podmpriv = adapter_to_pod(padapter);
> +
> +	if (mode == DYNAMIC_ALL_FUNC_ENABLE) {
> +		podmpriv->SupportAbility = pdmpriv->InitODMFlag;
> +	} else {
> +		podmpriv->SupportAbility |= mode;
> +	}
> +}
> +
> +void disable_dm_func(struct adapter *padapter, u32 mode)
> +{
> +	struct odm_dm_struct *podmpriv = adapter_to_pod(padapter);
> +
> +	podmpriv->SupportAbility &= mode;
> + }

I'd go even further.
One function for each of 'set', 'enable' and 'disable'.
Doing '=', '|=' and '&= ~'.

But then you realise it just isn't worth the effort.
Just type:
		adapter_to_pod(padapter)-> SupportAbility = ...
directly in the code.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] staging: r8188eu: remove local BIT macro

[Martin Kaiser]

Hi David,

thanks for digging into this mess.

Thus wrote David Laight (David.Laight@ACULAB.COM):

> From: Martin Kaiser <martin@kaiser.cx>
> > Sent: 19 March 2022 18:04

> > The r8188eu driver defines a local BIT(x) macro. Remove this local macro
> > and use the one from include/linux/bits.h.

> > The global BIT macro returns an unsigned long value, the removed local
> > BIT macro used a signed int.

> > DYNAMIC_BB_DYNAMIC_TXPWR is defined as BIT(2), ~DYNAMIC_BB_DYNAMIC_TXPWR
> > is passed to Switch_DM_Func as a u32 parameter. We need a cast in this
> > case as ~DYNAMIC_BB_DYNAMIC_TXPWR is a 64-bit value on x86_64 systems.

> Hmmm....
> Why not fix the called function so that the caller doesn't
> need to do the invert.

When I look into this driver, it's challenging to fix one thing at a time
and not be caught up in all the other problems I discover along the way.

Would you be ok with removing the local BIT macro first?

It seems to me that this whole DYNAMIC_BB_DYNAMIC_TXPWR handling can go.
DYNAMIC_BB_DYNAMIC_TXPWR isn't checked anywhere. I'll submit this as a
separate patch.

With DYNAMIC_BB_DYNAMIC_TXPWR gone, ony three calls remain

  16   6723  drivers/staging/r8188eu/core/rtw_mlme_ext.c <<mlmeext_joinbss_event_callback>>
             Switch_DM_Func(padapter, DYNAMIC_ALL_FUNC_ENABLE, true);
  17   7124  drivers/staging/r8188eu/core/rtw_mlme_ext.c <<createbss_hdl>>
             Switch_DM_Func(padapter, DYNAMIC_FUNC_DISABLE, false);
  18   7393  drivers/staging/r8188eu/core/rtw_mlme_ext.c <<sitesurvey_cmd_hdl>>
             Switch_DM_Func(padapter, DYNAMIC_FUNC_DISABLE, false);

DYNAMIC_FUNC_DISABLE is indeed 0 and sets podmpriv->SupportAbility = 0,
DYNAMIC_ALL_FUNC_ENABLE sets podmpriv->SupportAbility = pdmpriv->InitODMFlag.

We can do this without dubious casts and intermediate functions...

Best regards,
Martin