linux-kernel - Re: [PATCH 2/2] staging: vt6655: Replace unused return value of card_get_current

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:   Sun, 1 May 2022 18:53:02 +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 2/2] staging: vt6655: Replace unused return value of
 card_get_current_tsf

On Sun, May 01, 2022 at 06:37:11PM +0200, Philipp Hortmann wrote:
> Replace unused return value with u64 to increase readability,
> reduce address and dereference operators and omit pqwCurrTSF that
> uses CamelCase which is not accepted by checkpatch.pl
> 
> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@...il.com>
> ---
> Diffs for testing:
>         u64 qwNextTBTT;
> 
>         qwNextTBTT = card_get_current_tsf(priv); /* Get Local TSF counter */
> +       dev_info(&priv->pcid->dev, "CARDbSetBeaconPeriod 0x%016llx", qwNextTBTT);

Don't put a diff in a diff please, git _should_ handle it, but I know
other tools that will choke on it.

>         qwNextTBTT = CARDqGetNextTBTT(qwNextTBTT, wBeaconInterval);
> 
>         /* set HW beacon interval */
> @@ -810,7 +810,7 @@ void CARDvSetFirstNextTBTT(struct vnt_private *priv,
>         u64 qwNextTBTT;
> 
>         qwNextTBTT = card_get_current_tsf(priv); /* Get Local TSF counter */
> +       dev_info(&priv->pcid->dev, "CARDvSetFirstNextTBTT 0x%016llx", qwNextTBTT);
>         qwNextTBTT = CARDqGetNextTBTT(qwNextTBTT, wBeaconInterval);
>         /* Set NextTBTT */
>         VNSvOutPortD(iobase + MAC_REG_NEXTTBTT, (u32)qwNextTBTT);
> Log:
> vt6655 0000:01:05.0: CARDbSetBeaconPeriod  0x00 00 00 01 4a 89 52 c4
> vt6655 0000:01:05.0: CARDvSetFirstNextTBTT 0x00 00 00 01 4a 89 52 dc
> ---
>  drivers/staging/vt6655/card.c        | 20 +++++++++-----------
>  drivers/staging/vt6655/card.h        |  2 +-
>  drivers/staging/vt6655/device_main.c |  2 +-
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/card.c b/drivers/staging/vt6655/card.c
> index d1dfd96e13b7..50d70ebff83a 100644
> --- a/drivers/staging/vt6655/card.c
> +++ b/drivers/staging/vt6655/card.c
> @@ -288,7 +288,7 @@ bool CARDbUpdateTSF(struct vnt_private *priv, unsigned char byRxRate,
>  	u64 local_tsf;
>  	u64 qwTSFOffset = 0;
>  
> -	card_get_current_tsf(priv, &local_tsf);
> +	local_tsf = card_get_current_tsf(priv);

{sigh}  I should read all patches in the series before complaining about
previous ones, sorry about that.

Looks much better, except for the function name.



>  
>  	if (qwBSSTimestamp != local_tsf) {
>  		qwTSFOffset = CARDqGetTSFOffset(byRxRate, qwBSSTimestamp,
> @@ -320,9 +320,9 @@ bool CARDbUpdateTSF(struct vnt_private *priv, unsigned char byRxRate,
>  bool CARDbSetBeaconPeriod(struct vnt_private *priv,
>  			  unsigned short wBeaconInterval)
>  {
> -	u64 qwNextTBTT = 0;
> +	u64 qwNextTBTT;
>  
> -	card_get_current_tsf(priv, &qwNextTBTT); /* Get Local TSF counter */
> +	qwNextTBTT = card_get_current_tsf(priv); /* Get Local TSF counter */
>  
>  	qwNextTBTT = CARDqGetNextTBTT(qwNextTBTT, wBeaconInterval);
>  
> @@ -739,7 +739,7 @@ u64 CARDqGetTSFOffset(unsigned char byRxRate, u64 qwTSF1, u64 qwTSF2)
>   *
>   * Return Value: true if success; otherwise false
>   */
> -bool card_get_current_tsf(struct vnt_private *priv, u64 *pqwCurrTSF)
> +u64 card_get_current_tsf(struct vnt_private *priv)
>  {
>  	void __iomem *iobase = priv->port_offset;
>  	unsigned short ww;
> @@ -753,16 +753,14 @@ bool card_get_current_tsf(struct vnt_private *priv, u64 *pqwCurrTSF)
>  			break;
>  	}
>  	if (ww == W_MAX_TIMEOUT)
> -		return false;
> +		return 0;
>  	low = ioread32(iobase + MAC_REG_TSFCNTR);
>  	high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
>  #ifdef __BIG_ENDIAN

Note, this #ifdef really should never be in kernel code if you are doing
things properly.  There are functions to handle this correctly...

Things get "fun" when you have devices in one endian running on busses
of other endian.  Hopefully this driver never has to deal with that, but
for many others we do, so please stick with the normal functions to
handle this whenever possible.

thanks,

greg k-h