Message-ID: <566E60A3.5010102@roeck-us.net>
Date:	Sun, 13 Dec 2015 22:24:35 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Damien Riegel <damien.riegel@...oirfairelinux.com>,
	Wim Van Sebroeck <wim@...ana.be>,
	Pratyush Anand <panand@...hat.com>,
	linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC/RFT PATCH] watchdog: Move watchdog device creation to
 watchdog_dev.c

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@...oirfairelinux.com>
>>>>> Signed-off-by: Guenter Roeck <linux@...ck-us.net> --- 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.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists