Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 20 Mar 2021 15:17:48 +0100
From:   Fabio Aiuto <fabioaiuto83@gmail.com>
To:     Joe Perches <joe@perches.com>
Cc:     gregkh@linuxfoundation.org, apw@canonical.com,
        linux-kernel@vger.kernel.org
Subject: Re: CHECKPATCH: missing a warning soon after include files decl -c
Message-ID: <20210320141745.GA1611@agape.jhs>
References: <20210320105424.GA3698@agape.jhs>
 <YFXVoHvk/VesFn14@kroah.com>
 <c27ae8926ccbc0f520045fea9127811f6e8fa706.camel@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c27ae8926ccbc0f520045fea9127811f6e8fa706.camel@perches.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Sat, Mar 20, 2021 at 04:28:51AM -0700, Joe Perches wrote:
> On Sat, 2021-03-20 at 11:59 +0100, Greg KH wrote:
> > On Sat, Mar 20, 2021 at 11:54:24AM +0100, Fabio Aiuto wrote:
> > > Hi,
> > > 
> > > here's an issue in checkpatch.pl
> > > 
> > > $ perl script/checkpatch.pl -f drivers/staging/rtl8723bs/core/rtw_ap.c
> > > 
> > > I get three warning related to an extern declaration
> > > 
> > > WARNING: externs should be avoided in .c files
> > > #14: FILE: drivers/staging/rtl8723bs/core/rtw_ap.c:14:
> > > +extern unsigned char WMM_OUI[];
> > > --
> > > WARNING: externs should be avoided in .c files
> > > #15: FILE: drivers/staging/rtl8723bs/core/rtw_ap.c:15:
> > > +extern unsigned char WPS_OUI[];
> > > --
> > > WARNING: externs should be avoided in .c files
> > > #16: FILE: drivers/staging/rtl8723bs/core/rtw_ap.c:16:
> > > +extern unsigned char P2P_OUI[];
> > > ----------------------
> > > 
> > > but the file checked has 4 extern declaration:
> > > -----------------------------
> > > #define _RTW_AP_C_
> > > 
> > > #include <drv_types.h>
> > > #include <rtw_debug.h>
> > > #include <asm/unaligned.h>
> > > 
> > > extern unsigned char RTW_WPA_OUI[];
> > > extern unsigned char WMM_OUI[];
> > > extern unsigned char WPS_OUI[];
> > > extern unsigned char P2P_OUI[];
> > > 
> > > void init_mlme_ap_info(struct adapter *padapter)
> > > -------------------------------
> > > 
> > > If I add a ';' this way:
> > > ----------------------------
> > > #define _RTW_AP_C_
> > > 
> > > #include <drv_types.h>
> > > #include <rtw_debug.h>
> > > #include <asm/unaligned.h>
> > > ;
> > > extern unsigned char RTW_WPA_OUI[];
> > > extern unsigned char WMM_OUI[];
> > > extern unsigned char WPS_OUI[];
> > > extern unsigned char P2P_OUI[];
> > > 
> > > void init_mlme_ap_info(struct adapter *padapter)
> > > --------------------------------
> > 
> > Wait, why would you do the above?
> 
> It is rather an ugly hack.

yes it is, it was just a local and temporary one to verify that checkpatch.pl
recognizes a statement even in multiple lines, until the next ';'. In my case it has his own
drawbacks, but I will follow Greg's advice, to let checkpatch.pl doing his heavy duty:) 

> 
> > Don't try to trick a perl script that has a hard time parsing C files,
> > try to resolve the original issue here.
> > 
> > And that is that the above definitions should be in a .h file somewhere.
> > If you make that move then all should be fine.
> 
> Actually, these would seem to be better as one or multiple functions with
> local statics or even as static inlines functions in the .h file
> 
> $ git grep -w RTW_WPA_OUI drivers/staging/rtl8723bs/core
> drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char RTW_WPA_OUI[];
> drivers/staging/rtl8723bs/core/rtw_ap.c:        if (!memcmp(RTW_WPA_OUI, oui, 4))
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char RTW_WPA_OUI[] = {0x00, 0x50, 0xf2, 0x01};
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:                  if ((!memcmp(pIE->data, RTW_WPA_OUI, 4)) ||
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c:extern unsigned char RTW_WPA_OUI[];
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c:                         if ((!memcmp(pIE->data, RTW_WPA_OUI, 4)) && (!memcmp((pIE->data + 12), WPA_TKIP_CIPHER, 4)))
> 
> $ git grep -w WMM_OUI drivers/staging/rtl8723bs/core
> drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char WMM_OUI[];
> drivers/staging/rtl8723bs/core/rtw_ap.c:        else if (!memcmp(WMM_OUI, oui, 4))
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char WMM_OUI[] = {0x00, 0x50, 0xf2, 0x02};
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:                                  (!memcmp(pIE->data, WMM_OUI, 4)) ||
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:                  if (!memcmp(pIE->data, WMM_OUI, 4))
> drivers/staging/rtl8723bs/include/rtw_mlme_ext.h:extern unsigned char WMM_OUI[];
> 
> $ git grep -w WPS_OUI drivers/staging/rtl8723bs/core
> drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char WPS_OUI[];
> drivers/staging/rtl8723bs/core/rtw_ap.c:        else if (!memcmp(WPS_OUI, oui, 4))
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char WPS_OUI[] = {0x00, 0x50, 0xf2, 0x04};
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:                                  (!memcmp(pIE->data, WPS_OUI, 4))) {
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:                          if ((!padapter->registrypriv.wifi_spec) && (!memcmp(pIE->data, WPS_OUI, 4))) {
> 
> $ git grep -w P2P_OUI drivers/staging/rtl8723bs/core
> drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char P2P_OUI[];
> drivers/staging/rtl8723bs/core/rtw_ap.c:        else if (!memcmp(P2P_OUI, oui, 4))
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char P2P_OUI[] = {0x50, 0x6F, 0x9A, 0x09};
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:  if (!memcmp(frame_body + 2, P2P_OUI, 4)) {
> 
> So maybe something like the below (written in email client, maybe typos)
> 
> enum oui_type {
> 	RTW_WPA,
> 	WMM,
> 	WPS,
> 	P2P
> };
> 
> bool is_oui_type(const u8 *mem, enum oui_type type)
> {
> 	static const u8 rtw_wpa[] = {0x00, 0x50, 0xf2, 0x01};
> 	static const u8 wmm[] = {0x00, 0x50, 0xf2, 0x02};
> 	static const u8 wps[] = {0x00, 0x50, 0xf2, 0x04};
> 	static const u8 p2p[] = {0x50, 0x6F, 0x9A, 0x09};
> 
> 	const u8 *oui;
> 
> 	if (type == RTW_WPA)
> 		oui = rtw_wpa;
> 	else if (type == WMM)
> 		oui = wmm;
> 	else if (type == WPS)
> 		oui = wps;
> 	else if (type == P2P)
> 		oui = p2p;
> 	else
> 		return false;
> 
> 	return !memcmp(mem, oui, 4);
> }
> 
> so for instance the P2P uses would become
> 
> 	else if (is_oui_type(oui, P2P))
> and
> 	if (is_oui_type(frame_body + 2, P2P)) {
> 
> though I think 4 byte OUIs are just odd.
> 
> https://en.wikipedia.org/wiki/Organizationally_unique_identifier
> 
> An organizationally unique identifier (OUI) is a 24-bit number that uniquely identifies a vendor, manufacturer, or other organization.
> 
> 
> 

Thank you Joe, is the whole analisys involved with the simple task of removing them from .c files?