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

[David Laight]

From: Martin Kaiser <martin@xxxxxxxxx>
> 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)