2017-06-13 13:47:15

by Ramesh Shanmugasundaram

[permalink] [raw]
Subject: [PATCH v2 0/2] Avoid namespace collision within macros & tidyup

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/[email protected]/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


2017-06-13 13:47:18

by Ramesh Shanmugasundaram

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

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

Signed-off-by: Ramesh Shanmugasundaram <[email protected]>
---
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

2017-06-13 13:47:37

by Ramesh Shanmugasundaram

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

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

Signed-off-by: Ramesh Shanmugasundaram <[email protected]>
---
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

2017-06-13 14:15:10

by Geert Uytterhoeven

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

Hi Ramesh,

On Tue, Jun 13, 2017 at 3:33 PM, Ramesh Shanmugasundaram
<[email protected]> wrote:
> Renamed variable "timeout" to "__timeout" to avoid namespace collision.
> Tidy up macro arguments with paranthesis.
>
> Signed-off-by: Ramesh Shanmugasundaram <[email protected]>

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 -- [email protected]

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

2017-06-14 06:48:22

by Ian Arkver

[permalink] [raw]
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.

> 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/[email protected]/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(-)
>

2017-06-14 07:18:19

by Ramesh Shanmugasundaram

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

> 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/[email protected]/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(-)
> >

2017-06-14 07:23:30

by Ian Arkver

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

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/[email protected]/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(-)
>>>

2017-07-03 09:56:03

by Ramesh Shanmugasundaram

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

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
> <[email protected]> wrote:
> > Renamed variable "timeout" to "__timeout" to avoid namespace collision.
> > Tidy up macro arguments with paranthesis.
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <[email protected]>
>
> 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

2017-07-10 18:42:35

by Mark Brown

[permalink] [raw]
Subject: Applied "regmap: Avoid namespace collision within macro & tidy up" to the regmap tree

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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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