[PATCH] pinctrl: generic: Avoid several implicit enum conversions

[PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Nathan Chancellor]

Clang warns when one enumerated type is implicitly converted to another,
which happens several times in the pinctrl drivers for a few reasons:

* The PCONFDUMP macro, which sets the param member in pin_config_item.
* The pinconf_generic_params structure, which is used by drivers to
  configure their bindings, which has a param member like pin_config_item.
* The pinconf_to_config_packed, which takes either the generic enum
  pin_config_param or a specialized one.

Drivers are allowed to extend this enumerated type because of the gap
betweem PIN_CONFIG_END and PIN_CONFIG_MAX. Make it clear to Clang that
this is allowed by changing param's type in all of these instances to
int so no conversion needs to happen.

Link: https://github.com/ClangBuiltLinux/linux/issues/138
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 include/linux/pinctrl/pinconf-generic.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 6c0680641108..13076ae7acfb 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -150,7 +150,7 @@ static inline u32 pinconf_to_config_argument(unsigned long config)
 	return (u32) ((config >> 8) & 0xffffffUL);
 }
 
-static inline unsigned long pinconf_to_config_packed(enum pin_config_param param,
+static inline unsigned long pinconf_to_config_packed(int param,
 						     u32 argument)
 {
 	return PIN_CONF_PACKED(param, argument);
@@ -164,7 +164,7 @@ static inline unsigned long pinconf_to_config_packed(enum pin_config_param param
 	}
 
 struct pin_config_item {
-	const enum pin_config_param param;
+	const int param;
 	const char * const display;
 	const char * const format;
 	bool has_arg;
@@ -180,7 +180,7 @@ struct pinctrl_map;
 
 struct pinconf_generic_params {
 	const char * const property;
-	enum pin_config_param param;
+	int param;
 	u32 default_value;
 };
 
-- 
2.19.0

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Linus Walleij]

On Tue, Sep 25, 2018 at 8:19 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:

> Clang warns when one enumerated type is implicitly converted to another,
> which happens several times in the pinctrl drivers for a few reasons:
>
> * The PCONFDUMP macro, which sets the param member in pin_config_item.
> * The pinconf_generic_params structure, which is used by drivers to
>   configure their bindings, which has a param member like pin_config_item.
> * The pinconf_to_config_packed, which takes either the generic enum
>   pin_config_param or a specialized one.
>
> Drivers are allowed to extend this enumerated type because of the gap
> betweem PIN_CONFIG_END and PIN_CONFIG_MAX. Make it clear to Clang that
> this is allowed by changing param's type in all of these instances to
> int so no conversion needs to happen.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/138
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

I'm not superhappy about this because that enum is great for readability,
even if the static syntax checker is unhappy.

If we can't have an enum here I would argue that we can just as well
remove the enum altogether and just use #define for the config
parameters, would you agree?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Nathan Chancellor]

On Tue, Sep 25, 2018 at 12:58:16PM +0200, Linus Walleij wrote:
> On Tue, Sep 25, 2018 at 8:19 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> 
> > Clang warns when one enumerated type is implicitly converted to another,
> > which happens several times in the pinctrl drivers for a few reasons:
> >
> > * The PCONFDUMP macro, which sets the param member in pin_config_item.
> > * The pinconf_generic_params structure, which is used by drivers to
> >   configure their bindings, which has a param member like pin_config_item.
> > * The pinconf_to_config_packed, which takes either the generic enum
> >   pin_config_param or a specialized one.
> >
> > Drivers are allowed to extend this enumerated type because of the gap
> > betweem PIN_CONFIG_END and PIN_CONFIG_MAX. Make it clear to Clang that
> > this is allowed by changing param's type in all of these instances to
> > int so no conversion needs to happen.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/138
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> 
> I'm not superhappy about this because that enum is great for readability,
> even if the static syntax checker is unhappy.
> 
> If we can't have an enum here I would argue that we can just as well
> remove the enum altogether and just use #define for the config
> parameters, would you agree?
> 
> Yours,
> Linus Walleij

Hi Linus,

I see no reason to get rid of the enums. All this patch should do is
silence Clang's warnings because there is no other way to tell it that
this construct is okay except for changing the parameter/member type to
int (so no conversion needs to happen) or an explicit cast, which will
result in more noise in my opinion since this warning happens at least
10-15 times in the pinctrl drivers. There's no fundamental change to
how the enums are used.

See these other commits for similar fixes:

3eb95feac113 ("mm/zsmalloc.c: change stat type parameter to int")
04fecbf51b3c ("mm: memcontrol: use int for event/state parameter in several functions")

Cheers!
Nathan

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Nick Desaulniers]

