Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1556036 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/62] watchdog: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 22/62] watchdog: imx2_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 12/62] watchdog: da9055_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 04/62] watchdog: atlas7_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 15/62] watchdog: davinci_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 29/62] watchdog: max77620_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 08/62] watchdog: bcm_kona_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 30/62] watchdog: mena21_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
      Re: [PATCH 30/62] watchdog: mena21_wdt: Convert to use device managed  functions and other improvements Johannes Thumshirn <morbidrsa@gmail.com> - 2017-01-13 09:10 +0100
    [PATCH 02/62] watchdog: aspeed_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
      Re: [PATCH 02/62] watchdog: aspeed_wdt: Convert to use device managed functions Joel Stanley <joel@jms.id.au> - 2017-01-11 06:20 +0100
    [PATCH 03/62] watchdog: at91sam9_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 25/62] watchdog: kempld_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 09/62] watchdog: cadence_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:40 +0100
    [PATCH 05/62] watchdog: bcm2835_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:50 +0100
      Re: [PATCH 05/62] watchdog: bcm2835_wdt: Convert to use device managed functions and other improvements Eric Anholt <eric@anholt.net> - 2017-01-14 07:30 +0100
    [PATCH 21/62] watchdog: imgpdc_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:50 +0100
    [PATCH 19/62] watchdog: gpio_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:50 +0100
    [PATCH 10/62] watchdog: coh901327_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 00:50 +0100
      Re: [PATCH 10/62] watchdog: coh901327_wdt: Convert to use device  managed functions Linus Walleij <linus.walleij@linaro.org> - 2017-01-11 16:50 +0100
    [PATCH 38/62] watchdog: nic7018_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 49/62] watchdog: sama5d4_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 40/62] watchdog: omap_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 39/62] watchdog: of_xilinx_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 36/62] watchdog: mt7621_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 42/62] watchdog: pic32-dmt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 35/62] watchdog: mpc8xxx_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 46/62] watchdog: renesas_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 32/62] watchdog: meson_gxbb_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
      Re: [PATCH 32/62] watchdog: meson_gxbb_wdt: Convert to use device  managed functions and other improvements Neil Armstrong <narmstrong@baylibre.com> - 2017-01-11 09:50 +0100
      Re: [PATCH 32/62] watchdog: meson_gxbb_wdt: Convert to use device managed functions and other improvements Kevin Hilman <khilman@baylibre.com> - 2017-01-11 19:50 +0100
    [PATCH 41/62] watchdog: orion_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 34/62] watchdog: moxart_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 47/62] watchdog: retu_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 37/62] watchdog: mtk_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 50/62] watchdog: sbsa_gwdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 43/62] watchdog: pic32-wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 44/62] watchdog: pnx4008_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
      Re: [PATCH 44/62] watchdog: pnx4008_wdt: Convert to use device  managed functions Vladimir Zapolskiy <vz@mleia.com> - 2017-01-12 01:20 +0100
    [PATCH 45/62] watchdog: qcom-wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
    [PATCH 31/62] watchdog: menf21bmc_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
      [PATCH 48/62] watchdog: rt2880_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
      [PATCH 51/62] watchdog: shwdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
      [PATCH 33/62] watchdog: meson_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 01:50 +0100
        Re: [PATCH 33/62] watchdog: meson_wdt: Convert to use device managed functions and other improvements Kevin Hilman <khilman@baylibre.com> - 2017-01-11 19:50 +0100
    [PATCH 53/62] watchdog: st_lpc_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:10 +0100
    [PATCH 52/62] watchdog: sirfsoc_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:10 +0100
      [PATCH 55/62] watchdog: sunxi_wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:10 +0100
        Re: [PATCH 55/62] watchdog: sunxi_wdt: Convert to use device managed  functions and other improvements Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-01-11 13:20 +0100
      [PATCH 57/62] watchdog: tegra_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100
      [PATCH 61/62] watchdog: ux500_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100
      [PATCH 60/62] watchdog: txx9wdt: Convert to use device managed functions and other improvements Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100
      [PATCH 59/62] watchdog: twl4030_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100
      [PATCH 62/62] watchdog: wm831x_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100
        Re: [PATCH 62/62] watchdog: wm831x_wdt: Convert to use device  managed functions Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-01-12 11:30 +0100
      [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100
        Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-01-11 10:10 +0100
          Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 12:00 +0100
            Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-01-11 13:40 +0100
              Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 15:30 +0100
                Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions Måns Rullgård <mans@mansr.com> - 2017-01-11 15:50 +0100
                Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-01-11 16:30 +0100
                  Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device  managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 19:00 +0100
                    Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-01-12 10:50 +0100
                      Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device  managed functions Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-01-12 11:00 +0100
                      Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions Måns Rullgård <mans@mansr.com> - 2017-01-12 12:30 +0100
                        Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-01-12 13:20 +0100
              Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device  managed functions Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-01-11 15:40 +0100
                Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-01-11 16:00 +0100
                Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device  managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 18:30 +0100
                Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Guenter Roeck <linux@roeck-us.net> - 2017-01-13 06:20 +0100
            Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-12 01:20 +0100
              Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed  functions Guenter Roeck <linux@roeck-us.net> - 2017-01-12 02:40 +0100
      [PATCH 54/62] watchdog: stmp3xxx_rtc_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100
      [PATCH 58/62] watchdog: ts4800_wdt: Convert to use device managed functions Guenter Roeck <linux@roeck-us.net> - 2017-01-11 03:20 +0100

Page 4 of 4 — ← Prev page 1 2 3 [4]

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

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.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1556619 — 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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1556164 — [PATCH 54/62] watchdog: stmp3xxx_rtc_wdt: Convert to use device managed functions

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_watchdog_register_driver() to register watchdog device

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/stmp3xxx_rtc_wdt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
index d8b11eb269ad..c67f48fba8e1 100644
--- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
+++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
@@ -99,7 +99,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 	stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT);
 	stmp3xxx_wdd.parent = &pdev->dev;
 
