[PATCH v2 0/2] Avoid namespace collision within macros & tidyup

[PATCH v2 0/2] Avoid namespace collision within macros & tidyup

[Ramesh Shanmugasundaram]

Hi All,

The readx_poll_timeout & similar macros defines local variable that can
cause name space collision with the caller. Fixed this issue by prefixing
them with underscores. Also tidied couple of instances where the macro
arguments are used in expressions without paranthesis.

This patchset is based on top of today's linux-next repo.
commit bc4c75f41a1c ("Add linux-next specific files for 20170613")

Change history:

v2:
 - iopoll.h:
	- Enclosed timeout_us & sleep_us arguments with paranthesis
 - regmap.h:
	- Enclosed timeout_us & sleep_us arguments with paranthesis
	- Renamed pollret to __ret

Note: timeout_us cause spare check warning as identified here [1].

[1] https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg15138.html

Thanks,
Ramesh

Ramesh Shanmugasundaram (2):
  iopoll: Avoid namespace collision within macros & tidyup
  regmap: Avoid namespace collision within macro & tidyup

 include/linux/iopoll.h | 12 +++++++-----
 include/linux/regmap.h | 17 +++++++++--------
 2 files changed, 16 insertions(+), 13 deletions(-)

-- 
2.12.2

[PATCH v2 1/2] iopoll: Avoid namespace collision within macros & tidyup

[Ramesh Shanmugasundaram]

Renamed variable "timeout" to "__timeout" to avoid namespace collision.
Tidy up macro arguments with paranthesis.

Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
---
 include/linux/iopoll.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index d29e1e21bf3f..e000172bee54 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -42,18 +42,19 @@
  */
 #define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us)	\
 ({ \
-	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+	ktime_t __timeout = ktime_add_us(ktime_get(), timeout_us); \
 	might_sleep_if(sleep_us); \
 	for (;;) { \
 		(val) = op(addr); \
 		if (cond) \
 			break; \
-		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+		if ((timeout_us) && \
+		    ktime_compare(ktime_get(), __timeout) > 0) { \
 			(val) = op(addr); \
 			break; \
 		} \
 		if (sleep_us) \
-			usleep_range((sleep_us >> 2) + 1, sleep_us); \
+			usleep_range(((sleep_us) >> 2) + 1, sleep_us); \
 	} \
 	(cond) ? 0 : -ETIMEDOUT; \
 })
