On 06.05.2015 10:48, Uwe Kleine-König wrote:
Hello,
On Wed, May 06, 2015 at 10:26:07AM +0300, Timo Kokkonen wrote:
On 05.05.2015 16:50, Uwe Kleine-König wrote:
Hello,
I talked to Marc Kleine-Budde about your approach, thought a bit more
about it and want to share a few thoughts with you.
Actually your series addresses three different problems
a) some watchdog hardware isn't stoppable;
b) some watchdog hardware has short maximal timeout;
c) what to do with a watchdog that is already running at probe time?
The common solution is to add a mid-layer between userspace and the
driver that bridges the possible hardware limitations when userspace
wants to stop the watchdog or set a big timeout value. c) is a bit
different but could make use of the infrastructure that is introduced
while fixing a+b). The main difference between a+b) and c) is that for
c) you have to introduce some policy. If the series were mine I'd first
do three commits that address a), b) and c) each. Then convert drivers
to it.
The infrastructure needed for both a and b is the same, the actual
code to implement either a or b is also touching the same functions
and quite slim, so I think it may not be feasible to separate those
in their own patches. But I'll think about it and see if logical
separation makes sense.
Yeah, probably you have to actually try it to see if it's sensible.
Guenter and I already said something similar, but I will eventually
repeat it here more explicitly: When introducing a midlayer that
abstracts between hardware and it's users the IMHO most important thing
to get right is to be explicit about which side of a midlayer you're
currently working at. That is, be explicit about watchdog_is_running:
Does it mean the hardware is running, or does userspace believe the
watchdog to be active? Same for timeout, stoppable etc.pp.
Yes, I agree. Thanks for clarifying this. I will keep this in mind
and try to be explicit when naming functions and variables. The best
way would be to also rename the existing variables too, but that
would also touch all the existing drivers. Keeping the current
naming scheme in place no doubt is less explicit about which side of
the new midlayer they are working on. But luckily the logic of the
existing driver doesn't change, they are all exposing things
directly to userland and they can be documented as is. The new
variables and functions add interfaces only between the new midlayer
and HW, so they are explicit as well. So I think it is a fair
compromise to leave all existing interface towards the user space as
is.
Maybe a good first step before addressing a+b+c) is to cleanup the
naming? Not sure how well this works though.
I'm sure it can be done. I'll probably should use coccinelle to get it
right as I don't really trust sed alone is good enough.. And I may not
be able to compile test all drivers, let alone run any other testing.
There are 80+ drivers that I'd probably need to touch.. It is a lot of
code churn but still a trivial variable rename. If you are willing to
accept such a patch, then I will see how it can be done.
Although, are there any other variables besides timeout and min_timeout
that we need to change? The timeout would become sw_timeout and
min_timeout would be hw_min_timeout. Maybe watchdog_active() could be
renamed to watchdog_device_open() and then add new watchdog_hw_running()
to check whether the watchdog HW really is running or not.
So, two existing variable renames and one existing function rename. Or
if no rename, then just document thoroughly how they work now and avoid
excess code churn.
When you consider changing the unit the watchdog core is using, why not
change to nano seconds and 64 bit variables? You might be able to copy
some algorithms and ideas from the timer core that uses these.
Yes, sounds like a good idea. I will look into it.
I wouldn't be surprised if you had to build a parallel watchdog device
model for that and assert that both work until all drivers are converted
to the new model. Sounds like fun :-)
I just realized that we were talking about changing the time stamp model
for the core entirely, not just the new model... Which is much more
complicated. Damn.
Well I will think about it. There is not much new that I'm adding here
now. I'm hoping it could be done so that we just need to fill couple of
new variables in the driver and call a new init function to set up the
rest of the parameters and then clean up unneeded functionality. And the
core changes are really not that big either.
What I probably should do at least is to mark clearly or maybe factor
out the places where we deal with the old drivers and the deprecated
variables. Then this code could be removed easily once there no longer
is need for it. But I will keep on thinking it as I rework the series..
Thanks,
-Timo
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