On Tue, Sep 25, 2018 at 9:15 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Sep 25, 2018 at 12:58:16PM +0200, Linus Walleij wrote:
> > On Tue, Sep 25, 2018 at 8:19 AM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> >
> > > Clang warns when one enumerated type is implicitly converted to another,
> > > which happens several times in the pinctrl drivers for a few reasons:
> > >
> > > * The PCONFDUMP macro, which sets the param member in pin_config_item.
> > > * The pinconf_generic_params structure, which is used by drivers to
> > >   configure their bindings, which has a param member like pin_config_item.
> > > * The pinconf_to_config_packed, which takes either the generic enum
> > >   pin_config_param or a specialized one.
> > >
> > > Drivers are allowed to extend this enumerated type because of the gap
> > > betweem PIN_CONFIG_END and PIN_CONFIG_MAX. Make it clear to Clang that
> > > this is allowed by changing param's type in all of these instances to
> > > int so no conversion needs to happen.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/138
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> >
> > I'm not superhappy about this because that enum is great for readability,
> > even if the static syntax checker is unhappy.
> >
> > If we can't have an enum here I would argue that we can just as well
> > remove the enum altogether and just use #define for the config
> > parameters, would you agree?
> >
> > Yours,
> > Linus Walleij
>
> Hi Linus,
>
> I see no reason to get rid of the enums. All this patch should do is
> silence Clang's warnings because there is no other way to tell it that
> this construct is okay except for changing the parameter/member type to
> int (so no conversion needs to happen) or an explicit cast, which will
> result in more noise in my opinion since this warning happens at least
> 10-15 times in the pinctrl drivers. There's no fundamental change to
> how the enums are used.
>
> See these other commits for similar fixes:
>
> 3eb95feac113 ("mm/zsmalloc.c: change stat type parameter to int")
> 04fecbf51b3c ("mm: memcontrol: use int for event/state parameter in several functions")
>
> Cheers!
> Nathan

That or all of the call sites should have explicit casts to the base enum.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Nathan Chancellor]

On Tue, Sep 25, 2018 at 09:23:09AM -0700, Nick Desaulniers wrote:
> On Tue, Sep 25, 2018 at 9:15 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Tue, Sep 25, 2018 at 12:58:16PM +0200, Linus Walleij wrote:
> > > On Tue, Sep 25, 2018 at 8:19 AM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > >
> > > > Clang warns when one enumerated type is implicitly converted to another,
> > > > which happens several times in the pinctrl drivers for a few reasons:
> > > >
> > > > * The PCONFDUMP macro, which sets the param member in pin_config_item.
> > > > * The pinconf_generic_params structure, which is used by drivers to
> > > >   configure their bindings, which has a param member like pin_config_item.
> > > > * The pinconf_to_config_packed, which takes either the generic enum
> > > >   pin_config_param or a specialized one.
> > > >
> > > > Drivers are allowed to extend this enumerated type because of the gap
> > > > betweem PIN_CONFIG_END and PIN_CONFIG_MAX. Make it clear to Clang that
> > > > this is allowed by changing param's type in all of these instances to
> > > > int so no conversion needs to happen.
> > > >
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/138
> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > >
> > > I'm not superhappy about this because that enum is great for readability,
> > > even if the static syntax checker is unhappy.
> > >
> > > If we can't have an enum here I would argue that we can just as well
> > > remove the enum altogether and just use #define for the config
> > > parameters, would you agree?
> > >
> > > Yours,
> > > Linus Walleij
> >
> > Hi Linus,
> >
> > I see no reason to get rid of the enums. All this patch should do is
> > silence Clang's warnings because there is no other way to tell it that
> > this construct is okay except for changing the parameter/member type to
> > int (so no conversion needs to happen) or an explicit cast, which will
> > result in more noise in my opinion since this warning happens at least
> > 10-15 times in the pinctrl drivers. There's no fundamental change to
> > how the enums are used.
> >
> > See these other commits for similar fixes:
> >
> > 3eb95feac113 ("mm/zsmalloc.c: change stat type parameter to int")
> > 04fecbf51b3c ("mm: memcontrol: use int for event/state parameter in several functions")
> >
> > Cheers!
> > Nathan
> 
> That or all of the call sites should have explicit casts to the base enum.
> 

