Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 06:39 AM, Uwe Kleine-König wrote:

On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:

On 11/01/2017 11:52, Guenter Roeck wrote:


On 01/11/2017 01:07 AM, Marc Gonzalez wrote:


@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;


This looks wrong. There is no clk_unprepare_disable when
devm_add_action_or_reset fails.



Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?


Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.


More complicated, because the cleanup function will have to be deleted later?
The compiler will warn if someone forgets to do that.

In my opinion, it's not a good idea to rely on the fact that casting
void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
on most platforms. (It has undefined behavior, strictly speaking.)


I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
there couldn't be found a better solution.

If in the end it looks like the following that would be good I think:

clk = devm_clk_get(...);
if (IS_ERR(clk))
...

ret = devm_clk_prepare_enable(clk)
if (ret)
return ret;



It turns out that at least one static analyzer complains about different
parameter pointer types in situations like this, and at least one embedded
compiler manages to create function names with embedded parameter type
(eg it appends an 'i' to the function name for each integer parameter).

With that, I consider the typecast to be too risky after all. It may work
for all of today's Linux architectures and compilers, but who knows if I
get flooded with static analyzer warnings, and who knows if gcc version
18.0 or binutils 35.0 makes it truly incompatible (following the logic of
"we can, therefore we do"). Since I also dislike the stub function solution,
at least in this situation, I'll drop all patches touching clk_prepare_enable(),
and wait for devm_clk_prepare_enable() to be available.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 06:39 AM, Uwe Kleine-König wrote:

On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:

On 11/01/2017 11:52, Guenter Roeck wrote:


On 01/11/2017 01:07 AM, Marc Gonzalez wrote:


@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;


This looks wrong. There is no clk_unprepare_disable when
devm_add_action_or_reset fails.



Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?


Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.


More complicated, because the cleanup function will have to be deleted later?
The compiler will warn if someone forgets to do that.

In my opinion, it's not a good idea to rely on the fact that casting
void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
on most platforms. (It has undefined behavior, strictly speaking.)


I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
there couldn't be found a better solution.

If in the end it looks like the following that would be good I think:

clk = devm_clk_get(...);
if (IS_ERR(clk))
...

ret = devm_clk_prepare_enable(clk)
if (ret)
return ret;



It turns out that at least one static analyzer complains about different
parameter pointer types in situations like this, and at least one embedded
compiler manages to create function names with embedded parameter type
(eg it appends an 'i' to the function name for each integer parameter).

With that, I consider the typecast to be too risky after all. It may work
for all of today's Linux architectures and compilers, but who knows if I
get flooded with static analyzer warnings, and who knows if gcc version
18.0 or binutils 35.0 makes it truly incompatible (following the logic of
"we can, therefore we do"). Since I also dislike the stub function solution,
at least in this situation, I'll drop all patches touching clk_prepare_enable(),
and wait for devm_clk_prepare_enable() to be available.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 12/01/2017 12:24, Måns Rullgård wrote:

> Marc Gonzalez writes:
> 
>>> So far we have a claim that a cast to a void * may somehow be different
>>> to a cast to a different pointer, if used as function argument, and that
>>> the behavior with such a cast may be undefined. In other words, you claim
>>> that a function implemented as, say,
>>>
>>>void func(int *var) {}
>>>
>>> might result in undefined behavior if some header file declares it as
>>>
>>> void func(void *);
>>>
>>> and it is called as
>>>
>>> int var;
>>>
>>> func();
>>>
>>> That seems really far fetched to me.
>>
>> Thanks for giving me an opportunity to play the language lawyer :-)
>>
>> C99 6.3.2.3 sub-clause 8 states:
>>
>> "A pointer to a function of one type may be converted to a pointer to
>> a function of another type and back again; the result shall compare
>> equal to the original pointer. If a converted pointer is used to call
>> a function whose type is not compatible with the pointed-to type, the
>> behavior is undefined."
>>
>> So, the behavior is undefined, not when you cast clk_disable_unprepare,
>> but when clk_disable_unprepare is later called through the devres->action
>> function pointer.
> 
> Only if the function types are incompatible.  C99 6.7.5.3 subclause 15:
> 
>   For two function types to be compatible, both shall specify compatible
>   return types.  Moreover, the parameter type lists, if both are
>   present, shall agree in the number of parameters and in use of the
>   ellipsis terminator; corresponding parameters shall have compatible
>   types.
> 
> The question then is whether pointer to void and pointer to struct clk
> are compatible types.

6.2.7 Compatible type and composite type
sub-clause 1

"Two types have compatible type if their types are the same. Additional rules 
for
determining whether two types are compatible are described in 6.7.2 for type 
specifiers,
in 6.7.3 for type qualifiers, and in 6.7.5 for declarators."

6.7.5.1 Pointer declarators
sub-clause 2

"For two pointer types to be compatible, both shall be identically qualified 
and both shall
be pointers to compatible types."

I don't think void and struct clk are compatible types.
AFAIU, conversion and compatibility are two separate subjects.

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 12/01/2017 12:24, Måns Rullgård wrote:

> Marc Gonzalez writes:
> 
>>> So far we have a claim that a cast to a void * may somehow be different
>>> to a cast to a different pointer, if used as function argument, and that
>>> the behavior with such a cast may be undefined. In other words, you claim
>>> that a function implemented as, say,
>>>
>>>void func(int *var) {}
>>>
>>> might result in undefined behavior if some header file declares it as
>>>
>>> void func(void *);
>>>
>>> and it is called as
>>>
>>> int var;
>>>
>>> func();
>>>
>>> That seems really far fetched to me.
>>
>> Thanks for giving me an opportunity to play the language lawyer :-)
>>
>> C99 6.3.2.3 sub-clause 8 states:
>>
>> "A pointer to a function of one type may be converted to a pointer to
>> a function of another type and back again; the result shall compare
>> equal to the original pointer. If a converted pointer is used to call
>> a function whose type is not compatible with the pointed-to type, the
>> behavior is undefined."
>>
>> So, the behavior is undefined, not when you cast clk_disable_unprepare,
>> but when clk_disable_unprepare is later called through the devres->action
>> function pointer.
> 
> Only if the function types are incompatible.  C99 6.7.5.3 subclause 15:
> 
>   For two function types to be compatible, both shall specify compatible
>   return types.  Moreover, the parameter type lists, if both are
>   present, shall agree in the number of parameters and in use of the
>   ellipsis terminator; corresponding parameters shall have compatible
>   types.
> 
> The question then is whether pointer to void and pointer to struct clk
> are compatible types.

6.2.7 Compatible type and composite type
sub-clause 1

