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)