I don't know why I never replied to this...

In my opinion, there are enough of these warnings to warrant changing
the type of param globally (arm64 allyesconfig):

drivers/pinctrl/bcm/pinctrl-bcm2835.c:707:40: warning: implicit conversion from enumeration type 'enum bcm2835_pinconf_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/sprd/pinctrl-sprd.c:845:19: warning: implicit conversion from enumeration type 'enum sprd_pinconf_params' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/sprd/pinctrl-sprd.c:846:22: warning: implicit conversion from enumeration type 'enum sprd_pinconf_params' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/sprd/pinctrl-sprd.c:851:12: warning: implicit conversion from enumeration type 'enum sprd_pinconf_params' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/sprd/pinctrl-sprd.c:852:12: warning: implicit conversion from enumeration type 'enum sprd_pinconf_params' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-max77620.c:56:12: warning: implicit conversion from enumeration type 'enum max77620_pinconf_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-max77620.c:59:12: warning: implicit conversion from enumeration type 'enum max77620_pinconf_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-max77620.c:62:12: warning: implicit conversion from enumeration type 'enum max77620_pinconf_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-max77620.c:65:12: warning: implicit conversion from enumeration type 'enum max77620_pinconf_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-max77620.c:68:12: warning: implicit conversion from enumeration type 'enum max77620_pinconf_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-max77620.c:71:12: warning: implicit conversion from enumeration type 'enum max77620_pinconf_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-lpc18xx.c:643:29: warning: implicit conversion from enumeration type 'enum lpc18xx_pin_config_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/pinctrl/pinctrl-lpc18xx.c:648:12: warning: implicit conversion from enumeration type 'enum lpc18xx_pin_config_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/rtc/rtc-omap.c:574:21: warning: implicit conversion from enumeration type 'enum rtc_pin_config_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]
drivers/rtc/rtc-omap.c:579:12: warning: implicit conversion from enumeration type 'enum rtc_pin_config_param' to different enumeration type 'enum pin_config_param' [-Wenum-conversion]

The construct of extending pin_config_param with another enumerated type
is something that could be added in new drivers so there would be upkeep
of adding explicit casts if that happens, at least until something like
0day can add support for Clang so driver authors get alerted of these
warnings. Even that doesn't sound like a particularly welcoming model
to me.

Linus, did you have any other objections to this patch given my
reasoning in these past couple of emails or would you like me to try
adding explicit casts to all of these call sites? For example, changing
pinctrl-sprd.c would look something like the diff below (I personally
think this looks kind of ugly but I'm willing to do whatever the
maintainer is comfortable with in order to fix these warnings).

Let me know what you think,
Nathan

======================================================================================

