Re: [RFC/RFT PATCH] watchdog: Move watchdog device creation to watchdog_dev.c

[Wim Van Sebroeck]

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.


Kind regards,
Wim.

--
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/