"Two types have compatible type if their types are the same. Additional rules 
for
determining whether two types are compatible are described in 6.7.2 for type 
specifiers,
in 6.7.3 for type qualifiers, and in 6.7.5 for declarators."

6.7.5.1 Pointer declarators
sub-clause 2

"For two pointer types to be compatible, both shall be identically qualified 
and both shall
be pointers to compatible types."

I don't think void and struct clk are compatible types.
AFAIU, conversion and compatibility are two separate subjects.

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Måns Rullgård]

Marc Gonzalez  writes:

>> So far we have a claim that a cast to a void * may somehow be different
>> to a cast to a different pointer, if used as function argument, and that
>> the behavior with such a cast may be undefined. In other words, you claim
>> that a function implemented as, say,
>> 
>>void func(int *var) {}
>> 
>> might result in undefined behavior if some header file declares it as
>> 
>> void func(void *);
>> 
>> and it is called as
>> 
>> int var;
>> 
>> func();
>> 
>> That seems really far fetched to me.
>
> Thanks for giving me an opportunity to play the language lawyer :-)
>
> C99 6.3.2.3 sub-clause 8 states:
>
> "A pointer to a function of one type may be converted to a pointer to
> a function of another type and back again; the result shall compare
> equal to the original pointer. If a converted pointer is used to call
> a function whose type is not compatible with the pointed-to type, the
> behavior is undefined."
>
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.

Only if the function types are incompatible.  C99 6.7.5.3 subclause 15:

  For two function types to be compatible, both shall specify compatible
  return types.  Moreover, the parameter type lists, if both are
  present, shall agree in the number of parameters and in use of the
  ellipsis terminator; corresponding parameters shall have compatible
  types.

The question then is whether pointer to void and pointer to struct clk
are compatible types.  6.7.5.1 subclause 2:

  For two pointer types to be compatible, both shall be identically
  qualified and both shall be pointers to compatible types.

6.2.5 subclause 27:

  A pointer to void shall have the same representation and alignment
  requirements as a pointer to a character type. 39)

  39) The same representation and alignment requirements are meant to
  imply interchangeability as arguments to functions, return values
  from functions, and members of unions.

6.3.2.3 subclause 1:

  A pointer to void may be converted to or from a pointer to any
  incomplete or object type.

>From what I can tell, the standard stops just short of declaring pointer
to void compatible with other pointer types.

> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).

Yes, I don't see how it could possibly go wrong.

-- 
Måns Rullgård

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Måns Rullgård]

Marc Gonzalez  writes:

>> So far we have a claim that a cast to a void * may somehow be different
>> to a cast to a different pointer, if used as function argument, and that
>> the behavior with such a cast may be undefined. In other words, you claim
>> that a function implemented as, say,
>> 
>>void func(int *var) {}
>> 
>> might result in undefined behavior if some header file declares it as
>> 
>> void func(void *);
>> 
>> and it is called as
>> 
>> int var;
>> 
>> func();
>> 
>> That seems really far fetched to me.
>
> Thanks for giving me an opportunity to play the language lawyer :-)
>
> C99 6.3.2.3 sub-clause 8 states:
>
> "A pointer to a function of one type may be converted to a pointer to
> a function of another type and back again; the result shall compare
> equal to the original pointer. If a converted pointer is used to call
> a function whose type is not compatible with the pointed-to type, the
> behavior is undefined."
>
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.

Only if the function types are incompatible.  C99 6.7.5.3 subclause 15:

  For two function types to be compatible, both shall specify compatible
  return types.  Moreover, the parameter type lists, if both are
  present, shall agree in the number of parameters and in use of the
  ellipsis terminator; corresponding parameters shall have compatible
  types.

The question then is whether pointer to void and pointer to struct clk
are compatible types.  6.7.5.1 subclause 2:

  For two pointer types to be compatible, both shall be identically
  qualified and both shall be pointers to compatible types.

6.2.5 subclause 27:

  A pointer to void shall have the same representation and alignment
  requirements as a pointer to a character type. 39)

  39) The same representation and alignment requirements are meant to
  imply interchangeability as arguments to functions, return values
  from functions, and members of unions.

6.3.2.3 subclause 1:

  A pointer to void may be converted to or from a pointer to any
  incomplete or object type.

>From what I can tell, the standard stops just short of declaring pointer
to void compatible with other pointer types.

> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).

Yes, I don't see how it could possibly go wrong.

-- 
Måns Rullgård

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Uwe Kleine-König]

On Thu, Jan 12, 2017 at 10:44:07AM +0100, Marc Gonzalez wrote:
> On 11/01/2017 18:51, Guenter Roeck wrote:
> 
> > However, some other unrelated undefined behavior does not mean that this
> > specific behavior is undefined.
> 
> True :-)
> 
> Let me just give two additional examples of UB that /have/ bitten
> Linux kernel devs.
> 
> int i;
> for (i = 1; i > 0; ++i)
>   /* do_something(); */
> 
> => optimized into an infinite loop
> 
> and
> 
> void func(struct foo *p) {
>   int n = p->field;
>   if (!p) return;
> 
> => null-pointer check optimized away
> 
> > So far we have a claim that a cast to a void * may somehow be different
> > to a cast to a different pointer, if used as function argument, and that
> > the behavior with such a cast may be undefined. In other words, you claim
> > that a function implemented as, say,
> > 
> >void func(int *var) {}
> > 
> > might result in undefined behavior if some header file declares it as
> > 
> > void func(void *);
> > 
> > and it is called as
> > 
> > int var;
> > 
> > func();
> > 
> > That seems really far fetched to me.
> 
> Thanks for giving me an opportunity to play the language lawyer :-)
> 
> C99 6.3.2.3 sub-clause 8 states:
> 
> "A pointer to a function of one type may be converted to a pointer to a 
> function of another
> type and back again; the result shall compare equal to the original pointer. 
> If a converted
> pointer is used to call a function whose type is not compatible with the 
> pointed-to type,
> the behavior is undefined."
> 
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.
> 
> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).
> 
> > I do get the message that you do not like this kind of cast. But that 
> > doesn't
> > mean it is not correct.
> 
> If it's already widely used in the kernel, it seems there is no point
> fighting it ;-)

I'd say +.5 here (where +1 is an ack). My approach would be to push
devm_clk_prepare_enable and use that. It cannot be that hard, can it?

It looks prettier, is well defined, easier to fit into 80 chars per
line. I wonder why not everybody jubilates on this new function.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Uwe Kleine-König]

