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
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
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
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
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(-)
>
> 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(-)
> >
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(-)
>>>
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
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