diff --git a/drivers/pinctrl/sprd/pinctrl-sprd.c b/drivers/pinctrl/sprd/pinctrl-sprd.c
index 4537b5453996..9c82ba4dceff 100644
--- a/drivers/pinctrl/sprd/pinctrl-sprd.c
+++ b/drivers/pinctrl/sprd/pinctrl-sprd.c
@@ -842,14 +842,14 @@ static const struct pinconf_ops sprd_pinconf_ops = {
 };
 
 static const struct pinconf_generic_params sprd_dt_params[] = {
-       {"sprd,control", SPRD_PIN_CONFIG_CONTROL, 0},
-       {"sprd,sleep-mode", SPRD_PIN_CONFIG_SLEEP_MODE, 0},
+       {"sprd,control", (enum pin_config_param)SPRD_PIN_CONFIG_CONTROL, 0},
+       {"sprd,sleep-mode", (enum pin_config_param)SPRD_PIN_CONFIG_SLEEP_MODE, 0},
 };
 
 #ifdef CONFIG_DEBUG_FS
 static const struct pin_config_item sprd_conf_items[] = {
-       PCONFDUMP(SPRD_PIN_CONFIG_CONTROL, "global control", NULL, true),
-       PCONFDUMP(SPRD_PIN_CONFIG_SLEEP_MODE, "sleep mode", NULL, true),
+       PCONFDUMP((enum pin_config_param)SPRD_PIN_CONFIG_CONTROL, "global control", NULL, true),
+       PCONFDUMP((enum pin_config_param)SPRD_PIN_CONFIG_SLEEP_MODE, "sleep mode", NULL, true),
 };
 #endif
 
> -- 
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Linus Walleij]

On Thu, Oct 25, 2018 at 11:04 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:

> In my opinion, there are enough of these warnings to warrant changing
> the type of param globally (arm64 allyesconfig):

Yeah as it is from the compiler, sure we need to get rid of it.

> Linus, did you have any other objections to this patch given my
> reasoning in these past couple of emails or would you like me to try
> adding explicit casts to all of these call sites?

I would favor the model to:

1. Replace all occurences of enum pin_config_param with unsigned
   int.

2. Replace the whole definition of enum pin_config_param with
   #define PIN_CONFIG_BIAS_BUS_HOLD 0
   #define PIN_CONFIG_BIAS_DISABLE 1
   etc etc.

I think it is not a good idea to try to do both at the same time.

A slightly lesser evil variant is to add a few PIN_CONFIG_CUSTOM_1
PIN_CONFIG_CUSTOM_2 etc at the end of the enum and just
#define MY_CONFIG PIN_CONFIG_CUSTOM_1
in all drivers that use these.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Nathan Chancellor]

On Wed, Oct 31, 2018 at 02:33:24PM +0100, Linus Walleij wrote:
> On Thu, Oct 25, 2018 at 11:04 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> 
> > In my opinion, there are enough of these warnings to warrant changing
> > the type of param globally (arm64 allyesconfig):
> 
> Yeah as it is from the compiler, sure we need to get rid of it.
> 
> > Linus, did you have any other objections to this patch given my
> > reasoning in these past couple of emails or would you like me to try
> > adding explicit casts to all of these call sites?
> 
> I would favor the model to:
> 
> 1. Replace all occurences of enum pin_config_param with unsigned
>    int.
> 

Given that there are 57 files with this definition and some include it
multiple time, this doesn't seem super reasonable in my opinion.

> 2. Replace the whole definition of enum pin_config_param with
>    #define PIN_CONFIG_BIAS_BUS_HOLD 0
>    #define PIN_CONFIG_BIAS_DISABLE 1
>    etc etc.
> 
> I think it is not a good idea to try to do both at the same time.
> 
> A slightly lesser evil variant is to add a few PIN_CONFIG_CUSTOM_1
> PIN_CONFIG_CUSTOM_2 etc at the end of the enum and just
> #define MY_CONFIG PIN_CONFIG_CUSTOM_1
> in all drivers that use these.
> 

Some drivers actually just define their pin config params like:

#define VAR (PIN_CONFIG_END + 1)

