linux-kernel - Re: [PATCH] staging: rts5208: Parenthesize macro arguments

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:   Thu, 12 Oct 2023 00:48:38 -0700
From:   Soumya Negi <soumya.negi97@...il.com>
To:     Dan Carpenter <dan.carpenter@...aro.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Micky Ching <micky_ching@...lsil.com.cn>,
        outreachy@...ts.linux.dev, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rts5208: Parenthesize macro arguments

Hi Dan,

On Thu, Oct 12, 2023 at 09:33:07AM +0300, Dan Carpenter wrote:
> On Wed, Oct 11, 2023 at 10:02:40PM -0700, Soumya Negi wrote:
> > Use parenthesis with macro arguments to avoid possible precedence
...
> >   */
> >  #define rtsx_writel(chip, reg, value) \
> > -	iowrite32(value, (chip)->rtsx->remap_addr + reg)
> > +	iowrite32(value, (chip)->rtsx->remap_addr + (reg))
> 
> These would be better as functions instead of defines.

There are actually more macros in the code. Should all of
them be redefined as functions? The original code looks like this:

/*
 * macros for easy use
 */
#define rtsx_writel(chip, reg, value) \
	iowrite32(value, (chip)->rtsx->remap_addr + reg)
#define rtsx_readl(chip, reg) \
	ioread32((chip)->rtsx->remap_addr + reg)
#define rtsx_writew(chip, reg, value) \
	iowrite16(value, (chip)->rtsx->remap_addr + reg)
#define rtsx_readw(chip, reg) \
	ioread16((chip)->rtsx->remap_addr + reg)
#define rtsx_writeb(chip, reg, value) \
	iowrite8(value, (chip)->rtsx->remap_addr + reg)
#define rtsx_readb(chip, reg) \
	ioread8((chip)->rtsx->remap_addr + reg)

#define rtsx_read_config_byte(chip, where, val) \
	pci_read_config_byte((chip)->rtsx->pci, where, val)

#define rtsx_write_config_byte(chip, where, val) \
	pci_write_config_byte((chip)->rtsx->pci, where, val)

#define wait_timeout_x(task_state, msecs)	\
do {						\
	set_current_state((task_state));	\
	schedule_timeout((msecs) * HZ / 1000);	\
} while (0)
#define wait_timeout(msecs)	wait_timeout_x(TASK_INTERRUPTIBLE, (msecs))

> >  	pci_read_config_byte((chip)->rtsx->pci, where, val)
> > @@ -131,8 +131,8 @@ static inline struct rtsx_dev *host_to_rtsx(struct Scsi_Host *host)
> >   * The scsi_lock() and scsi_unlock() macros protect the sm_state and the
> >   * single queue element srb for write access
> >   */
> > -#define scsi_unlock(host)	spin_unlock_irq(host->host_lock)
> > -#define scsi_lock(host)		spin_lock_irq(host->host_lock)
> > +#define scsi_unlock(host)	spin_unlock_irq((host)->host_lock)
> > +#define scsi_lock(host)		spin_lock_irq((host)->host_lock)
> 
> For these ones, the name is too generic.  probably the right thing is
> to just get rid of them completely and call spin_lock/unlock_irq()
> directly.

I understand that there should be 2 different patches, one for the
macro-to-function rewrites & one for replacing the scsi lock/unlock macros with
direct spinlock calls. But, should these be in a patchset(they are vaguely
related since the patches together would get rid of the checkpatch warnings)?
I'm not sure.

Thanks,
Soumya

> regards,
> dan carpenter
>