On Thu, Jan 12, 2017 at 10:44:07AM +0100, Marc Gonzalez wrote:
> On 11/01/2017 18:51, Guenter Roeck wrote:
> 
> > However, some other unrelated undefined behavior does not mean that this
> > specific behavior is undefined.
> 
> True :-)
> 
> Let me just give two additional examples of UB that /have/ bitten
> Linux kernel devs.
> 
> int i;
> for (i = 1; i > 0; ++i)
>   /* do_something(); */
> 
> => optimized into an infinite loop
> 
> and
> 
> void func(struct foo *p) {
>   int n = p->field;
>   if (!p) return;
> 
> => null-pointer check optimized away
> 
> > So far we have a claim that a cast to a void * may somehow be different
> > to a cast to a different pointer, if used as function argument, and that
> > the behavior with such a cast may be undefined. In other words, you claim
> > that a function implemented as, say,
> > 
> >void func(int *var) {}
> > 
> > might result in undefined behavior if some header file declares it as
> > 
> > void func(void *);
> > 
> > and it is called as
> > 
> > int var;
> > 
> > func();
> > 
> > That seems really far fetched to me.
> 
> Thanks for giving me an opportunity to play the language lawyer :-)
> 
> C99 6.3.2.3 sub-clause 8 states:
> 
> "A pointer to a function of one type may be converted to a pointer to a 
> function of another
> type and back again; the result shall compare equal to the original pointer. 
> If a converted
> pointer is used to call a function whose type is not compatible with the 
> pointed-to type,
> the behavior is undefined."
> 
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.
> 
> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).
> 
> > I do get the message that you do not like this kind of cast. But that 
> > doesn't
> > mean it is not correct.
> 
> If it's already widely used in the kernel, it seems there is no point
> fighting it ;-)

I'd say +.5 here (where +1 is an ack). My approach would be to push
devm_clk_prepare_enable and use that. It cannot be that hard, can it?

It looks prettier, is well defined, easier to fit into 80 chars per
line. I wonder why not everybody jubilates on this new function.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 18:51, Guenter Roeck wrote:

> However, some other unrelated undefined behavior does not mean that this
> specific behavior is undefined.

True :-)

Let me just give two additional examples of UB that /have/ bitten
Linux kernel devs.

int i;
for (i = 1; i > 0; ++i)
/* do_something(); */

=> optimized into an infinite loop

and

void func(struct foo *p) {
int n = p->field;
if (!p) return;

=> null-pointer check optimized away

> So far we have a claim that a cast to a void * may somehow be different
> to a cast to a different pointer, if used as function argument, and that
> the behavior with such a cast may be undefined. In other words, you claim
> that a function implemented as, say,
> 
>void func(int *var) {}
> 
> might result in undefined behavior if some header file declares it as
> 
> void func(void *);
> 
> and it is called as
> 
> int var;
> 
> func();
> 
> That seems really far fetched to me.

Thanks for giving me an opportunity to play the language lawyer :-)

C99 6.3.2.3 sub-clause 8 states:

"A pointer to a function of one type may be converted to a pointer to a 
function of another
type and back again; the result shall compare equal to the original pointer. If 
a converted
pointer is used to call a function whose type is not compatible with the 
pointed-to type,
the behavior is undefined."

So, the behavior is undefined, not when you cast clk_disable_unprepare,
but when clk_disable_unprepare is later called through the devres->action
function pointer.

However, I agree that it will work as expected on typical platforms
(where all pointers are the same size, and the calling convention
treats all pointers the same).

> I do get the message that you do not like this kind of cast. But that doesn't
> mean it is not correct.

If it's already widely used in the kernel, it seems there is no point
fighting it ;-)

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 18:51, Guenter Roeck wrote:

> However, some other unrelated undefined behavior does not mean that this
> specific behavior is undefined.

True :-)

Let me just give two additional examples of UB that /have/ bitten
Linux kernel devs.

int i;
for (i = 1; i > 0; ++i)
/* do_something(); */

=> optimized into an infinite loop

and

void func(struct foo *p) {
int n = p->field;
if (!p) return;

=> null-pointer check optimized away

> So far we have a claim that a cast to a void * may somehow be different
> to a cast to a different pointer, if used as function argument, and that
> the behavior with such a cast may be undefined. In other words, you claim
> that a function implemented as, say,
> 
>void func(int *var) {}
> 
> might result in undefined behavior if some header file declares it as
> 
> void func(void *);
> 
> and it is called as
> 
> int var;
> 
> func();
> 
> That seems really far fetched to me.

Thanks for giving me an opportunity to play the language lawyer :-)

C99 6.3.2.3 sub-clause 8 states:

"A pointer to a function of one type may be converted to a pointer to a 
function of another
type and back again; the result shall compare equal to the original pointer. If 
a converted
pointer is used to call a function whose type is not compatible with the 
pointed-to type,
the behavior is undefined."

So, the behavior is undefined, not when you cast clk_disable_unprepare,
but when clk_disable_unprepare is later called through the devres->action
function pointer.

However, I agree that it will work as expected on typical platforms
(where all pointers are the same size, and the calling convention
treats all pointers the same).

> I do get the message that you do not like this kind of cast. But that doesn't
> mean it is not correct.

If it's already widely used in the kernel, it seems there is no point
fighting it ;-)

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 04:12 PM, Andy Shevchenko wrote:

On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck  wrote:

On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.

Actually what is the status to the patch series which brings devm_clk
stuff like prepare_enable()? It was submitted 2(?) years ago.



It stalled.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 04:12 PM, Andy Shevchenko wrote:

On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck  wrote:

On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.

Actually what is the status to the patch series which brings devm_clk
stuff like prepare_enable()? It was submitted 2(?) years ago.



It stalled.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Andy Shevchenko]

On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck  wrote:
> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> Not really. It would just make it more complicated to replace the
> call with devm_clk_prepare_enable(), should it ever find its way
> into the light of day.
Actually what is the status to the patch series which brings devm_clk
stuff like prepare_enable()? It was submitted 2(?) years ago.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Andy Shevchenko]

