* [PATCH] staging: rts5208: Parenthesize macro arguments
@ 2023-10-12 5:02 Soumya Negi
2023-10-12 6:33 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Soumya Negi @ 2023-10-12 5:02 UTC (permalink / raw)
To: Greg Kroah-Hartman, Micky Ching
Cc: Soumya Negi, outreachy, linux-staging, linux-kernel
Use parenthesis with macro arguments to avoid possible precedence
issues. Found by checkpatch.pl
Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
---
drivers/staging/rts5208/rtsx.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/rts5208/rtsx.h b/drivers/staging/rts5208/rtsx.h
index 2e101da83220..1cc05956ab6d 100644
--- a/drivers/staging/rts5208/rtsx.h
+++ b/drivers/staging/rts5208/rtsx.h
@@ -40,17 +40,17 @@
* macros for easy use
*/
#define rtsx_writel(chip, reg, value) \
- iowrite32(value, (chip)->rtsx->remap_addr + reg)
+ iowrite32(value, (chip)->rtsx->remap_addr + (reg))
#define rtsx_readl(chip, reg) \
- ioread32((chip)->rtsx->remap_addr + reg)
+ ioread32((chip)->rtsx->remap_addr + (reg))
#define rtsx_writew(chip, reg, value) \
- iowrite16(value, (chip)->rtsx->remap_addr + reg)
+ iowrite16(value, (chip)->rtsx->remap_addr + (reg))
#define rtsx_readw(chip, reg) \
- ioread16((chip)->rtsx->remap_addr + reg)
+ ioread16((chip)->rtsx->remap_addr + (reg))
#define rtsx_writeb(chip, reg, value) \
- iowrite8(value, (chip)->rtsx->remap_addr + reg)
+ iowrite8(value, (chip)->rtsx->remap_addr + (reg))
#define rtsx_readb(chip, reg) \
- ioread8((chip)->rtsx->remap_addr + reg)
+ ioread8((chip)->rtsx->remap_addr + (reg))
#define rtsx_read_config_byte(chip, where, val) \
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)
#define lock_state(chip) spin_lock_irq(&((chip)->rtsx->reg_lock))
#define unlock_state(chip) spin_unlock_irq(&((chip)->rtsx->reg_lock))
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rts5208: Parenthesize macro arguments
2023-10-12 5:02 [PATCH] staging: rts5208: Parenthesize macro arguments Soumya Negi
@ 2023-10-12 6:33 ` Dan Carpenter
2023-10-12 7:48 ` Soumya Negi
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-10-12 6:33 UTC (permalink / raw)
To: Soumya Negi
Cc: Greg Kroah-Hartman, Micky Ching, outreachy, linux-staging, linux-kernel
On Wed, Oct 11, 2023 at 10:02:40PM -0700, Soumya Negi wrote:
> Use parenthesis with macro arguments to avoid possible precedence
> issues. Found by checkpatch.pl
>
> Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> ---
> drivers/staging/rts5208/rtsx.h | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.h b/drivers/staging/rts5208/rtsx.h
> index 2e101da83220..1cc05956ab6d 100644
> --- a/drivers/staging/rts5208/rtsx.h
> +++ b/drivers/staging/rts5208/rtsx.h
> @@ -40,17 +40,17 @@
> * macros for easy use
> */
> #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.
> #define rtsx_readl(chip, reg) \
> - ioread32((chip)->rtsx->remap_addr + reg)
> + ioread32((chip)->rtsx->remap_addr + (reg))
> #define rtsx_writew(chip, reg, value) \
> - iowrite16(value, (chip)->rtsx->remap_addr + reg)
> + iowrite16(value, (chip)->rtsx->remap_addr + (reg))
> #define rtsx_readw(chip, reg) \
> - ioread16((chip)->rtsx->remap_addr + reg)
> + ioread16((chip)->rtsx->remap_addr + (reg))
> #define rtsx_writeb(chip, reg, value) \
> - iowrite8(value, (chip)->rtsx->remap_addr + reg)
> + iowrite8(value, (chip)->rtsx->remap_addr + (reg))
> #define rtsx_readb(chip, reg) \
> - ioread8((chip)->rtsx->remap_addr + reg)
> + ioread8((chip)->rtsx->remap_addr + (reg))
>
> #define rtsx_read_config_byte(chip, where, val) \
> 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.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rts5208: Parenthesize macro arguments
2023-10-12 6:33 ` Dan Carpenter
@ 2023-10-12 7:48 ` Soumya Negi
2023-10-12 7:51 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Soumya Negi @ 2023-10-12 7:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Greg Kroah-Hartman, Micky Ching, outreachy, linux-staging, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rts5208: Parenthesize macro arguments
2023-10-12 7:48 ` Soumya Negi
@ 2023-10-12 7:51 ` Julia Lawall
2023-10-12 12:49 ` Soumya Negi
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2023-10-12 7:51 UTC (permalink / raw)
To: Soumya Negi
Cc: Dan Carpenter, Greg Kroah-Hartman, Micky Ching, outreachy,
linux-staging, linux-kernel
On Thu, 12 Oct 2023, Soumya Negi wrote:
> 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:
You should make a static inline function if you can. That would require
that the parameter types are the same at all call sites and that all of
the arguments are used as values, not eg as the left side of an assignment
(looks fine here).
>
> /*
> * 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.
Patch set, since they affect the same file. Otherwise, Greg doesn't know
in what order to apply them.
julia
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rts5208: Parenthesize macro arguments
2023-10-12 7:51 ` Julia Lawall
@ 2023-10-12 12:49 ` Soumya Negi
2023-10-12 13:03 ` Soumya Negi
0 siblings, 1 reply; 6+ messages in thread
From: Soumya Negi @ 2023-10-12 12:49 UTC (permalink / raw)
To: Julia Lawall
Cc: Dan Carpenter, Greg Kroah-Hartman, Micky Ching, outreachy,
linux-staging, linux-kernel
Hi Julia,
On Thu, Oct 12, 2023 at 09:51:27AM +0200, Julia Lawall wrote:
>
>
> On Thu, 12 Oct 2023, Soumya Negi wrote:
>
> > Hi Dan,
> > > 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.
>
> Patch set, since they affect the same file. Otherwise, Greg doesn't know
> in what order to apply them.
Thank you for explaining each point. I'm sending over the patch set for
review in a new email thread.
- Soumya
> julia
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rts5208: Parenthesize macro arguments
2023-10-12 12:49 ` Soumya Negi
@ 2023-10-12 13:03 ` Soumya Negi
0 siblings, 0 replies; 6+ messages in thread
From: Soumya Negi @ 2023-10-12 13:03 UTC (permalink / raw)
To: Julia Lawall
Cc: Dan Carpenter, Greg Kroah-Hartman, Micky Ching, outreachy,
linux-staging, linux-kernel
Hi,
On Thu, Oct 12, 2023 at 05:49:20AM -0700, Soumya Negi wrote:
> Hi Julia,
>
> On Thu, Oct 12, 2023 at 09:51:27AM +0200, Julia Lawall wrote:
> >
> >
> > On Thu, 12 Oct 2023, Soumya Negi wrote:
> >
> > > Hi Dan,
> > > > 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.
> >
> > Patch set, since they affect the same file. Otherwise, Greg doesn't know
> > in what order to apply them.
>
> Thank you for explaining each point. I'm sending over the patch set for
> review in a new email thread.
My last patch in the set didn't go through. THe error message is "multiple In-Reply-To
headers. To reduce the amount of spam sent to Gmail, this message has
been blocked." I used the --thread=shallow flag with git format-patch.
Should I try resend the entire patch set again without the flag? Or is
there any way to send the remaining patch by itself?
Thanks,
Soumya
> - Soumya
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-12 13:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-12 5:02 [PATCH] staging: rts5208: Parenthesize macro arguments Soumya Negi
2023-10-12 6:33 ` Dan Carpenter
2023-10-12 7:48 ` Soumya Negi
2023-10-12 7:51 ` Julia Lawall
2023-10-12 12:49 ` Soumya Negi
2023-10-12 13:03 ` Soumya Negi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).