Message-ID: <20151216073344.GA29639@spo001.leaseweb.nl>
Date:	Wed, 16 Dec 2015 08:33:44 +0100
From:	Wim Van Sebroeck <wim@...ana.be>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Damien Riegel <damien.riegel@...oirfairelinux.com>,
	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

Hi Guenter,

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

Then we will go with option 3.

Kind regards,
Wim.

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