On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck  wrote:
> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> Not really. It would just make it more complicated to replace the
> call with devm_clk_prepare_enable(), should it ever find its way
> into the light of day.
Actually what is the status to the patch series which brings devm_clk
stuff like prepare_enable()? It was submitted 2(?) years ago.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On Wed, Jan 11, 2017 at 04:28:12PM +0100, Marc Gonzalez wrote:
> On 11/01/2017 15:25, Guenter Roeck wrote:
> > On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
> >> On 11/01/2017 11:52, Guenter Roeck wrote:
> >>
> >>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> >>>
> > @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct 
> > platform_device *pdev)
> > err = clk_prepare_enable(dev->clk);
> > if (err)
> > return err;
> > +   err = devm_add_action_or_reset(>dev,
> > +  (void(*)(void 
> > *))clk_disable_unprepare,
> > +  dev->clk);
> > +   if (err)
> > +   return err;
> 
>  Hello Guenter,
> 
>  I would rather avoid the function pointer cast.
>  How about defining an auxiliary function for the cleanup action?
> 
>  clk_disable_unprepare() is static inline, so gcc will have to
>  define an auxiliary function either way. What do you think?
> >>>
> >>> Not really. It would just make it more complicated to replace the
> >>> call with devm_clk_prepare_enable(), should it ever find its way
> >>> into the light of day.
> >>
> >> More complicated, because the cleanup function will have to be deleted 
> >> later?
> >> The compiler will warn if someone forgets to do that.
> >>
> >> In my opinion, it's not a good idea to rely on the fact that casting
> >> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> >> on most platforms. (It has undefined behavior, strictly speaking.)
> >
> > I do hear that you object to this code.
> > 
> > However, I must admit that you completely lost me here. It is a cast from
> > one function pointer to another,
> 
> Perhaps you are used to work at the assembly level, where pointers are
> just addresses, and all pointers are interchangeable.
> 
> At a slightly higher level (C abstract machine), it is not so.
> 
> > passed as argument to another function,
> > with a secondary cast of its argument from a typed pointer to a void 
> > pointer.
> > I don't think C permits for "undefined behavior, strictly speaking".
> 
> The C standard leaves quite a lot of behavior undefined, e.g.
> 
> char *foo = "hello";
> foo[1] = 'a'; // UB
> 
> char buf[4];
> *(int *) = 0xdeadbeef; // UB
> 
> 1 << 64; // UB
> 
Ah, yes, I stand corrected.

However, some other unrelated undefined behavior does not mean that this
specific behavior is undefined.

So far we have a claim that a cast to a void * may somehow be different
to a cast to a different pointer, if used as function argument, and that
the behavior with such a cast may be undefined. In other words, you claim
that a function implemented as, say,

   void func(int *var) {}

might result in undefined behavior if some header file declares it as

void func(void *);

and it is called as

int var;

func();

That seems really far fetched to me.

I do get the message that you do not like this kind of cast. But that doesn't
mean it is not correct.

> > Besides, that same mechanism is already used elsewhere, which is how I
> > got the idea. Are you claiming that there are situations where it won't
> > work ?
> 
> If this technique is already used elsewhere in the kernel, then I'll
> crawl back under my rock (and weep).
> 

git grep "(void(\*)(void \*))"

and variants thereof:

git grep "(void(\*)"

> I can see two issues with the code you propose.
> 
> First is the same for all casts: silencing potential warnings,
> e.g. if the prototype of clk_disable_unprepare ever changed.
> (Though casts are required for vararg function arguments.)
> 
Understood. However, one should really hope that anyone changing
an API has a look at all its callers and does not just pray that
there are no problems.

> Second is just theory and not a real-world concern.
> 
> >> Do you really dislike the portable solution I suggested? :-(
> >
> > It is not more portable than the above. It is more expensive and adds more
> > code.
> 
> Maybe I am mistaken. Can you tell me why adding an auxiliary function
> is more expensive? (In CPU cycles?)
> 
In terms of code required. The idea here is to simplify the code, not
to make it more complex. The auxiliary function needs to be declared
and maintained in each affected file. I do find it easier and better
(and safer, for that matter) to let the C compiler handle it.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On Wed, Jan 11, 2017 at 04:28:12PM +0100, Marc Gonzalez wrote:
> On 11/01/2017 15:25, Guenter Roeck wrote:
> > On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
> >> On 11/01/2017 11:52, Guenter Roeck wrote:
> >>
> >>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> >>>
> > @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct 
> > platform_device *pdev)
> > err = clk_prepare_enable(dev->clk);
> > if (err)
> > return err;
> > +   err = devm_add_action_or_reset(>dev,
> > +  (void(*)(void 
> > *))clk_disable_unprepare,
> > +  dev->clk);
> > +   if (err)
> > +   return err;
> 
>  Hello Guenter,
> 
>  I would rather avoid the function pointer cast.
>  How about defining an auxiliary function for the cleanup action?
> 
>  clk_disable_unprepare() is static inline, so gcc will have to
>  define an auxiliary function either way. What do you think?
> >>>
> >>> Not really. It would just make it more complicated to replace the
> >>> call with devm_clk_prepare_enable(), should it ever find its way
> >>> into the light of day.
> >>
> >> More complicated, because the cleanup function will have to be deleted 
> >> later?
> >> The compiler will warn if someone forgets to do that.
> >>
> >> In my opinion, it's not a good idea to rely on the fact that casting
> >> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> >> on most platforms. (It has undefined behavior, strictly speaking.)
> >
> > I do hear that you object to this code.
> > 
> > However, I must admit that you completely lost me here. It is a cast from
> > one function pointer to another,
> 
> Perhaps you are used to work at the assembly level, where pointers are
> just addresses, and all pointers are interchangeable.
> 
> At a slightly higher level (C abstract machine), it is not so.
> 
> > passed as argument to another function,
> > with a secondary cast of its argument from a typed pointer to a void 
> > pointer.
> > I don't think C permits for "undefined behavior, strictly speaking".
> 
> The C standard leaves quite a lot of behavior undefined, e.g.
> 
> char *foo = "hello";
> foo[1] = 'a'; // UB
> 
> char buf[4];
> *(int *) = 0xdeadbeef; // UB
> 
> 1 << 64; // UB
> 
Ah, yes, I stand corrected.

However, some other unrelated undefined behavior does not mean that this
specific behavior is undefined.

So far we have a claim that a cast to a void * may somehow be different
to a cast to a different pointer, if used as function argument, and that
the behavior with such a cast may be undefined. In other words, you claim
that a function implemented as, say,

   void func(int *var) {}

might result in undefined behavior if some header file declares it as

void func(void *);

and it is called as

int var;

func();

That seems really far fetched to me.

I do get the message that you do not like this kind of cast. But that doesn't
mean it is not correct.

> > Besides, that same mechanism is already used elsewhere, which is how I
> > got the idea. Are you claiming that there are situations where it won't
> > work ?
> 
> If this technique is already used elsewhere in the kernel, then I'll
> crawl back under my rock (and weep).
> 

git grep "(void(\*)(void \*))"

and variants thereof:

git grep "(void(\*)"

> I can see two issues with the code you propose.
> 
> First is the same for all casts: silencing potential warnings,
> e.g. if the prototype of clk_disable_unprepare ever changed.
> (Though casts are required for vararg function arguments.)
> 
Understood. However, one should really hope that anyone changing
an API has a look at all its callers and does not just pray that
there are no problems.

