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

Groups > linux.kernel > #1417474

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

Show all headers | View raw

Hi Wolfram,

On 08.06.2016 09:54, Wolfram Sang wrote:
> On Tue, Jun 07, 2016 at 08:38:45PM +0300, Vladimir Zapolskiy wrote:
>> The change adds a simple watchdog pretimeout framework infrastructure,
>> its purpose is to allow users to select a desired handling of watchdog
>> pretimeout events, which may be generated by some watchdog devices.
>>
>> A user selects a default watchdog pretimeout governor during
>> compilation stage.
>>
>> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
>> attributes in sysfs: pretimeout to display currently set pretimeout
>> value and pretimeout_governor attribute to display the selected
>> watchdog pretimeout governor.
>>
>> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
>> sysfs, and such watchdog devices do not require the framework.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>> Changes from v2 to v3:
>> * essentially simplified the implementation due to removal of runtime
>>   dynamic selection of watchdog pretimeout governors by a user, this
>>   feature is supposed to be added later on
> 
> Hmm, your call, but I'm not sure this will make the reviewing process
> easier...

Looks like the series hits a relatively vague contradiction described
in Documentation//development-process/5.Posting :

1) logically independent change should be formatted as a separate patch
2) the changes need to be considered in their final form

While in v1/v2 I've selected point 2) and even tried to defend it
in conversation with Guenter, for v3 I primarily choose point 1),
because all other features can be added on top.

Guenter, do you have any judgment?

>> * removed support of sleepable watchdog pretimeout governors
> 
> This does.

Same as above.

>> * moved sysfs device attributes to watchdog_dev.c, this required to
>>   add exported watchdog_pretimeout_governor_name() interface
> 
> Why this move? Before, all the pretimeout stuff was nicely encapsulated
> in its own file which could be compiled out. Now things are mixing. What
> was wrong with the approach I took?`

Simplification of the "struct device" life time management?
A lot of time and efforts were spent to centralize it, while you know
that I took both approaches, I tend to keep it exclusively in
watchdog_dev.c , probably Guenter can express his point of view.

>> @@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>>  		}
>>  	}
>>  
>> +	ret = watchdog_register_pretimeout(wdd);
>> +	if (ret) {
>> +		watchdog_dev_unregister(wdd);
>> +		ida_simple_remove(&watchdog_ida, wdd->id);
>> +		return ret;
>> +	}
>> +
> 
> What is the advantage of adding it here instead of adding it in
> watchdog_dev.c? I mean the files to control govenors are tied to the
> watchdog_device anyhow, so I'd think it's cleaner to move all that
> action to watchdog_dev instead of having this stray one in the core.
> 

This makes sense, I will move it to watchdog_dev.c.

Thank you for review.

--
Best wishes,
Vladimir

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
  [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
    Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Guenter Roeck <linux@roeck-us.net> - 2016-06-07 23:50 +0200
      Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 15:40 +0200
        Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Guenter Roeck <linux@roeck-us.net> - 2016-06-08 16:00 +0200
          Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 17:20 +0200
            kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add  watchdog pretimeout framework) Joe Perches <joe@perches.com> - 2016-06-08 17:40 +0200
              Re: kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog:  add watchdog pretimeout framework) Guenter Roeck <linux@roeck-us.net> - 2016-06-08 20:10 +0200
    Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 09:00 +0200
      Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 16:10 +0200
        Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Guenter Roeck <linux@roeck-us.net> - 2016-06-08 20:30 +0200
          Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:30 +0200
        Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:20 +0200
  [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
    Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout  governor Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 09:10 +0200
      Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout  governor Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 16:30 +0200
        Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout  governor Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:30 +0200
  [PATCH v3 2/6] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
  [PATCH v3 3/6] watchdog: add pretimeout read-only device attribute to sysfs Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
    Re: [PATCH v3 3/6] watchdog: add pretimeout read-only device  attribute to sysfs Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 09:00 +0200
  Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 10:00 +0200
    Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 17:40 +0200
      Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:40 +0200

csiph-web