-	ret = watchdog_register_device(&stmp3xxx_wdd);
+	ret = devm_watchdog_register_device(&pdev->dev, &stmp3xxx_wdd);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "cannot register watchdog device\n");
 		return ret;
@@ -116,7 +116,6 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 static int stmp3xxx_wdt_remove(struct platform_device *pdev)
 {
 	unregister_reboot_notifier(&wdt_notifier);
-	watchdog_unregister_device(&stmp3xxx_wdd);
 	return 0;
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1556166 — [PATCH 58/62] watchdog: ts4800_wdt: Convert to use device managed functions

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

- Drop assignments to otherwise unused variables
- Drop remove function
- Drop platform_set_drvdata()
- Use devm_watchdog_register_driver() to register watchdog device

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/ts4800_wdt.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/watchdog/ts4800_wdt.c b/drivers/watchdog/ts4800_wdt.c
index 2b8de8602b67..93e0da560b48 100644
--- a/drivers/watchdog/ts4800_wdt.c
+++ b/drivers/watchdog/ts4800_wdt.c
@@ -168,15 +168,13 @@ static int ts4800_wdt_probe(struct platform_device *pdev)
 	 */
 	ts4800_wdt_stop(wdd);
 
-	ret = watchdog_register_device(wdd);
+	ret = devm_watchdog_register_device(&pdev->dev, wdd);
 	if (ret) {
 		dev_err(&pdev->dev,
 			"failed to register watchdog device\n");
 		return ret;
 	}
 
-	platform_set_drvdata(pdev, wdt);
-
 	dev_info(&pdev->dev,
 		 "initialized (timeout = %d sec, nowayout = %d)\n",
 		 wdd->timeout, nowayout);
@@ -184,15 +182,6 @@ static int ts4800_wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int ts4800_wdt_remove(struct platform_device *pdev)
-{
-	struct ts4800_wdt *wdt = platform_get_drvdata(pdev);
-
-	watchdog_unregister_device(&wdt->wdd);
-
-	return 0;
-}
-
 static const struct of_device_id ts4800_wdt_of_match[] = {
 	{ .compatible = "technologic,ts4800-wdt", },
 	{ },
@@ -201,7 +190,6 @@ MODULE_DEVICE_TABLE(of, ts4800_wdt_of_match);
 
 static struct platform_driver ts4800_wdt_driver = {
 	.probe		= ts4800_wdt_probe,
-	.remove		= ts4800_wdt_remove,
 	.driver		= {
 		.name	= "ts4800_wdt",
 		.of_match_table = ts4800_wdt_of_match,
-- 
2.7.4

[toc] | [prev] | [standalone]

Page 4 of 4 — ← Prev page 1 2 3 [4]

Back to top | Article view | linux.kernel

csiph-web