> Second is just theory and not a real-world concern.
> 
> >> Do you really dislike the portable solution I suggested? :-(
> >
> > It is not more portable than the above. It is more expensive and adds more
> > code.
> 
> Maybe I am mistaken. Can you tell me why adding an auxiliary function
> is more expensive? (In CPU cycles?)
> 
In terms of code required. The idea here is to simplify the code, not
to make it more complex. The auxiliary function needs to be declared
and maintained in each affected file. I do find it easier and better
(and safer, for that matter) to let the C compiler handle it.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On Wed, Jan 11, 2017 at 03:39:17PM +0100, Uwe Kleine-König wrote:
> On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
> > On 11/01/2017 11:52, Guenter Roeck wrote:
> > 
> > > On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> > > 
> > >>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct 
> > >>> platform_device *pdev)
> > >>> err = clk_prepare_enable(dev->clk);
> > >>> if (err)
> > >>> return err;
> > >>> +   err = devm_add_action_or_reset(>dev,
> > >>> +  (void(*)(void 
> > >>> *))clk_disable_unprepare,
> > >>> +  dev->clk);
> > >>> +   if (err)
> > >>> +   return err;
> 
> This looks wrong. There is no clk_unprepare_disable when
> devm_add_action_or_reset fails.
> 
That is what the _or_reset part of devm_add_action_or_reset() is for.

> > >>
> > >> Hello Guenter,
> > >>
> > >> I would rather avoid the function pointer cast.
> > >> How about defining an auxiliary function for the cleanup action?
> > >>
> > >> clk_disable_unprepare() is static inline, so gcc will have to
> > >> define an auxiliary function either way. What do you think?
> > > 
> > > Not really. It would just make it more complicated to replace the
> > > call with devm_clk_prepare_enable(), should it ever find its way
> > > into the light of day.
> > 
> > More complicated, because the cleanup function will have to be deleted 
> > later?
> > The compiler will warn if someone forgets to do that.
> > 
> > In my opinion, it's not a good idea to rely on the fact that casting
> > void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> > on most platforms. (It has undefined behavior, strictly speaking.)
> 
> I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
> there couldn't be found a better solution.
> 
> If in the end it looks like the following that would be good I think:
> 
>   clk = devm_clk_get(...);
>   if (IS_ERR(clk))
>   ...
> 
>   ret = devm_clk_prepare_enable(clk)
>   if (ret)
>   return ret;
> 
Yes, Dmitry tried to introduce devm_clk_prepare_enable() some 5 years ago,
but the effort stalled.

My take is that it will be easy to write another coccinelle script to convert
to devm_clk_prepare_enable() once that is available, but I didn't see the point
of waiting for that, especially since it may never happen.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On Wed, Jan 11, 2017 at 03:39:17PM +0100, Uwe Kleine-König wrote:
> On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
> > On 11/01/2017 11:52, Guenter Roeck wrote:
> > 
> > > On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> > > 
> > >>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct 
> > >>> platform_device *pdev)
> > >>> err = clk_prepare_enable(dev->clk);
> > >>> if (err)
> > >>> return err;
> > >>> +   err = devm_add_action_or_reset(>dev,
> > >>> +  (void(*)(void 
> > >>> *))clk_disable_unprepare,
> > >>> +  dev->clk);
> > >>> +   if (err)
> > >>> +   return err;
> 
> This looks wrong. There is no clk_unprepare_disable when
> devm_add_action_or_reset fails.
> 
That is what the _or_reset part of devm_add_action_or_reset() is for.

> > >>
> > >> Hello Guenter,
> > >>
> > >> I would rather avoid the function pointer cast.
> > >> How about defining an auxiliary function for the cleanup action?
> > >>
> > >> clk_disable_unprepare() is static inline, so gcc will have to
> > >> define an auxiliary function either way. What do you think?
> > > 
> > > Not really. It would just make it more complicated to replace the
> > > call with devm_clk_prepare_enable(), should it ever find its way
> > > into the light of day.
> > 
> > More complicated, because the cleanup function will have to be deleted 
> > later?
> > The compiler will warn if someone forgets to do that.
> > 
> > In my opinion, it's not a good idea to rely on the fact that casting
> > void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> > on most platforms. (It has undefined behavior, strictly speaking.)
> 
> I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
> there couldn't be found a better solution.
> 
> If in the end it looks like the following that would be good I think:
> 
>   clk = devm_clk_get(...);
>   if (IS_ERR(clk))
>   ...
> 
>   ret = devm_clk_prepare_enable(clk)
>   if (ret)
>   return ret;
> 
Yes, Dmitry tried to introduce devm_clk_prepare_enable() some 5 years ago,
but the effort stalled.

My take is that it will be easy to write another coccinelle script to convert
to devm_clk_prepare_enable() once that is available, but I didn't see the point
of waiting for that, especially since it may never happen.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 15:25, Guenter Roeck wrote:
> On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;

 Hello Guenter,

 I would rather avoid the function pointer cast.
 How about defining an auxiliary function for the cleanup action?

 clk_disable_unprepare() is static inline, so gcc will have to
 define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>
> I do hear that you object to this code.
> 
> However, I must admit that you completely lost me here. It is a cast from
> one function pointer to another,

Perhaps you are used to work at the assembly level, where pointers are
just addresses, and all pointers are interchangeable.

At a slightly higher level (C abstract machine), it is not so.

> passed as argument to another function,
> with a secondary cast of its argument from a typed pointer to a void pointer.
> I don't think C permits for "undefined behavior, strictly speaking".

The C standard leaves quite a lot of behavior undefined, e.g.

char *foo = "hello";
foo[1] = 'a'; // UB

char buf[4];
*(int *) = 0xdeadbeef; // UB

1 << 64; // UB

> Besides, that same mechanism is already used elsewhere, which is how I
> got the idea. Are you claiming that there are situations where it won't
> work ?

If this technique is already used elsewhere in the kernel, then I'll
crawl back under my rock (and weep).

I can see two issues with the code you propose.

First is the same for all casts: silencing potential warnings,
e.g. if the prototype of clk_disable_unprepare ever changed.
(Though casts are required for vararg function arguments.)

Second is just theory and not a real-world concern.

