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: <8173b8e8846646de844de50952c5117e@AcuMS.aculab.com>
Date:   Mon, 21 Mar 2022 09:14:36 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Dan Carpenter' <dan.carpenter@...cle.com>
CC:     'Martin Kaiser' <martin@...ser.cx>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        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: 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)