In fact, more drivers do that than not. I will go ahead and draft some
patches tonight and send them out tonight to see what driver authors
think.

Example with drivers/pinctrl/sprd/pinctrl-sprd.c

================================================

diff --git a/drivers/pinctrl/sprd/pinctrl-sprd.c b/drivers/pinctrl/sprd/pinctrl-sprd.c
index 4537b5453996..7d9a44bd0047 100644
--- a/drivers/pinctrl/sprd/pinctrl-sprd.c
+++ b/drivers/pinctrl/sprd/pinctrl-sprd.c
@@ -159,10 +159,8 @@ struct sprd_pinctrl {
        struct sprd_pinctrl_soc_info *info;
 };
 
-enum sprd_pinconf_params {
-       SPRD_PIN_CONFIG_CONTROL = PIN_CONFIG_END + 1,
-       SPRD_PIN_CONFIG_SLEEP_MODE = PIN_CONFIG_END + 2,
-};
+#define SPRD_PIN_CONFIG_CONTROL                (PIN_CONFIG_END + 1)
+#define SPRD_PIN_CONFIG_SLEEP_MODE     (PIN_CONFIG_END + 2)
 
 static int sprd_pinctrl_get_id_by_name(struct sprd_pinctrl *sprd_pctl,
                                       const char *name)

> Yours,
> Linus Walleij

Thank you for the reply and advice!
Nathan

RE: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[David Laight]

From: Nathan Chancellor
> Sent: 01 November 2018 00:04
...
> > A slightly lesser evil variant is to add a few PIN_CONFIG_CUSTOM_1
> > PIN_CONFIG_CUSTOM_2 etc at the end of the enum and just
> > #define MY_CONFIG PIN_CONFIG_CUSTOM_1
> > in all drivers that use these.
> >
> 
> Some drivers actually just define their pin config params like:
> 
> #define VAR (PIN_CONFIG_END + 1)

You probably want to add 'custom' definitions after PIN_CONFIG_END
so that a future compiler versions doesn't generate an error
because (PIN_CONFIG_END + 1) isn't a valid value for the enum.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Linus Walleij]

On Thu, Nov 1, 2018 at 1:03 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> [Me]
> > A slightly lesser evil variant is to add a few PIN_CONFIG_CUSTOM_1
> > PIN_CONFIG_CUSTOM_2 etc at the end of the enum and just
> > #define MY_CONFIG PIN_CONFIG_CUSTOM_1
> > in all drivers that use these.
> >
>
> Some drivers actually just define their pin config params like:
>
> #define VAR (PIN_CONFIG_END + 1)
>
> In fact, more drivers do that than not. I will go ahead and draft some
> patches tonight and send them out tonight to see what driver authors
> think.

This seems to work. Is your kernel compile working without
warnings after this round of patches?

Thanks for driving this BTW.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: generic: Avoid several implicit enum conversions

[Nathan Chancellor]

On Fri, Nov 09, 2018 at 10:29:02AM +0100, Linus Walleij wrote:
> On Thu, Nov 1, 2018 at 1:03 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > [Me]
> > > A slightly lesser evil variant is to add a few PIN_CONFIG_CUSTOM_1
> > > PIN_CONFIG_CUSTOM_2 etc at the end of the enum and just
> > > #define MY_CONFIG PIN_CONFIG_CUSTOM_1
> > > in all drivers that use these.
> > >
> >
> > Some drivers actually just define their pin config params like:
> >
> > #define VAR (PIN_CONFIG_END + 1)
> >
> > In fact, more drivers do that than not. I will go ahead and draft some
> > patches tonight and send them out tonight to see what driver authors
> > think.
> 
> This seems to work. Is your kernel compile working without
> warnings after this round of patches?
> 

Yes, there are no more enum-conversion warnings and the tree shows there
are no more enums that use PIN_CONFIG_END.

> Thanks for driving this BTW.
> 

Thank you for being patient and forcing me to come up with a solution
that works for you!

Nathan

> Yours,
> Linus Walleij