>> Do you really dislike the portable solution I suggested? :-(
>
> It is not more portable than the above. It is more expensive and adds more
> code.

Maybe I am mistaken. Can you tell me why adding an auxiliary function
is more expensive? (In CPU cycles?)

clk_disable_unprepare() is static inline, so an auxiliary function
exists either way (implicit or explicit).

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 15:25, Guenter Roeck wrote:
> On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;

 Hello Guenter,

 I would rather avoid the function pointer cast.
 How about defining an auxiliary function for the cleanup action?

 clk_disable_unprepare() is static inline, so gcc will have to
 define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>
> I do hear that you object to this code.
> 
> However, I must admit that you completely lost me here. It is a cast from
> one function pointer to another,

Perhaps you are used to work at the assembly level, where pointers are
just addresses, and all pointers are interchangeable.

At a slightly higher level (C abstract machine), it is not so.

> passed as argument to another function,
> with a secondary cast of its argument from a typed pointer to a void pointer.
> I don't think C permits for "undefined behavior, strictly speaking".

The C standard leaves quite a lot of behavior undefined, e.g.

char *foo = "hello";
foo[1] = 'a'; // UB

char buf[4];
*(int *) = 0xdeadbeef; // UB

1 << 64; // UB

> Besides, that same mechanism is already used elsewhere, which is how I
> got the idea. Are you claiming that there are situations where it won't
> work ?

If this technique is already used elsewhere in the kernel, then I'll
crawl back under my rock (and weep).

I can see two issues with the code you propose.

First is the same for all casts: silencing potential warnings,
e.g. if the prototype of clk_disable_unprepare ever changed.
(Though casts are required for vararg function arguments.)

Second is just theory and not a real-world concern.

>> Do you really dislike the portable solution I suggested? :-(
>
> It is not more portable than the above. It is more expensive and adds more
> code.

Maybe I am mistaken. Can you tell me why adding an auxiliary function
is more expensive? (In CPU cycles?)

clk_disable_unprepare() is static inline, so an auxiliary function
exists either way (implicit or explicit).

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Måns Rullgård]

Guenter Roeck  writes:

> On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;

 Hello Guenter,

 I would rather avoid the function pointer cast.
 How about defining an auxiliary function for the cleanup action?

 clk_disable_unprepare() is static inline, so gcc will have to
 define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>>
> I do hear that you object to this code.
>
> However, I must admit that you completely lost me here. It is a cast from
> one function pointer to another, passed as argument to another function,
> with a secondary cast of its argument from a typed pointer to a void pointer.
> I don't think C permits for "undefined behavior, strictly speaking".
> Besides, that same mechanism is already used elsewhere, which is how I
> got the idea. Are you claiming that there are situations where it won't
> work ?

A pointer to void is interchangeable with any other pointer type.  That
doesn't necessarily imply that pointers to functions taking arguments of
different pointer types (as we have here) are always compatible.  I'd
have to read the C standard carefully to see if there's any such
promise, and I have other things to do right now.  I am, however, not
aware of any ABI (certainly none used by Linux) where it would pose a
problem.

-- 
Måns Rullgård

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Måns Rullgård]

Guenter Roeck  writes:

> On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;

 Hello Guenter,

 I would rather avoid the function pointer cast.
 How about defining an auxiliary function for the cleanup action?

 clk_disable_unprepare() is static inline, so gcc will have to
 define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>>
> I do hear that you object to this code.
>
> However, I must admit that you completely lost me here. It is a cast from
> one function pointer to another, passed as argument to another function,
> with a secondary cast of its argument from a typed pointer to a void pointer.
> I don't think C permits for "undefined behavior, strictly speaking".
> Besides, that same mechanism is already used elsewhere, which is how I
> got the idea. Are you claiming that there are situations where it won't
> work ?

A pointer to void is interchangeable with any other pointer type.  That
doesn't necessarily imply that pointers to functions taking arguments of
different pointer types (as we have here) are always compatible.  I'd
have to read the C standard carefully to see if there's any such
promise, and I have other things to do right now.  I am, however, not
aware of any ABI (certainly none used by Linux) where it would pose a
problem.

-- 
Måns Rullgård

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Vladimir Zapolskiy]

Hello Uwe,

On 01/11/2017 04:39 PM, Uwe Kleine-König wrote:
> On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;
> 
> This looks wrong. There is no clk_unprepare_disable when
> devm_add_action_or_reset fails.

actually there is a call to clk_disable_unprepare() on error path, you may
take a look at devm_add_action_or_reset() implementation.

Your comment is valid for devm_add_action() function though, but it's not
the case here.

--
With best wishes,
Vladimir

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Vladimir Zapolskiy]

Hello Uwe,

On 01/11/2017 04:39 PM, Uwe Kleine-König wrote:
> On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;
> 
> This looks wrong. There is no clk_unprepare_disable when
> devm_add_action_or_reset fails.

actually there is a call to clk_disable_unprepare() on error path, you may
take a look at devm_add_action_or_reset() implementation.

Your comment is valid for devm_add_action() function though, but it's not
the case here.

--
With best wishes,
Vladimir

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Uwe Kleine-König]

On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
> On 11/01/2017 11:52, Guenter Roeck wrote:
> 
> > On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> > 
> >>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> >>> *pdev)
> >>>   err = clk_prepare_enable(dev->clk);
> >>>   if (err)
> >>>   return err;
> >>> + err = devm_add_action_or_reset(>dev,
> >>> +(void(*)(void *))clk_disable_unprepare,
> >>> +dev->clk);
> >>> + if (err)
> >>> + return err;

This looks wrong. There is no clk_unprepare_disable when
devm_add_action_or_reset fails.

> >>
> >> Hello Guenter,
> >>
> >> I would rather avoid the function pointer cast.
> >> How about defining an auxiliary function for the cleanup action?
> >>
> >> clk_disable_unprepare() is static inline, so gcc will have to
> >> define an auxiliary function either way. What do you think?
> > 
> > Not really. It would just make it more complicated to replace the
> > call with devm_clk_prepare_enable(), should it ever find its way
> > into the light of day.
> 
> More complicated, because the cleanup function will have to be deleted later?
> The compiler will warn if someone forgets to do that.
> 
> In my opinion, it's not a good idea to rely on the fact that casting
> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> on most platforms. (It has undefined behavior, strictly speaking.)

I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
there couldn't be found a better solution.

If in the end it looks like the following that would be good I think:

clk = devm_clk_get(...);
if (IS_ERR(clk))
...

ret = devm_clk_prepare_enable(clk)
if (ret)
return ret;

...

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Uwe Kleine-König]

On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
> On 11/01/2017 11:52, Guenter Roeck wrote:
> 
> > On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> > 
> >>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> >>> *pdev)
> >>>   err = clk_prepare_enable(dev->clk);
> >>>   if (err)
> >>>   return err;
> >>> + err = devm_add_action_or_reset(>dev,
> >>> +(void(*)(void *))clk_disable_unprepare,
> >>> +dev->clk);
> >>> + if (err)
> >>> + return err;

