| Message ID | bd56e90c-5cb8-02d8-3518-c8f5483a6df4@sigmadesigns.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Guenter
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.) Do you really dislike the portable solution I suggested? :-( Regards.
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, 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
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(&pdev->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
Guenter Roeck <linux@roeck-us.net> 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(&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, 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.
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(&pdev->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
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.
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(&pdev->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
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
On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck <linux@roeck-us.net> 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.
On 01/11/2017 04:12 PM, Andy Shevchenko wrote: > On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck <linux@roeck-us.net> 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
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(&var); > > 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.
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(&var); > > > > 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
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> 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(&var); >> >> 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.
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(&var); >>> >>> 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.
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(&pdev->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
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(&pdev->dev, cleanup, dev->clk); + if (err) + return err; + dev->clk_rate = clk_get_rate(dev->clk); if (!dev->clk_rate) { err = -EINVAL;