linux-kernel - Re: [PATCH v3 3/3] staging: vt6655: Replace VNSvInPortD with ioread32

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: <Ymjaxby2vDJYz6KA@kroah.com>
Date:   Wed, 27 Apr 2022 07:55:17 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Philipp Hortmann <philipp.g.hortmann@...il.com>
Cc:     Forest Bond <forest@...ttletooquiet.net>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] staging: vt6655: Replace VNSvInPortD with ioread32

On Wed, Apr 27, 2022 at 07:42:23AM +0200, Philipp Hortmann wrote:
> Replace macro VNSvInPortD with ioread32 and as it was
> the only user, it can now be removed.
> The name of macro and the arguments use CamelCase which
> is not accepted by checkpatch.pl
> 
> Reported-by: kernel test robot <lkp@...el.com>
> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@...il.com>
> ---
> V1 -> V2: This patch was 5/7 and is now 4/6
> V2 -> V3: Inserted patch that was before in a different way in
>           "Rename macros VNSvInPortB,W,D with CamelCase ..."
>           This patch was part of 4/6 and is now 3/7
> V3 -> V4: Removed casting of the output variable
> V4 -> V5: Joint patch "Replace two VNSvInPortD with ioread64_lo_hi"
>           with this patch. Changed ioread64 to two ioread32 as
>           ioread64 does not work with 32 Bit computers.
>           Shorted and simplified patch description.
> V5 -> V6: Added missing version in subject
> ---
>  drivers/staging/vt6655/card.c        |  6 ++++--
>  drivers/staging/vt6655/device_main.c |  6 +++---
>  drivers/staging/vt6655/mac.h         | 18 +++++++++---------
>  drivers/staging/vt6655/rf.c          |  2 +-
>  drivers/staging/vt6655/upc.h         |  3 ---
>  5 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/card.c b/drivers/staging/vt6655/card.c
> index 022310af5485..0dd13e475d6b 100644
> --- a/drivers/staging/vt6655/card.c
> +++ b/drivers/staging/vt6655/card.c
> @@ -744,6 +744,7 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
>  	void __iomem *iobase = priv->port_offset;
>  	unsigned short ww;
>  	unsigned char data;
> +	u32 low, high;
>  
>  	MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
>  	for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {
> @@ -753,8 +754,9 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
>  	}
>  	if (ww == W_MAX_TIMEOUT)
>  		return false;
> -	VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);
> -	VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1);
> +	low = ioread32(iobase + MAC_REG_TSFCNTR);
> +	high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
> +	*pqwCurrTSF = low + ((u64)high << 32);

Are you _sure_ this is doing the same thing?

Adding 1 to a u64 pointer increments it by a full u64.  So I guess the
cast to u32 * moves it only by a u32?  Hopefully?  That's messy.

Why not keep the current mess and do:
	pqwCurrTSF = ioread32(iobase + MAC_REG_TSFCNTR);
	((u32 *)pqwCurTSF + 1) = ioread32(iobase + MAC_REG_TSFCNTR + 4);

Or does that not compile?  Ick, how about:
	u32 *temp = (u32 *)pqwCurTSF;

	temp = ioread32(iobase + MAC_REG_TSFCNTR);
	temp++;
	temp = ioread32(iobase + MAC_REG_TSFCNTR + 4);
As that duplicates the current code a bit better.

I don't know, it's like polishing dirt, in the end, it's still dirt...

How about looking at the caller of this to see what it expects to do
with this information?  Unwind the mess from there?

thanks,

greg k-h