This looks wrong. There is no clk_unprepare_disable when
devm_add_action_or_reset fails.

> >>
> >> Hello Guenter,
> >>
> >> I would rather avoid the function pointer cast.
> >> How about defining an auxiliary function for the cleanup action?
> >>
> >> clk_disable_unprepare() is static inline, so gcc will have to
> >> define an auxiliary function either way. What do you think?
> > 
> > Not really. It would just make it more complicated to replace the
> > call with devm_clk_prepare_enable(), should it ever find its way
> > into the light of day.
> 
> More complicated, because the cleanup function will have to be deleted later?
> The compiler will warn if someone forgets to do that.
> 
> In my opinion, it's not a good idea to rely on the fact that casting
> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> on most platforms. (It has undefined behavior, strictly speaking.)

I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
there couldn't be found a better solution.

If in the end it looks like the following that would be good I think:

clk = devm_clk_get(...);
if (IS_ERR(clk))
...

ret = devm_clk_prepare_enable(clk)
if (ret)
return ret;

...

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 04:31 AM, Marc Gonzalez wrote:

On 11/01/2017 11:52, Guenter Roeck wrote:


On 01/11/2017 01:07 AM, Marc Gonzalez wrote:


@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;


Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?


Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.


More complicated, because the cleanup function will have to be deleted later?
The compiler will warn if someone forgets to do that.

In my opinion, it's not a good idea to rely on the fact that casting
void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
on most platforms. (It has undefined behavior, strictly speaking.)


I do hear that you object to this code.

However, I must admit that you completely lost me here. It is a cast from
one function pointer to another, passed as argument to another function,
with a secondary cast of its argument from a typed pointer to a void pointer.
I don't think C permits for "undefined behavior, strictly speaking".
Besides, that same mechanism is already used elsewhere, which is how I
got the idea. Are you claiming that there are situations where it won't
work ?


