diff mbox series

[v6,1/3] phy: mediatek: fix build warning caused by clang

Message ID 20230104132646.7100-1-chunfeng.yun@mediatek.com (mailing list archive)
State New, archived
Headers show
Series [v6,1/3] phy: mediatek: fix build warning caused by clang | expand

Commit Message

Chunfeng Yun Jan. 4, 2023, 1:26 p.m. UTC
Remove the temporary @mask_, this may cause build warning when use clang
compiler for powerpc, but can't reproduce it when compile for arm64.
the build warning is -Wtautological-constant-out-of-range-compare, and
caused by
"BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask)"

After removing @mask_, there is a "CHECK:MACRO_ARG_REUSE" when run
checkpatch.pl, due to @mask is constant, no reuse problem will happen.

Fixes: 84513eccd678 ("phy: mediatek: fix build warning of FIELD_PREP()")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v6: modify the title
v5: no changes
v4 new patch, I'm not sure it can fix build warning, due to I don't cross compile
    it for powerpc using clang in office.
---
 drivers/phy/mediatek/phy-mtk-io.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Vinod Koul Jan. 13, 2023, 6:12 p.m. UTC | #1
On 04-01-23, 21:26, Chunfeng Yun wrote:
> Remove the temporary @mask_, this may cause build warning when use clang
> compiler for powerpc, but can't reproduce it when compile for arm64.
> the build warning is -Wtautological-constant-out-of-range-compare, and
> caused by
> "BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask)"
> 
> After removing @mask_, there is a "CHECK:MACRO_ARG_REUSE" when run
> checkpatch.pl, due to @mask is constant, no reuse problem will happen.
> 
> Fixes: 84513eccd678 ("phy: mediatek: fix build warning of FIELD_PREP()")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v6: modify the title

Title still does not tell what this patch is.. It tells me effect of the
patch but not the changes, pls revise...

"remove temp mask" can be better title

