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(&pdev->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 *)&buf = 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(&var);

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