linux-kernel - RE: [PATCH] staging: r8188eu: remove local BIT macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ac3343ea279b4d048d77b3cf2724bcbe@AcuMS.aculab.com>
Date:   Sat, 19 Mar 2022 22:35:58 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Martin Kaiser' <martin@...ser.cx>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Michael Straube <straube.linux@...il.com>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] staging: r8188eu: remove local BIT macro

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