Do you really dislike the portable solution I suggested? :-(


It is not more portable than the above. It is more expensive and adds more
code.

Thanks,
Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 04:31 AM, Marc Gonzalez wrote:

On 11/01/2017 11:52, Guenter Roeck wrote:


On 01/11/2017 01:07 AM, Marc Gonzalez wrote:


@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;


Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?


Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.


More complicated, because the cleanup function will have to be deleted later?
The compiler will warn if someone forgets to do that.

In my opinion, it's not a good idea to rely on the fact that casting
void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
on most platforms. (It has undefined behavior, strictly speaking.)


I do hear that you object to this code.

However, I must admit that you completely lost me here. It is a cast from
one function pointer to another, passed as argument to another function,
with a secondary cast of its argument from a typed pointer to a void pointer.
I don't think C permits for "undefined behavior, strictly speaking".
Besides, that same mechanism is already used elsewhere, which is how I
got the idea. Are you claiming that there are situations where it won't
work ?


Do you really dislike the portable solution I suggested? :-(


It is not more portable than the above. It is more expensive and adds more
code.

Thanks,
Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 11:52, Guenter Roeck wrote:

> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> 
>>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
>>> *pdev)
>>> err = clk_prepare_enable(dev->clk);
>>> if (err)
>>> return err;
>>> +   err = devm_add_action_or_reset(>dev,
>>> +  (void(*)(void *))clk_disable_unprepare,
>>> +  dev->clk);
>>> +   if (err)
>>> +   return err;
>>
>> Hello Guenter,
>>
>> I would rather avoid the function pointer cast.
>> How about defining an auxiliary function for the cleanup action?
>>
>> clk_disable_unprepare() is static inline, so gcc will have to
>> define an auxiliary function either way. What do you think?
> 
> Not really. It would just make it more complicated to replace the
> call with devm_clk_prepare_enable(), should it ever find its way
> into the light of day.

More complicated, because the cleanup function will have to be deleted later?
The compiler will warn if someone forgets to do that.

In my opinion, it's not a good idea to rely on the fact that casting
void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
on most platforms. (It has undefined behavior, strictly speaking.)

Do you really dislike the portable solution I suggested? :-(

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 11:52, Guenter Roeck wrote:

> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> 
>>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
>>> *pdev)
>>> err = clk_prepare_enable(dev->clk);
>>> if (err)
>>> return err;
>>> +   err = devm_add_action_or_reset(>dev,
>>> +  (void(*)(void *))clk_disable_unprepare,
>>> +  dev->clk);
>>> +   if (err)
>>> +   return err;
>>
>> Hello Guenter,
>>
>> I would rather avoid the function pointer cast.
>> How about defining an auxiliary function for the cleanup action?
>>
>> clk_disable_unprepare() is static inline, so gcc will have to
>> define an auxiliary function either way. What do you think?
> 
> Not really. It would just make it more complicated to replace the
> call with devm_clk_prepare_enable(), should it ever find its way
> into the light of day.

More complicated, because the cleanup function will have to be deleted later?
The compiler will warn if someone forgets to do that.

In my opinion, it's not a good idea to rely on the fact that casting
void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
on most platforms. (It has undefined behavior, strictly speaking.)

Do you really dislike the portable solution I suggested? :-(

Regards.

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 01:07 AM, Marc Gonzalez wrote:


@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;


Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?



Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 01:07 AM, Marc Gonzalez wrote:


@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;


Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?



Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.

Guenter

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 03:09, Guenter Roeck wrote:

> Use device managed functions to simplify error handling, reduce
> source code size, improve readability, and reduce the likelyhood of bugs.
> 
> The conversion was done automatically with coccinelle using the
> following semantic patches. The semantic patches and the scripts used
> to generate this commit log are available at
> https://github.com/groeck/coccinelle-patches
> 
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Replace 'goto l; ... l: return e;' with 'return e;'
> - Replace 'val = e; return val;' with 'return e;'
> - Replace 'if (e) { return expr; }' with 'if (e) return expr;'
> - Use devm_watchdog_register_driver() to register watchdog device
> 
> Cc: Marc Gonzalez 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/watchdog/tangox_wdt.c | 22 +-
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
> index d5fcce062920..7688e1b35867 100644
> --- a/drivers/watchdog/tangox_wdt.c
> +++ b/drivers/watchdog/tangox_wdt.c
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;

Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?

Regards.


diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
index 202c4b9cc921..1a4f6d245a83 100644
--- a/drivers/watchdog/tangox_wdt.c
+++ b/drivers/watchdog/tangox_wdt.c
@@ -114,6 +114,11 @@ static int tangox_wdt_restart(struct notifier_block *nb, 
unsigned long action,
return NOTIFY_DONE;
 }
 
+static void cleanup(void *clk)
+{
+   clk_disable_unprepare(clk);
+}
+
 static int tangox_wdt_probe(struct platform_device *pdev)
 {
struct tangox_wdt_device *dev;
@@ -138,6 +143,10 @@ static int tangox_wdt_probe(struct platform_device *pdev)
if (err)
return err;
 
+   err = devm_add_action_or_reset(>dev, cleanup, dev->clk);
+   if (err)
+   return err;
+
dev->clk_rate = clk_get_rate(dev->clk);
if (!dev->clk_rate) {
err = -EINVAL;

Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 03:09, Guenter Roeck wrote:

> Use device managed functions to simplify error handling, reduce
> source code size, improve readability, and reduce the likelyhood of bugs.
> 
> The conversion was done automatically with coccinelle using the
> following semantic patches. The semantic patches and the scripts used
> to generate this commit log are available at
> https://github.com/groeck/coccinelle-patches
> 
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Replace 'goto l; ... l: return e;' with 'return e;'
> - Replace 'val = e; return val;' with 'return e;'
> - Replace 'if (e) { return expr; }' with 'if (e) return expr;'
> - Use devm_watchdog_register_driver() to register watchdog device
> 
> Cc: Marc Gonzalez 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/watchdog/tangox_wdt.c | 22 +-
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
> index d5fcce062920..7688e1b35867 100644
> --- a/drivers/watchdog/tangox_wdt.c
> +++ b/drivers/watchdog/tangox_wdt.c
> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device 
> *pdev)
>   err = clk_prepare_enable(dev->clk);
>   if (err)
>   return err;
> + err = devm_add_action_or_reset(>dev,
> +(void(*)(void *))clk_disable_unprepare,
> +dev->clk);
> + if (err)
> + return err;

Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?

Regards.


diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
index 202c4b9cc921..1a4f6d245a83 100644
--- a/drivers/watchdog/tangox_wdt.c
+++ b/drivers/watchdog/tangox_wdt.c
@@ -114,6 +114,11 @@ static int tangox_wdt_restart(struct notifier_block *nb, 
unsigned long action,
return NOTIFY_DONE;
 }
 
+static void cleanup(void *clk)
+{
+   clk_disable_unprepare(clk);
+}
+
 static int tangox_wdt_probe(struct platform_device *pdev)
 {
struct tangox_wdt_device *dev;
@@ -138,6 +143,10 @@ static int tangox_wdt_probe(struct platform_device *pdev)
if (err)
return err;
 
+   err = devm_add_action_or_reset(>dev, cleanup, dev->clk);
+   if (err)
+   return err;
+
dev->clk_rate = clk_get_rate(dev->clk);
if (!dev->clk_rate) {
err = -EINVAL;

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

Use device managed functions to simplify error handling, reduce
source code size, improve readability, and reduce the likelyhood of bugs.

The conversion was done automatically with coccinelle using the
following semantic patches. The semantic patches and the scripts used
to generate this commit log are available at
https://github.com/groeck/coccinelle-patches

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Replace 'goto l; ... l: return e;' with 'return e;'
- Replace 'val = e; return val;' with 'return e;'
- Replace 'if (e) { return expr; }' with 'if (e) return expr;'
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Marc Gonzalez 
Signed-off-by: Guenter Roeck 
---
 drivers/watchdog/tangox_wdt.c | 22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
index d5fcce062920..7688e1b35867 100644
--- a/drivers/watchdog/tangox_wdt.c
+++ b/drivers/watchdog/tangox_wdt.c
@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;
 
dev->clk_rate = clk_get_rate(dev->clk);
-   if (!dev->clk_rate) {
-   err = -EINVAL;
-   goto err;
-   }
+   if (!dev->clk_rate)
+   return -EINVAL;
 
dev->wdt.parent = >dev;
dev->wdt.info = _wdt_info;
@@ -173,19 +176,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
 
watchdog_set_restart_priority(>wdt, 128);
 
-   err = watchdog_register_device(>wdt);
+   err = devm_watchdog_register_device(>dev, >wdt);
if (err)
-   goto err;
+   return err;
 
platform_set_drvdata(pdev, dev);
 
dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
 
return 0;
-
- err:
-   clk_disable_unprepare(dev->clk);
-   return err;
 }
 
 static int tangox_wdt_remove(struct platform_device *pdev)
@@ -193,9 +192,6 @@ static int tangox_wdt_remove(struct platform_device *pdev)
struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
 
tangox_wdt_stop(>wdt);
-   clk_disable_unprepare(dev->clk);
-
-   watchdog_unregister_device(>wdt);
 
return 0;
 }
-- 
2.7.4

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

Use device managed functions to simplify error handling, reduce
source code size, improve readability, and reduce the likelyhood of bugs.

The conversion was done automatically with coccinelle using the
following semantic patches. The semantic patches and the scripts used
to generate this commit log are available at
https://github.com/groeck/coccinelle-patches

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Replace 'goto l; ... l: return e;' with 'return e;'
- Replace 'val = e; return val;' with 'return e;'
- Replace 'if (e) { return expr; }' with 'if (e) return expr;'
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Marc Gonzalez 
Signed-off-by: Guenter Roeck 
---
 drivers/watchdog/tangox_wdt.c | 22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
index d5fcce062920..7688e1b35867 100644
--- a/drivers/watchdog/tangox_wdt.c
+++ b/drivers/watchdog/tangox_wdt.c
@@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+   err = devm_add_action_or_reset(>dev,
+  (void(*)(void *))clk_disable_unprepare,
+  dev->clk);
+   if (err)
+   return err;
 
dev->clk_rate = clk_get_rate(dev->clk);
-   if (!dev->clk_rate) {
-   err = -EINVAL;
-   goto err;
-   }
+   if (!dev->clk_rate)
+   return -EINVAL;
 
dev->wdt.parent = >dev;
dev->wdt.info = _wdt_info;
@@ -173,19 +176,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
 
watchdog_set_restart_priority(>wdt, 128);
 
-   err = watchdog_register_device(>wdt);
+   err = devm_watchdog_register_device(>dev, >wdt);
if (err)
-   goto err;
+   return err;
 
platform_set_drvdata(pdev, dev);
 
dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
 
return 0;
-
- err:
-   clk_disable_unprepare(dev->clk);
-   return err;
 }
 
 static int tangox_wdt_remove(struct platform_device *pdev)
@@ -193,9 +192,6 @@ static int tangox_wdt_remove(struct platform_device *pdev)
struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
 
tangox_wdt_stop(>wdt);
-   clk_disable_unprepare(dev->clk);
-
-   watchdog_unregister_device(>wdt);
 
return 0;
 }
-- 
2.7.4