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

[Vladimir Zapolskiy]

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@xxxxxxxxxx>
>> ---
>> 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