@@ -77,12 +78,13 @@
  */
 #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
 ({ \
-	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+	ktime_t __timeout = ktime_add_us(ktime_get(), timeout_us); \
 	for (;;) { \
 		(val) = op(addr); \
 		if (cond) \
 			break; \
-		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+		if ((timeout_us) && \
+		    ktime_compare(ktime_get(), __timeout) > 0) { \
 			(val) = op(addr); \
 			break; \
 		} \
-- 
2.12.2

Re: [PATCH v2 1/2] iopoll: Avoid namespace collision within macros & tidyup

[Geert Uytterhoeven]

Hi Ramesh,

On Tue, Jun 13, 2017 at 3:33 PM, Ramesh Shanmugasundaram
<ramesh.shanmugasundaram@bp.renesas.com> wrote:
> Renamed variable "timeout" to "__timeout" to avoid namespace collision.
> Tidy up macro arguments with paranthesis.
>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>

Thanks for your patches!

> --- a/include/linux/iopoll.h
> +++ b/include/linux/iopoll.h
> @@ -42,18 +42,19 @@
>   */
>  #define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us)  \
>  ({ \
> -       ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> +       ktime_t __timeout = ktime_add_us(ktime_get(), timeout_us); \

I think timeout_us should be within parentheses, too.

>         might_sleep_if(sleep_us); \
>         for (;;) { \
>                 (val) = op(addr); \
>                 if (cond) \
>                         break; \
> -               if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
> +               if ((timeout_us) && \
> +                   ktime_compare(ktime_get(), __timeout) > 0) { \
>                         (val) = op(addr); \
>                         break; \
>                 } \
>                 if (sleep_us) \
> -                       usleep_range((sleep_us >> 2) + 1, sleep_us); \
> +                       usleep_range(((sleep_us) >> 2) + 1, sleep_us); \

Same for sleep_us.

Also in readx_poll_timeout_atomic(), and in your second patch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 1/2] iopoll: Avoid namespace collision within macros & tidyup

[Ramesh Shanmugasundaram]

Hi Geert,

Thanks for the review. Replying to the thread to update what we discussed in IRC sometime back.

> On Tue, Jun 13, 2017 at 3:33 PM, Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@bp.renesas.com> wrote:
> > Renamed variable "timeout" to "__timeout" to avoid namespace collision.
> > Tidy up macro arguments with paranthesis.
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram@bp.renesas.com>
> 
> Thanks for your patches!
> 
> > --- a/include/linux/iopoll.h
> > +++ b/include/linux/iopoll.h
> > @@ -42,18 +42,19 @@
> >   */
> >  #define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us)
> > \  ({ \
> > -       ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> > +       ktime_t __timeout = ktime_add_us(ktime_get(), timeout_us); \
> 
> I think timeout_us should be within parentheses, too.

It is not required as it is passed as an function (ktime_add_us) argument.

> 
> >         might_sleep_if(sleep_us); \
> >         for (;;) { \
> >                 (val) = op(addr); \
> >                 if (cond) \
> >                         break; \
> > -               if (timeout_us && ktime_compare(ktime_get(), timeout) >
> 0) { \
> > +               if ((timeout_us) && \
> > +                   ktime_compare(ktime_get(), __timeout) > 0) { \
> >                         (val) = op(addr); \
> >                         break; \
> >                 } \
> >                 if (sleep_us) \
> > -                       usleep_range((sleep_us >> 2) + 1, sleep_us); \
> > +                       usleep_range(((sleep_us) >> 2) + 1, sleep_us);
> > + \
> 
> Same for sleep_us.
> 
> Also in readx_poll_timeout_atomic(), and in your second patch.

Same as the above comment.

Thanks,
Ramesh

[PATCH v2 2/2] regmap: Avoid namespace collision within macro & tidyup

[Ramesh Shanmugasundaram]

Renamed variable "timeout" to "__timeout" & "pollret" to "__ret" to
avoid namespace collision. Tidy up macro arguments with paranthesis.

Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
---
 include/linux/regmap.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 86eeacc1425a..ebc7282abc80 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -120,23 +120,24 @@ struct reg_sequence {
  */
 #define regmap_read_poll_timeout(map, addr, val, cond, sleep_us, timeout_us) \
 ({ \
-	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
-	int pollret; \
+	ktime_t __timeout = ktime_add_us(ktime_get(), timeout_us); \
+	int __ret; \
 	might_sleep_if(sleep_us); \
 	for (;;) { \
-		pollret = regmap_read((map), (addr), &(val)); \
-		if (pollret) \
+		__ret = regmap_read((map), (addr), &(val)); \
+		if (__ret) \
 			break; \
 		if (cond) \
 			break; \
-		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
-			pollret = regmap_read((map), (addr), &(val)); \
+		if ((timeout_us) && \
+		    ktime_compare(ktime_get(), __timeout) > 0) { \
+			__ret = regmap_read((map), (addr), &(val)); \
 			break; \
 		} \
 		if (sleep_us) \
-			usleep_range((sleep_us >> 2) + 1, sleep_us); \
+			usleep_range(((sleep_us) >> 2) + 1, sleep_us); \
 	} \
-	pollret ?: ((cond) ? 0 : -ETIMEDOUT); \
+	__ret ?: ((cond) ? 0 : -ETIMEDOUT); \
 })
 
 #ifdef CONFIG_REGMAP
-- 
2.12.2

Applied "regmap: Avoid namespace collision within macro & tidy up" to the regmap tree

[Mark Brown]

The patch

   regmap: Avoid namespace collision within macro & tidy up

has been applied to the regmap tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 780b1350d316fda28d85fcae17854c778d89cbbe Mon Sep 17 00:00:00 2001
From: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
Date: Mon, 3 Jul 2017 12:04:21 +0100
Subject: [PATCH] regmap: Avoid namespace collision within macro & tidy up

Renamed variable "timeout" to "__timeout" & "pollret" to "__ret" to
avoid namespace collision. Tidy up macro arguments with parentheses.

Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/regmap.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index e88649225a60..6e1df5e721a9 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -120,23 +120,24 @@ struct reg_sequence {
  */
 #define regmap_read_poll_timeout(map, addr, val, cond, sleep_us, timeout_us) \
 ({ \
-	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
-	int pollret; \
+	ktime_t __timeout = ktime_add_us(ktime_get(), timeout_us); \
+	int __ret; \
 	might_sleep_if(sleep_us); \
 	for (;;) { \
-		pollret = regmap_read((map), (addr), &(val)); \
-		if (pollret) \
+		__ret = regmap_read((map), (addr), &(val)); \
+		if (__ret) \
 			break; \
 		if (cond) \
 			break; \
-		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
-			pollret = regmap_read((map), (addr), &(val)); \
+		if ((timeout_us) && \
+		    ktime_compare(ktime_get(), __timeout) > 0) { \
+			__ret = regmap_read((map), (addr), &(val)); \
 			break; \
 		} \
 		if (sleep_us) \
-			usleep_range((sleep_us >> 2) + 1, sleep_us); \
+			usleep_range(((sleep_us) >> 2) + 1, sleep_us); \
 	} \
-	pollret ?: ((cond) ? 0 : -ETIMEDOUT); \
+	__ret ?: ((cond) ? 0 : -ETIMEDOUT); \
 })
 
 #ifdef CONFIG_REGMAP
-- 
2.13.2

Re: [PATCH v2 0/2] Avoid namespace collision within macros & tidyup

[Ian Arkver]

On 13/06/17 14:33, Ramesh Shanmugasundaram wrote:
> Hi All,
> 
> The readx_poll_timeout & similar macros defines local variable that can
> cause name space collision with the caller. Fixed this issue by prefixing
> them with underscores.

The compound statement has a local variable scope, so these won't 
collide with the caller I believe.

> Also tidied couple of instances where the macro
> arguments are used in expressions without paranthesis.
> 
> This patchset is based on top of today's linux-next repo.
> commit bc4c75f41a1c ("Add linux-next specific files for 20170613")
> 
> Change history:
> 
> v2:
>   - iopoll.h:
> 	- Enclosed timeout_us & sleep_us arguments with paranthesis
>   - regmap.h:
> 	- Enclosed timeout_us & sleep_us arguments with paranthesis
> 	- Renamed pollret to __ret
> 
> Note: timeout_us cause spare check warning as identified here [1].
> 
> [1] https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg15138.html
> 
> Thanks,
> Ramesh
> 
> Ramesh Shanmugasundaram (2):
>    iopoll: Avoid namespace collision within macros & tidyup
>    regmap: Avoid namespace collision within macro & tidyup
> 
>   include/linux/iopoll.h | 12 +++++++-----
>   include/linux/regmap.h | 17 +++++++++--------
>   2 files changed, 16 insertions(+), 13 deletions(-)
> 

RE: [PATCH v2 0/2] Avoid namespace collision within macros & tidyup

[Ramesh Shanmugasundaram]

> Subject: Re: [PATCH v2 0/2] Avoid namespace collision within macros &
> tidyup
> 
> On 13/06/17 14:33, Ramesh Shanmugasundaram wrote:
> > Hi All,
> >
> > The readx_poll_timeout & similar macros defines local variable that
> > can cause name space collision with the caller. Fixed this issue by
> > prefixing them with underscores.
> 
> The compound statement has a local variable scope, so these won't collide
> with the caller I believe.

But xxx_poll_timeout is a macro??

Usage regmap_read_poll_timeout(..., timeout) with variable name "timeout" in the caller results in 

include/linux/regmap.h:123:20: warning: 'timeout' is used uninitialized in this function [-Wuninitialized]
  ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \

> 
> > Also tidied couple of instances where the macro arguments are used in
> > expressions without paranthesis.
> >
> > This patchset is based on top of today's linux-next repo.
> > commit bc4c75f41a1c ("Add linux-next specific files for 20170613")
> >
> > Change history:
> >
> > v2:
> >   - iopoll.h:
> > 	- Enclosed timeout_us & sleep_us arguments with paranthesis
> >   - regmap.h:
> > 	- Enclosed timeout_us & sleep_us arguments with paranthesis
> > 	- Renamed pollret to __ret
> >
> > Note: timeout_us cause spare check warning as identified here [1].
> >
> > [1]
> > https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg1513
> > 8.html
> >
> > Thanks,
> > Ramesh
> >
> > Ramesh Shanmugasundaram (2):
> >    iopoll: Avoid namespace collision within macros & tidyup
> >    regmap: Avoid namespace collision within macro & tidyup
> >
> >   include/linux/iopoll.h | 12 +++++++-----
> >   include/linux/regmap.h | 17 +++++++++--------
> >   2 files changed, 16 insertions(+), 13 deletions(-)
> >

Re: [PATCH v2 0/2] Avoid namespace collision within macros & tidyup

[Ian Arkver]

On 14/06/17 08:18, Ramesh Shanmugasundaram wrote:
>> Subject: Re: [PATCH v2 0/2] Avoid namespace collision within macros &
>> tidyup
>>
>> On 13/06/17 14:33, Ramesh Shanmugasundaram wrote:
>>> Hi All,
>>>
>>> The readx_poll_timeout & similar macros defines local variable that
>>> can cause name space collision with the caller. Fixed this issue by
>>> prefixing them with underscores.
>>
>> The compound statement has a local variable scope, so these won't collide
>> with the caller I believe.
> 
> But xxx_poll_timeout is a macro??
> 
> Usage regmap_read_poll_timeout(..., timeout) with variable name "timeout" in the caller results in
> 
> include/linux/regmap.h:123:20: warning: 'timeout' is used uninitialized in this function [-Wuninitialized]
>    ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> 

Oh right, collide with a passed in variable, yes. Sorry.

>>
>>> Also tidied couple of instances where the macro arguments are used in
>>> expressions without paranthesis.
>>>
>>> This patchset is based on top of today's linux-next repo.
>>> commit bc4c75f41a1c ("Add linux-next specific files for 20170613")
>>>
>>> Change history:
>>>
>>> v2:
>>>    - iopoll.h:
>>> 	- Enclosed timeout_us & sleep_us arguments with paranthesis
>>>    - regmap.h:
>>> 	- Enclosed timeout_us & sleep_us arguments with paranthesis
>>> 	- Renamed pollret to __ret
>>>
>>> Note: timeout_us cause spare check warning as identified here [1].
>>>
>>> [1]
>>> https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg1513
>>> 8.html
>>>
>>> Thanks,
>>> Ramesh
>>>
>>> Ramesh Shanmugasundaram (2):
>>>     iopoll: Avoid namespace collision within macros & tidyup
>>>     regmap: Avoid namespace collision within macro & tidyup
>>>
>>>    include/linux/iopoll.h | 12 +++++++-----
>>>    include/linux/regmap.h | 17 +++++++++--------
>>>    2 files changed, 16 insertions(+), 13 deletions(-)
>>>