linux-kernel - Re: [PATCH v3 4/7] staging: vt6655: Replace VNSvOutPortB with iowrite8

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]

Date:   Tue, 12 Apr 2022 09:37:32 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Philipp Hortmann <philipp.g.hortmann@...il.com>
Cc:     Forest Bond <forest@...ttletooquiet.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/7] staging: vt6655: Replace VNSvOutPortB with
 iowrite8

On Mon, Apr 11, 2022 at 10:49:39PM +0200, Philipp Hortmann wrote:
> Replace macro VNSvOutPortB with iowrite8.
> The name of macro and the arguments use CamelCase which
> is not accepted by checkpatch.pl
> 
> For constants from 0 to below 0x80 the u8 cast was omitted.
> For variables which are defined as unsigned char the u8 is omitted.

I hate that GCC prints warnings for this.  Useless.  Horrible.  But I
understand that GCC does and we haven't figured out how to disable it
or who needs to approve that.

But even then I still don't understand the casting in this patch.

Shouldn't the rule be to do the minimum work arounds to silence GCC?
My understand is that the the casting is only needed when you're dealing
with a bitwise negated constant.  These are macros so the parameters
might be constant so basically any bitwise negate gets a cast.

>  #define MACvWordRegBitsOff(iobase, byRegOfs, wBits)			\
> @@ -578,37 +578,30 @@ do {									\
>  
>  #define MACvWriteBSSIDAddress(iobase, pbyEtherAddr)		\
>  do {								\
> -	VNSvOutPortB(iobase + MAC_REG_PAGE1SEL, 1);		\
> -	VNSvOutPortB(iobase + MAC_REG_BSSID0,			\
> -		     *(pbyEtherAddr));				\
> -	VNSvOutPortB(iobase + MAC_REG_BSSID0 + 1,		\
> -		     *(pbyEtherAddr + 1));			\
> -	VNSvOutPortB(iobase + MAC_REG_BSSID0 + 2,		\
> -		     *(pbyEtherAddr + 2));			\
> -	VNSvOutPortB(iobase + MAC_REG_BSSID0 + 3,		\
> -		     *(pbyEtherAddr + 3));			\
> -	VNSvOutPortB(iobase + MAC_REG_BSSID0 + 4,		\
> -		     *(pbyEtherAddr + 4));			\
> -	VNSvOutPortB(iobase + MAC_REG_BSSID0 + 5,		\
> -		     *(pbyEtherAddr + 5));			\
> -	VNSvOutPortB(iobase + MAC_REG_PAGE1SEL, 0);		\
> +	iowrite8(1, iobase + MAC_REG_PAGE1SEL);		\
> +	iowrite8((u8)*(pbyEtherAddr), iobase + MAC_REG_BSSID0);				\
> +	iowrite8((u8)*(pbyEtherAddr + 1), iobase + MAC_REG_BSSID0 + 1);			\
> +	iowrite8((u8)*(pbyEtherAddr + 2), iobase + MAC_REG_BSSID0 + 2);			\
> +	iowrite8((u8)*(pbyEtherAddr + 3), iobase + MAC_REG_BSSID0 + 3);			\
> +	iowrite8((u8)*(pbyEtherAddr + 4), iobase + MAC_REG_BSSID0 + 4);			\
> +	iowrite8((u8)*(pbyEtherAddr + 5), iobase + MAC_REG_BSSID0 + 5);			\
> +	iowrite8(0, iobase + MAC_REG_PAGE1SEL);		\
>  } while (0)


If these casts are required then the pointer math is wrong.  #SeriousBug
I looked at the caller and these casts are not required.  Just remove
the casts.

>  #define MACvClearStckDS(iobase)					\
>  do {									\
>  	unsigned char byOrgValue;					\
>  	byOrgValue = ioread8(iobase + MAC_REG_STICKHW);		\
>  	byOrgValue = byOrgValue & 0xFC;					\
> -	VNSvOutPortB(iobase + MAC_REG_STICKHW, byOrgValue);		\
> +	iowrite8((u8)byOrgValue, iobase + MAC_REG_STICKHW);		\
>  } while (0)

This cast is definitely not required.  byOrgValue is declared as local
to the block so we know it's unsigned char.

> diff --git a/drivers/staging/vt6655/srom.c b/drivers/staging/vt6655/srom.c
> index a0432bacb6a0..7feaa5138de0 100644
> --- a/drivers/staging/vt6655/srom.c
> +++ b/drivers/staging/vt6655/srom.c
> @@ -68,13 +68,13 @@ unsigned char SROMbyReadEmbedded(void __iomem *iobase,
>  	byData = 0xFF;
>  	byOrg = ioread8(iobase + MAC_REG_I2MCFG);
>  	/* turn off hardware retry for getting NACK */
> -	VNSvOutPortB(iobase + MAC_REG_I2MCFG, (byOrg & (~I2MCFG_NORETRY)));
> +	iowrite8((u8)(byOrg & (~I2MCFG_NORETRY)), iobase + MAC_REG_I2MCFG);

In this case we have a bitwise negate but it's ANDed with a u8 so there
is no cast required (hopefully 0_0).

regards,
dan carpenter