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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f7d9227c-4912-5c52-5ba7-ae7fa3b89857@sigmadesigns.com>
Date:   Wed, 11 Jan 2017 16:28:12 +0100
From:   Marc Gonzalez <marc_gonzalez@...madesigns.com>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     Wim Van Sebroeck <wim@...ana.be>, <linux-watchdog@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Mans Rullgard <mans@...sr.com>,
        Thibaud Cornic <thibaud_cornic@...madesigns.com>,
        Mark Rutland <mark.rutland@....com>,
        Uwe Kleine-Konig <u.kleine-koenig@...gutronix.de>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed
 functions

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

> 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.