| Message ID | BANLkTinaQpnVxwX5rB1jrabGX6ArvhhS7g@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote: > The current MPCore watchdog driver uses a misc_dev device node. > This patch changes it to use dynamically allocated device numbers. I'm not sure that this is the correct thing to do. All other watchdog devices use a miscdevice with a major:minor of 10:130, is there a specific reason that this node needs to be dynamic? Also few other comments inline. Jamie > --- > drivers/watchdog/mpcore_wdt.c | 48 +++++++++++++++++++++++++++++------------ > 1 files changed, 34 insertions(+), 14 deletions(-) > > diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c > index 2b4af22..f6afc20 100644 > --- a/drivers/watchdog/mpcore_wdt.c > +++ b/drivers/watchdog/mpcore_wdt.c > @@ -32,6 +32,8 @@ > #include <linux/uaccess.h> > #include <linux/slab.h> > #include <linux/io.h> > +#include <linux/cdev.h> > +#include <linux/device.h> > > #include <asm/smp_twd.h> > > @@ -42,6 +44,9 @@ struct mpcore_wdt { > int irq; > unsigned int perturb; > char expect_close; > + struct cdev cdev; > + struct class *class; > + dev_t number; It looks like tabs have been converted to spaces here. > }; > > static struct platform_device *mpcore_wdt_dev; > @@ -318,12 +323,6 @@ static const struct file_operations mpcore_wdt_fops = { > .release = mpcore_wdt_release, > }; > > -static struct miscdevice mpcore_wdt_miscdev = { > - .minor = WATCHDOG_MINOR, > - .name = "watchdog", > - .fops = &mpcore_wdt_fops, > -}; > - > static int __devinit mpcore_wdt_probe(struct platform_device *dev) > { > struct mpcore_wdt *wdt; > @@ -358,14 +357,15 @@ static int __devinit mpcore_wdt_probe(struct > platform_device *dev) > goto err_free; > } > > - mpcore_wdt_miscdev.parent = &dev->dev; > - ret = misc_register(&mpcore_wdt_miscdev); > - if (ret) { > + ret = alloc_chrdev_region(&wdt->number, 0, 1, "mpcore_wdt"); > + if (ret < 0) { > dev_printk(KERN_ERR, wdt->dev, > - "cannot register miscdev on minor=%d (err=%d)\n", > - WATCHDOG_MINOR, ret); > + "cannot register with dynamic device number (err=%d)\n", -ret); > goto err_misc; > } > + dev_printk(KERN_INFO, wdt->dev, "using device number %d, %d", > MAJOR(wdt->number), MINOR(wdt->number)); > + > + cdev_init(&wdt->cdev, &mpcore_wdt_fops); > > ret = request_irq(wdt->irq, mpcore_wdt_fire, IRQF_DISABLED, > "mpcore_wdt", wdt); > @@ -379,10 +379,24 @@ static int __devinit mpcore_wdt_probe(struct > platform_device *dev) > platform_set_drvdata(dev, wdt); > mpcore_wdt_dev = dev; > > - return 0; > + ret = cdev_add(&wdt->cdev, wdt->number, 1); > + if (ret < 0) { > + dev_printk(KERN_ERR, wdt->dev, "failed to add character device\n"); > + goto err_cdev_add; > + } > > + /* create /dev/watchdog > + * we use udev to make the file > + */ > + wdt->class = class_create(THIS_MODULE,"watchdog"); > + (void) device_create(wdt->class, wdt->dev, > wdt->number,NULL,"watchdog"); The return value of class_create() and device_create() should be checked here and handled appropriately. I believe the sysfs classes are pretty much deprecated now in preference of a bus too. > + > + return 0; > + > +err_cdev_add: > + free_irq(wdt->irq, wdt); > err_irq: > - misc_deregister(&mpcore_wdt_miscdev); > + unregister_chrdev_region (wdt->number, 1); > err_misc: > iounmap(wdt->base); > err_free: > @@ -395,9 +409,15 @@ static int __devexit mpcore_wdt_remove(struct > platform_device *dev) > { > struct mpcore_wdt *wdt = platform_get_drvdata(dev); > > + device_destroy(wdt->class,wdt->number); > + class_unregister(wdt->class); > + class_destroy(wdt->class); class_destroy() calls calls_unregister() internally so you wouldn't need to call this. > + cdev_del(&wdt->cdev); > + > platform_set_drvdata(dev, NULL); > > - misc_deregister(&mpcore_wdt_miscdev); > + unregister_chrdev_region (wdt->number, 1); > > mpcore_wdt_dev = NULL; > > -- > 1.7.0.4 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote: >> The current MPCore watchdog driver uses a misc_dev device node. >> This patch changes it to use dynamically allocated device numbers. > > I'm not sure that this is the correct thing to do. All other watchdog devices > use a miscdevice with a major:minor of 10:130, is there a specific reason > that this node needs to be dynamic? I was under the impressions that dynamic device nodes were the way of the future. Is that not the case? I'll add the relevant checks in other places as per your suggestions. > I believe the sysfs classes are pretty much > deprecated now in preference of a bus too. Can you give me some more info here? I thought the sysfs stuff was the new right way of doing stuff. The class stuff allows udev to automatically create the right device node. thanks, -Pete
On Tue, Jun 14, 2011 at 05:19:06PM -0700, Peter Fordham wrote: > > On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote: > >> The current MPCore watchdog driver uses a misc_dev device node. > >> This patch changes it to use dynamically allocated device numbers. > > > > I'm not sure that this is the correct thing to do. All other watchdog devices > > use a miscdevice with a major:minor of 10:130, is there a specific reason > > that this node needs to be dynamic? > > I was under the impressions that dynamic device nodes were the way of the > future. Is that not the case? Well they are for new devices/subsystems but watchdog has an established major:minor pair that all other devices use so you don't really have to worry about a namespace clash. > I'll add the relevant checks in other places as per your suggestions. I've added Wim (watchdog driver maintainer), but I would think that if this change is worth doing then it should be done for all drivers. > > I believe the sysfs classes are pretty much > > deprecated now in preference of a bus too. > > Can you give me some more info here? I thought the sysfs stuff was the > new right way of doing stuff. The class stuff allows udev to automatically > create the right device node. So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502. So there's nothing wrong with using sysfs and a bus, but certainly a bus is preferred over a class. Jamie
> I've added Wim (watchdog driver maintainer), but I would think that if > this change is worth doing then it should be done for all drivers. Agreed, but today this may be more work than I'm strictly interested in doing. If we do decide it's the right thing then I'll look into converting the other drivers later. >> > I believe the sysfs classes are pretty much >> > deprecated now in preference of a bus too. >> >> Can you give me some more info here? I thought the sysfs stuff was the >> new right way of doing stuff. The class stuff allows udev to automatically >> create the right device node. > > So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502. So > there's nothing wrong with using sysfs and a bus, but certainly a bus is > preferred over a class. I still don't really get this. If I don't use the class code my /dev node gets the name /dev/mpcore_wdt instaed of /dev/watchdog. Is the right thing to do to use a udev rule or is there a way to tell udev via sysfs to make /dev/watchdog instead of /dev/mpcore_wdt? Pete
On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote: > > I've added Wim (watchdog driver maintainer), but I would think that if > > this change is worth doing then it should be done for all drivers. > > Agreed, but today this may be more work than I'm strictly interested in > doing. If we do decide it's the right thing then I'll look into converting the > other drivers later. So I'm not fully aware of what particular problem this is solving, but if it is needed for this driver then it should really be done for all of them. That's probably one for Wim to answer though. Wim posted a series for a generic watchdog framework a while back[1] but this still made use of a misc device. > >> > I believe the sysfs classes are pretty much > >> > deprecated now in preference of a bus too. > >> > >> Can you give me some more info here? I thought the sysfs stuff was the > >> new right way of doing stuff. The class stuff allows udev to automatically > >> create the right device node. > > > > So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502. So > > there's nothing wrong with using sysfs and a bus, but certainly a bus is > > preferred over a class. > > I still don't really get this. If I don't use the class code my /dev node gets > the name /dev/mpcore_wdt instaed of /dev/watchdog. Is the right thing > to do to use a udev rule or is there a way to tell udev via sysfs to make > /dev/watchdog instead of /dev/mpcore_wdt? If you use a bus then the device creation is slightly more involved than with a class - create the bus, allocate the device and assign the bus_type then set the name with dev_set_name(). That should be enough for udev to work out the name of the device. Jamie 1. http://www.spinics.net/lists/linux-watchdog/msg00082.html
The framework looks great. I hope it gets in soon. > If you use a bus then the device creation is slightly more involved than > with a class - create the bus, allocate the device and assign the > bus_type then set the name with dev_set_name(). That should be enough > for udev to work out the name of the device. It's a platform device so it's already on the platform bus under: /sys/devices/platform/mpcore_wdt Do I need to create another bus? The platform driver name is used in driver matching so it doesn't seem like the right thing to do to change it to watchdog. -Pete
On Wednesday 15 June 2011 21:09:45 Jamie Iles wrote: > On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote: > > > I've added Wim (watchdog driver maintainer), but I would think that if > > > this change is worth doing then it should be done for all drivers. > > > > Agreed, but today this may be more work than I'm strictly interested in > > doing. If we do decide it's the right thing then I'll look into converting the > > other drivers later. > > So I'm not fully aware of what particular problem this is solving, but > if it is needed for this driver then it should really be done for all of > them. That's probably one for Wim to answer though. Wim posted a > series for a generic watchdog framework a while back[1] but this still > made use of a misc device. Let's first wait until the generic watchdog framework gets merged and the drivers are converted. After that is in, we can discuss further changes. I highly doubt that making an incompatible API change benefits anyone here, but it certainly shouldn't be done for a single driver that is separate from the framework. Arnd
> It's a platform device so it's already on the platform bus under: > > /sys/devices/platform/mpcore_wdt sorry, that should have been: /sys/bus/platform/devices/mpcore_wdt.
On 15 June 2011 12:36, Arnd Bergmann <arnd@arndb.de> wrote: > Let's first wait until the generic watchdog framework gets merged and > the drivers are converted. After that is in, we can discuss further changes. That seems logical and reasonable. Are we saying no changes to watchdog drivers in general until this is done? because I have other patches for this driver that among other things actually make it work properly. As it stands today the timeout calculations are broken which results in random reboots. The framework patch was submitted 3.5 months ago and isn't in yet. Is it held up on something? I don't see any negative comments. > I highly doubt that making an incompatible API change benefits anyone here, > but it certainly shouldn't be done for a single driver that is separate from > the framework. Not to be a pedant but, this doesn't change the API at all. -Pete
On Wednesday 15 June 2011 21:49:43 Peter Fordham wrote: > On 15 June 2011 12:36, Arnd Bergmann <arnd@arndb.de> wrote: > > Let's first wait until the generic watchdog framework gets merged and > > the drivers are converted. After that is in, we can discuss further changes. > > That seems logical and reasonable. Are we saying no changes to watchdog > drivers in general until this is done? because I have other patches > for this driver > that among other things actually make it work properly. As it stands today > the timeout calculations are broken which results in random reboots. I mean only user-visible changes. > The framework patch was submitted 3.5 months ago and isn't in yet. Is it held up > on something? I don't see any negative comments. I think Wim has been working on this on and off for years. I'm trying to encourage him to just put it into linux-next now and merge the stuff for the 3.1 merge window. I think having over 100 drivers implementing the same interface with trivial differences is no fun any more. > > I highly doubt that making an incompatible API change benefits anyone here, > > but it certainly shouldn't be done for a single driver that is separate from > > the framework. > > Not to be a pedant but, this doesn't change the API at all. It does change the default name of the device node, and the major/minor number, which are user-visible. It's quite possible that there are systems relying on static device nodes with this driver, and I'm rather sure that that are systems around relying on static device nodes with other watchdogs. Arnd
Hi All, > On Tue, Jun 14, 2011 at 05:19:06PM -0700, Peter Fordham wrote: > > > On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote: > > >> The current MPCore watchdog driver uses a misc_dev device node. > > >> This patch changes it to use dynamically allocated device numbers. > > > > > > I'm not sure that this is the correct thing to do. All other watchdog devices > > > use a miscdevice with a major:minor of 10:130, is there a specific reason > > > that this node needs to be dynamic? > > > > I was under the impressions that dynamic device nodes were the way of the > > future. Is that not the case? > > Well they are for new devices/subsystems but watchdog has an established > major:minor pair that all other devices use so you don't really have to > worry about a namespace clash. > > > I'll add the relevant checks in other places as per your suggestions. > > I've added Wim (watchdog driver maintainer), but I would think that if > this change is worth doing then it should be done for all drivers. > > > > I believe the sysfs classes are pretty much > > > deprecated now in preference of a bus too. > > > > Can you give me some more info here? I thought the sysfs stuff was the > > new right way of doing stuff. The class stuff allows udev to automatically > > create the right device node. > > So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502. So > there's nothing wrong with using sysfs and a bus, but certainly a bus is > preferred over a class. My opinion: we stick with the /dev/watchdog interface, we don't go for dynamic device nodes but go for a sysfs interface. And this for all drivers and via the new watchdog API. (updated version will be sent out today). Makes no sense to do this for every driver. Kind regards, Wim.
Hi All, > > I've added Wim (watchdog driver maintainer), but I would think that if > > this change is worth doing then it should be done for all drivers. > > Agreed, but today this may be more work than I'm strictly interested in > doing. If we do decide it's the right thing then I'll look into converting the > other drivers later. No, we need to do it by first converting all drivers to the new API and then a change of the API will take care of it. Kind regards, Wim.
Hi Arnd, > On Wednesday 15 June 2011 21:09:45 Jamie Iles wrote: > > On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote: > > > > I've added Wim (watchdog driver maintainer), but I would think that if > > > > this change is worth doing then it should be done for all drivers. > > > > > > Agreed, but today this may be more work than I'm strictly interested in > > > doing. If we do decide it's the right thing then I'll look into converting the > > > other drivers later. > > > > So I'm not fully aware of what particular problem this is solving, but > > if it is needed for this driver then it should really be done for all of > > them. That's probably one for Wim to answer though. Wim posted a > > series for a generic watchdog framework a while back[1] but this still > > made use of a misc device. > > Let's first wait until the generic watchdog framework gets merged and > the drivers are converted. After that is in, we can discuss further changes. > > I highly doubt that making an incompatible API change benefits anyone here, > but it certainly shouldn't be done for a single driver that is separate from > the framework. Agree for the full 100%. Thanks, Wim.
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c index 2b4af22..f6afc20 100644 --- a/drivers/watchdog/mpcore_wdt.c +++ b/drivers/watchdog/mpcore_wdt.c @@ -32,6 +32,8 @@ #include <linux/uaccess.h> #include <linux/slab.h> #include <linux/io.h> +#include <linux/cdev.h> +#include <linux/device.h> #include <asm/smp_twd.h> @@ -42,6 +44,9 @@ struct mpcore_wdt { int irq; unsigned int perturb; char expect_close; + struct cdev cdev; + struct class *class; + dev_t number; }; static struct platform_device *mpcore_wdt_dev; @@ -318,12 +323,6 @@ static const struct file_operations mpcore_wdt_fops = { .release = mpcore_wdt_release, }; -static struct miscdevice mpcore_wdt_miscdev = { - .minor = WATCHDOG_MINOR, - .name = "watchdog", - .fops = &mpcore_wdt_fops, -}; - static int __devinit mpcore_wdt_probe(struct platform_device *dev) { struct mpcore_wdt *wdt; @@ -358,14 +357,15 @@ static int __devinit mpcore_wdt_probe(struct platform_device *dev) goto err_free; } - mpcore_wdt_miscdev.parent = &dev->dev; - ret = misc_register(&mpcore_wdt_miscdev); - if (ret) { + ret = alloc_chrdev_region(&wdt->number, 0, 1, "mpcore_wdt"); + if (ret < 0) { dev_printk(KERN_ERR, wdt->dev, - "cannot register miscdev on minor=%d (err=%d)\n", - WATCHDOG_MINOR, ret); + "cannot register with dynamic device number (err=%d)\n", -ret); goto err_misc; } + dev_printk(KERN_INFO, wdt->dev, "using device number %d, %d", MAJOR(wdt->number), MINOR(wdt->number)); + + cdev_init(&wdt->cdev, &mpcore_wdt_fops);
The current MPCore watchdog driver uses a misc_dev device node. This patch changes it to use dynamically allocated device numbers. --- drivers/watchdog/mpcore_wdt.c | 48 +++++++++++++++++++++++++++++------------ 1 files changed, 34 insertions(+), 14 deletions(-) ret = request_irq(wdt->irq, mpcore_wdt_fire, IRQF_DISABLED, "mpcore_wdt", wdt); @@ -379,10 +379,24 @@ static int __devinit mpcore_wdt_probe(struct platform_device *dev) platform_set_drvdata(dev, wdt); mpcore_wdt_dev = dev; - return 0; + ret = cdev_add(&wdt->cdev, wdt->number, 1); + if (ret < 0) { + dev_printk(KERN_ERR, wdt->dev, "failed to add character device\n"); + goto err_cdev_add; + } + /* create /dev/watchdog + * we use udev to make the file + */ + wdt->class = class_create(THIS_MODULE,"watchdog"); + (void) device_create(wdt->class, wdt->dev, wdt->number,NULL,"watchdog"); + + return 0; + +err_cdev_add: + free_irq(wdt->irq, wdt); err_irq: - misc_deregister(&mpcore_wdt_miscdev); + unregister_chrdev_region (wdt->number, 1); err_misc: iounmap(wdt->base); err_free: @@ -395,9 +409,15 @@ static int __devexit mpcore_wdt_remove(struct platform_device *dev) { struct mpcore_wdt *wdt = platform_get_drvdata(dev); + device_destroy(wdt->class,wdt->number); + class_unregister(wdt->class); + class_destroy(wdt->class); + + cdev_del(&wdt->cdev); + platform_set_drvdata(dev, NULL); - misc_deregister(&mpcore_wdt_miscdev); + unregister_chrdev_region (wdt->number, 1); mpcore_wdt_dev = NULL;