> v5: no changes
> v4 new patch, I'm not sure it can fix build warning, due to I don't cross compile
>     it for powerpc using clang in office.
> ---
>  drivers/phy/mediatek/phy-mtk-io.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/mediatek/phy-mtk-io.h b/drivers/phy/mediatek/phy-mtk-io.h
> index d20ad5e5be81..58f06db822cb 100644
> --- a/drivers/phy/mediatek/phy-mtk-io.h
> +++ b/drivers/phy/mediatek/phy-mtk-io.h
> @@ -39,8 +39,8 @@ static inline void mtk_phy_update_bits(void __iomem *reg, u32 mask, u32 val)
>  /* field @mask shall be constant and continuous */
>  #define mtk_phy_update_field(reg, mask, val) \
>  ({ \
> -	typeof(mask) mask_ = (mask);	\
> -	mtk_phy_update_bits(reg, mask_, FIELD_PREP(mask_, val)); \
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not constant"); \
> +	mtk_phy_update_bits(reg, mask, FIELD_PREP(mask, val)); \
>  })
>  
>  #endif
> -- 
> 2.18.0
Nick Desaulniers Jan. 13, 2023, 6:36 p.m. UTC | #2
On Wed, Jan 4, 2023 at 5:26 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> Remove the temporary @mask_, this may cause build warning when use clang
> compiler for powerpc, but can't reproduce it when compile for arm64.
> the build warning is -Wtautological-constant-out-of-range-compare, and
> caused by
> "BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask)"

Can you please include the text of the observed warning?

>
> After removing @mask_, there is a "CHECK:MACRO_ARG_REUSE" when run
> checkpatch.pl, due to @mask is constant, no reuse problem will happen.
>
> Fixes: 84513eccd678 ("phy: mediatek: fix build warning of FIELD_PREP()")

Uh, why was 84513eccd678 sent/reviewed/merged if it didn't fix the
issue correctly in the first place?

Is the issue perhaps that your mask isn't wide enough in the first
place, and should be?  See:
commit cfd6fb45cfaf ("crypto: ccree - avoid out-of-range warnings from clang")
for inspiration.

> Reported-by: kernel test robot <lkp@intel.com>

Can you please include the Link: tag to the lore URL of the report?

> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v6: modify the title
> v5: no changes
> v4 new patch, I'm not sure it can fix build warning, due to I don't cross compile
>     it for powerpc using clang in office.
> ---
>  drivers/phy/mediatek/phy-mtk-io.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/mediatek/phy-mtk-io.h b/drivers/phy/mediatek/phy-mtk-io.h
> index d20ad5e5be81..58f06db822cb 100644
> --- a/drivers/phy/mediatek/phy-mtk-io.h
> +++ b/drivers/phy/mediatek/phy-mtk-io.h
> @@ -39,8 +39,8 @@ static inline void mtk_phy_update_bits(void __iomem *reg, u32 mask, u32 val)
>  /* field @mask shall be constant and continuous */
>  #define mtk_phy_update_field(reg, mask, val) \
>  ({ \
> -       typeof(mask) mask_ = (mask);    \
> -       mtk_phy_update_bits(reg, mask_, FIELD_PREP(mask_, val)); \
> +       BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not constant"); \
> +       mtk_phy_update_bits(reg, mask, FIELD_PREP(mask, val)); \
>  })
>
>  #endif
> --
> 2.18.0
>
Chunfeng Yun Jan. 16, 2023, 7:24 a.m. UTC | #3
On Fri, 2023-01-13 at 23:42 +0530, Vinod Koul wrote:
> On 04-01-23, 21:26, Chunfeng Yun wrote:
> > Remove the temporary @mask_, this may cause build warning when use
> > clang
> > compiler for powerpc, but can't reproduce it when compile for
> > arm64.
> > the build warning is -Wtautological-constant-out-of-range-compare,
> > and
> > caused by
> > "BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask)"
> > 
> > After removing @mask_, there is a "CHECK:MACRO_ARG_REUSE" when run
> > checkpatch.pl, due to @mask is constant, no reuse problem will
> > happen.
> > 
> > Fixes: 84513eccd678 ("phy: mediatek: fix build warning of
> > FIELD_PREP()")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v6: modify the title
> 
> Title still does not tell what this patch is.. It tells me effect of
> the
> patch but not the changes, pls revise...
> 
> "remove temp mask" can be better title
Got it, will modify it in next version, thanks

> 
> > v5: no changes
> > v4 new patch, I'm not sure it can fix build warning, due to I don't
> > cross compile
> >     it for powerpc using clang in office.
> > ---
> >  drivers/phy/mediatek/phy-mtk-io.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/phy/mediatek/phy-mtk-io.h
> > b/drivers/phy/mediatek/phy-mtk-io.h
> > index d20ad5e5be81..58f06db822cb 100644
> > --- a/drivers/phy/mediatek/phy-mtk-io.h
> > +++ b/drivers/phy/mediatek/phy-mtk-io.h
> > @@ -39,8 +39,8 @@ static inline void mtk_phy_update_bits(void
> > __iomem *reg, u32 mask, u32 val)
> >  /* field @mask shall be constant and continuous */
> >  #define mtk_phy_update_field(reg, mask, val) \
> >  ({ \
> > -	typeof(mask) mask_ = (mask);	\
> > -	mtk_phy_update_bits(reg, mask_, FIELD_PREP(mask_, val)); \
> > +	BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not
> > constant"); \
> > +	mtk_phy_update_bits(reg, mask, FIELD_PREP(mask, val)); \
> >  })
> >  
> >  #endif
> > -- 
> > 2.18.0
> 
>
Chunfeng Yun Jan. 16, 2023, 7:51 a.m. UTC | #4
On Fri, 2023-01-13 at 10:36 -0800, Nick Desaulniers wrote:
> On Wed, Jan 4, 2023 at 5:26 AM Chunfeng Yun <
> chunfeng.yun@mediatek.com> wrote:
> > 
> > Remove the temporary @mask_, this may cause build warning when use
> > clang
> > compiler for powerpc, but can't reproduce it when compile for
> > arm64.
> > the build warning is -Wtautological-constant-out-of-range-compare,
> > and
> > caused by
> > "BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask)"
> 
> Can you please include the text of the observed warning?
Ok, will add more logs

> 
> > 
> > After removing @mask_, there is a "CHECK:MACRO_ARG_REUSE" when run
> > checkpatch.pl, due to @mask is constant, no reuse problem will
> > happen.
> > 
> > Fixes: 84513eccd678 ("phy: mediatek: fix build warning of
> > FIELD_PREP()")
> 
> Uh, why was 84513eccd678 sent/reviewed/merged if it didn't fix the
> issue correctly in the first place?
Sorry, I can't reproduce it, but make sure no such issue happens on arm
arch using gcc/clang. MTK only uses arm/mips arch, it's difficult for
me to set up clang cross compiler for other archs in office. 

> 
> Is the issue perhaps that your mask isn't wide enough in the first
> place, and should be? 
the masks are usually created by GENMASK macro;

There is no warning when build for arm64 using clang.

>  See:
> commit cfd6fb45cfaf ("crypto: ccree - avoid out-of-range warnings
> from clang")
> for inspiration.
Ok, I'll do it
> 
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> Can you please include the Link: tag to the lore URL of the report?
Ok, thanks a lot

> 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v6: modify the title
> > v5: no changes
> > v4 new patch, I'm not sure it can fix build warning, due to I don't
> > cross compile
> >     it for powerpc using clang in office.
> > ---
> >  drivers/phy/mediatek/phy-mtk-io.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/phy/mediatek/phy-mtk-io.h
> > b/drivers/phy/mediatek/phy-mtk-io.h
> > index d20ad5e5be81..58f06db822cb 100644
> > --- a/drivers/phy/mediatek/phy-mtk-io.h
> > +++ b/drivers/phy/mediatek/phy-mtk-io.h
> > @@ -39,8 +39,8 @@ static inline void mtk_phy_update_bits(void
> > __iomem *reg, u32 mask, u32 val)
> >  /* field @mask shall be constant and continuous */
> >  #define mtk_phy_update_field(reg, mask, val) \
> >  ({ \
> > -       typeof(mask) mask_ = (mask);    \
> > -       mtk_phy_update_bits(reg, mask_, FIELD_PREP(mask_, val)); \
> > +       BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not
> > constant"); \
> > +       mtk_phy_update_bits(reg, mask, FIELD_PREP(mask, val)); \
> >  })
> > 
> >  #endif
> > --
> > 2.18.0
> > 
> 
>
Chunfeng Yun Jan. 16, 2023, 9:11 a.m. UTC | #5
On Fri, 2023-01-13 at 10:36 -0800, Nick Desaulniers wrote:
> On Wed, Jan 4, 2023 at 5:26 AM Chunfeng Yun <
> chunfeng.yun@mediatek.com> wrote:
> > 
> > Remove the temporary @mask_, this may cause build warning when use
> > clang
> > compiler for powerpc, but can't reproduce it when compile for
> > arm64.
> > the build warning is -Wtautological-constant-out-of-range-compare,
> > and
> > caused by
> > "BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask)"
> 
> Can you please include the text of the observed warning?
> 
> > 
> > After removing @mask_, there is a "CHECK:MACRO_ARG_REUSE" when run
> > checkpatch.pl, due to @mask is constant, no reuse problem will
> > happen.
> > 
> > Fixes: 84513eccd678 ("phy: mediatek: fix build warning of
> > FIELD_PREP()")
> 
> Uh, why was 84513eccd678 sent/reviewed/merged if it didn't fix the
> issue correctly in the first place?
> 
> Is the issue perhaps that your mask isn't wide enough in the first
> place, and should be?  See:
> commit cfd6fb45cfaf ("crypto: ccree - avoid out-of-range warnings
> from clang")
> for inspiration.

I look at this patch, it said that "
This does not happen in other places that just pass a constant here.
"
That's indeed true, no such warning before using FIELD_PREP() due to
the masks are always constant.

So seems that it can fix the waring if avoid using a temporary variable
@mask_ in mtk_phy_update_field() macro.

Thanks a lot

> 
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> Can you please include the Link: tag to the lore URL of the report?
> 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v6: modify the title
> > v5: no changes
> > v4 new patch, I'm not sure it can fix build warning, due to I don't
> > cross compile
> >     it for powerpc using clang in office.
> > ---
> >  drivers/phy/mediatek/phy-mtk-io.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/phy/mediatek/phy-mtk-io.h
> > b/drivers/phy/mediatek/phy-mtk-io.h
> > index d20ad5e5be81..58f06db822cb 100644
> > --- a/drivers/phy/mediatek/phy-mtk-io.h
> > +++ b/drivers/phy/mediatek/phy-mtk-io.h
> > @@ -39,8 +39,8 @@ static inline void mtk_phy_update_bits(void
> > __iomem *reg, u32 mask, u32 val)
> >  /* field @mask shall be constant and continuous */
> >  #define mtk_phy_update_field(reg, mask, val) \
> >  ({ \
> > -       typeof(mask) mask_ = (mask);    \
> > -       mtk_phy_update_bits(reg, mask_, FIELD_PREP(mask_, val)); \
> > +       BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not
> > constant"); \
> > +       mtk_phy_update_bits(reg, mask, FIELD_PREP(mask, val)); \
> >  })
> > 
> >  #endif
> > --
> > 2.18.0
> > 
> 
>
Chunfeng Yun Jan. 31, 2023, 8:57 a.m. UTC | #6
On Fri, 2023-01-13 at 10:36 -0800, Nick Desaulniers wrote:
> On Wed, Jan 4, 2023 at 5:26 AM Chunfeng Yun <
> chunfeng.yun@mediatek.com> wrote:
> > 
> > Remove the temporary @mask_, this may cause build warning when use
> > clang
> > compiler for powerpc, but can't reproduce it when compile for
> > arm64.
> > the build warning is -Wtautological-constant-out-of-range-compare,
> > and
> > caused by
> > "BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask)"
> 
> Can you please include the text of the observed warning?
> 
> > 
> > After removing @mask_, there is a "CHECK:MACRO_ARG_REUSE" when run
> > checkpatch.pl, due to @mask is constant, no reuse problem will
> > happen.
> > 
> > Fixes: 84513eccd678 ("phy: mediatek: fix build warning of
> > FIELD_PREP()")
> 
> Uh, why was 84513eccd678 sent/reviewed/merged if it didn't fix the
> issue correctly in the first place?
> 
> Is the issue perhaps that your mask isn't wide enough in the first
> place, and should be?  See:
> commit cfd6fb45cfaf ("crypto: ccree - avoid out-of-range warnings
> from clang")
> for inspiration.
I look at this patch, it said that "
This does not happen in other places that just pass a constant here.
"
That's indeed true, no such warning before using FIELD_PREP() due to
the masks are always constant.

So seems that it can fix the waring if avoid using a temporary variable
@mask_.

Thanks a lot

> 
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> Can you please include the Link: tag to the lore URL of the report?
> 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v6: modify the title
> > v5: no changes
> > v4 new patch, I'm not sure it can fix build warning, due to I don't
> > cross compile
> >     it for powerpc using clang in office.
> > ---
> >  drivers/phy/mediatek/phy-mtk-io.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/phy/mediatek/phy-mtk-io.h
> > b/drivers/phy/mediatek/phy-mtk-io.h
> > index d20ad5e5be81..58f06db822cb 100644
> > --- a/drivers/phy/mediatek/phy-mtk-io.h
> > +++ b/drivers/phy/mediatek/phy-mtk-io.h
> > @@ -39,8 +39,8 @@ static inline void mtk_phy_update_bits(void
> > __iomem *reg, u32 mask, u32 val)
> >  /* field @mask shall be constant and continuous */
> >  #define mtk_phy_update_field(reg, mask, val) \
> >  ({ \
> > -       typeof(mask) mask_ = (mask);    \
> > -       mtk_phy_update_bits(reg, mask_, FIELD_PREP(mask_, val)); \
> > +       BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not
> > constant"); \
> > +       mtk_phy_update_bits(reg, mask, FIELD_PREP(mask, val)); \
> >  })
> > 
> >  #endif
> > --
> > 2.18.0
> > 
> 
>
diff mbox series

Patch

diff --git a/drivers/phy/mediatek/phy-mtk-io.h b/drivers/phy/mediatek/phy-mtk-io.h
index d20ad5e5be81..58f06db822cb 100644
--- a/drivers/phy/mediatek/phy-mtk-io.h
+++ b/drivers/phy/mediatek/phy-mtk-io.h
@@ -39,8 +39,8 @@  static inline void mtk_phy_update_bits(void __iomem *reg, u32 mask, u32 val)
 /* field @mask shall be constant and continuous */
 #define mtk_phy_update_field(reg, mask, val) \
 ({ \
-	typeof(mask) mask_ = (mask);	\
-	mtk_phy_update_bits(reg, mask_, FIELD_PREP(mask_, val)); \
+	BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not constant"); \
+	mtk_phy_update_bits(reg, mask, FIELD_PREP(mask, val)); \
 })
 
 #endif