Re: [RFC/RFT PATCH] watchdog: Move watchdog device creation to watchdog_dev.c
From: Guenter Roeck
Date: Tue Dec 15 2015 - 21:57:15 EST
On 12/14/2015 12:44 PM, Wim Van Sebroeck wrote:
On Sun, Dec 13, 2015 at 10:24:35PM -0800, Guenter Roeck wrote:
On 12/13/2015 02:02 PM, Damien Riegel wrote:
On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
Hi All,
On 12/07/2015 08:15 AM, Damien Riegel wrote:
On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
The watchdog character device s currently created in
watchdog_dev.c, and the watchdog device in watchdog_core.c. This
results in cross-dependencies, as the device creation needs to
know the watchdog character device number.
On top of that, the watchdog character device is created before
the watchdog device is created. This can result in race conditions
if the watchdog device node is accessed before the watchdog device
has been created.
To solve the problem, move watchdog device creation into
watchdog_dev.c, and create the watchdog device prior to creating
its device node. Also move device class creation into
watchdog_dev.c, since this is now the only place where the
watchdog class is needed.
Inspired by an earlier patch set from Damien Riegel.
Cc: Damien Riegel <damien.riegel@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx> --- Hi Damien,
I think this approach would be a bit better. The watchdog device
isn't really used in the watchdog core code, so it is better
created in watchdog_dev.c. That also fits well with other pending
changes, such as sysfs attribute support, and my attempts to move
the ref/unref functions completely into the watchdog core. As a
side effect, it also cleans up the error path in
__watchdog_register_device().
What do you think ?
Hi Guenter,
Like the idea, but I don't really get the separation. For instance,
you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
in watchdog_core.c whereas it is only used for device
creation/deletion.
The class is watchdog driver internal, and it is device related, so
I think it made sense to move it to watchdog_dev.c. On top of that,
it will be needed there if/when we introduce sysfs attributes.
The watchdog id can be determined by obtaining an id using ida, or
it can be provided through the watchdog alias. The operation to get
it is not device related, and it is not straightforward to obtain
it, so I thought it makes sense to keep the code in watchdog_core.c.
Of course a lot of it is personal preference.
Let me go back to how I saw the design when I created the generic
watchdog framework: When using watchdog device drivers we need to be
able to support the /dev/watchdog system. I also foresaw that we
should have a sysfs interface and I saw the future for watchdog
devices that you should be able to choose between the 2 different
systems. You should be able to use only the /dev/watchdog interfacing,
but you should also be able to use both a sysfs interface and a
/dev/watchdog interface and it should even be possible to have only a
sysfs interface in certain embedded devices. So that's why I split the
watchdog framework over 3 files: core code, the /dev/watchdog
interfacing and the sysfs code. Since I want to have compiled code
small enough when choosing either /Dev/watchdog or sysfs or both this
sounded the most logical thing to do (Unless you have a single file
full of #ifdef-ery that becomes unreadable).
So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
listen to it and see what the benefits are. But I want a clean system
for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
(watchdog_sysfs.c) in the future. Off-course the current behaviour is
to have the /dev/ interface and have the option to add sysfs
attributes.
I agree that keeping sysfs code separate makes sense, as someone might
want to not use it.
I am not really sure about that. I don't recall a similar concern with
any other subsystem.
Anyway, sure, we can move the code to another file. Sure, we can add a
configuration option. That means we'll also need to make several functions
non-static, and possibly move some functions out of watchdog_dev.c
into yet another file. But we'll need some guidance for that and an idea
what is going to be acceptable.
The question is: can we make the /dev/watchdog entries optional ? That
would break the compatibility, right? Imho, it would be saner to keep
only one way to interact with watchdogs (ie. keep /dev/watchdog as is
and don't make it optional, and sysfs read-only and eventually
optional). I think that question should be answered before we can decide
how we want to split the code between watchdog_dev.c and watchdog_core.c
All I know for my part is that all pending structural enhancements in one
way or another depend on the assumption that the character device and the
sysfs (kernel) device both exist. I have not been able to find any code
in the kernel where cdev_add is not associated with device_create, so I
don't even know if cdev_add without device_create is even possible.
The current code in watchdog_dev.c definitely depends on watchdog->dev,
though not deeply (it uses it for some dev_ messages). If that dependency
isn't supposed to exist, we'll need to do something about it now.
We now have the sysfs changes, the changes to move "soft" timeout handling
into the kernel, multiple approaches to add pretimeout into the watchdog
core,
and my attempt to solve the ref/unref mess. All of those won't be able to
proceed without knowing how to handle the cdev vs. dev problem.
Wim, we really need some guidance from you.
Basically the options we have are:
1) go with the original design. The problem here (as Guenter pointed it out
correctly) is that sysfs went from optional to integrated with the character
devices.
2) change the origial design so that the watchdog_sysfs part is still optional
(meaning the additional read-only sysfs attributes) but is called from within
watchdog_dev.c instead of from watchdog_core.c . This would make it possible
to avoid race conditions. This would also mean that it is harder for an
embedded developer to get rid of the /dev/watchdog interface.
3) put the sysfs attributes together with the /dev/watchdog code in
watchdog_dev.c . The sysfs attributes can be optional with a minimum of
#ifdef's if needed.
4) consolidate everything in only watchdog_core.c . (if we put the sysfs and
the /dev/watchdog* code in 1 file, then why not put everything in one file).
For the linux-kernel leaving the dev/watchdog* interface and only use the sysfs
interface (but then read-write so that you can use the interface also for
starting and stopping a watchdog) would indeed mean a compatibility break.
But as an embedded developer it would not be a necessity and thus not a
compatibility issue.
Since /dev/watchdog was always tied with the misc devices, it makes sence
to follow the misc route and thus the character device route. This indeed
means that we always have the watchdog class (but also linkage to parent
devices via misc.parent).
So I think we should stick to the misc device approach (since this is how
it has always been). Which means that we also follow the character device
approach. So to me we should go for option 3 or 4.
I would suggest to go with 3), and also move the watchdog class to watchdog_dev.c.
I don't see a real reason to move everything into a single file - it would just
make code maintenance more difficult, and the current separation seems to work
quite well as far as I can see.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/