linux-kernel - Re: [PATCH 5/7] staging: vt6655: Replace MACvReceive1 with function vt6655_mac_dma

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220719100420.GF2338@kadam>
Date:   Tue, 19 Jul 2022 13:04:20 +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 5/7] staging: vt6655: Replace MACvReceive1 with function
 vt6655_mac_dma_ctl

On Mon, Jul 18, 2022 at 10:00:59PM +0200, Philipp Hortmann wrote:
> > > diff --git a/drivers/staging/vt6655/mac.h b/drivers/staging/vt6655/mac.h
> > > index 5747de436911..129a6602f6f0 100644
> > > --- a/drivers/staging/vt6655/mac.h
> > > +++ b/drivers/staging/vt6655/mac.h
> > > @@ -537,16 +537,6 @@
> > >   /*---------------------  Export Macros ------------------------------*/
> > > -#define MACvReceive1(iobase)						\
> > > -do {									\
> > > -	unsigned long reg_value;					\
> > > -	reg_value = ioread32(iobase + MAC_REG_RXDMACTL1);		\
> > > -	if (reg_value & DMACTL_RUN)					\
> > > -		iowrite32(DMACTL_WAKE, iobase + MAC_REG_RXDMACTL1);	\
> > > -	else								\
> > > -		iowrite32(DMACTL_RUN, iobase + MAC_REG_RXDMACTL1);	\
> > > -} while (0)
> > > -
> > >   #define MACvTransmit0(iobase)						\
> > >   do {									\
> > >   	unsigned long reg_value;					\
> > 
> 
> I was asking in kernelnewbies what to do with multi line macros as
> checkpatch.pl warnings cannot be totally avoided.
> 
> Greg replied to make functions out of them.
> 
> Please find the full email under:
> 
> https://www.mail-archive.com/kernelnewbies@kernelnewbies.org/msg22042.html
> 
> In this case I really like the static function solution much more than the
> macros.

Yes.  We all like the static function.  It's the commit message, I'm not
so keen on.

You could have avoided the checkpatch warning with an assignment at the
start of the macro:

	typeof(iobase) base = (iobase);

#define MACvReceive1(iobase)						\
do {									\
	typeof(iobase) base = (iobase);					\
	unsigned long reg_value = ioread32(base + MAC_REG_RXDMACTL1);	\
	if (reg_value & DMACTL_RUN)					\
		iowrite32(DMACTL_WAKE, base + MAC_REG_RXDMACTL1);	\
	else								\
		iowrite32(DMACTL_RUN, base + MAC_REG_RXDMACTL1);	\
} while (0)

It's not a *good* solution, but it works.

The means the "iobase" argument would only be executed one time.
Imagine if someone passed "iobase++" to the original function.  It
would have incremented twice instead of once as expected.  That's what
the checkpatch warning is saying.  Nothing to do with multiple lines.

regards,
dan carpenter