LinuxLists.cc - [KJ][PATCH] Correct misc_register return code handling in several drivers

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Mon, Oct 23, 2006 at 06:39:24PM +0100, Alan Cox wrote:
> Ar Llu, 2006-10-23 am 13:19 -0400, ysgrifennodd Neil Horman:
> > + for_each_online_node(node) {
> > + if(timers[node] != NULL)
> > + kfree(timers[node]);
> > + }
>
> The if test appears to be redundant as kfree(NULL) is a valid no-op.
>
> > + if (misc_register(&hp_sdc_rtc_dev) != 0)
> > + printk(KERN_INFO "Could not register misc. dev for sdc\n");
>
> Many users will misconstrue this has a hard disk name - is there a
> better name to use.
>
> > if (ret)
> > printk(KERN_ERR "tun: Can't register misc device %d\n", TUN_MINOR);
>
> These all really show that we should pass the name into the misc
> register and do the printk there to cut down on random name variants and
> kernel size. Separate problem, just noting it in case someone feels
> inspired 8)
>
> Alan

New patch attached, taking Alan's suggestions into account. I'll consolidate
the printk error messages into misc_register in a later patch when I have a
moment. Thanks Alan.

Regards
Neil

Signed-off-by: Neil Horman <[email protected]>

drivers/char/mmtimer.c | 28 ++++++++++++++++++++++------
drivers/char/tpm/tpm.c | 1 +
drivers/input/misc/hp_sdc_rtc.c | 6 +++++-
drivers/macintosh/apm_emu.c | 5 ++++-
drivers/net/tun.c | 2 ++
fs/dlm/user.c | 1 +
6 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c
--- a/drivers/char/mmtimer.c
+++ b/drivers/char/mmtimer.c
@@ -669,6 +669,7 @@ static struct k_clock sgi_clock = {
static int __init mmtimer_init(void)
{
unsigned i;
+ int err = -1;
cnodeid_t node, maxn = -1;

if (!ia64_platform_is("sn2"))
@@ -680,7 +681,7 @@ static int __init mmtimer_init(void)
if (sn_rtc_cycles_per_second < 100000) {
printk(KERN_ERR "%s: unable to determine clock frequency\n",
MMTIMER_NAME);
- return -1;
+ goto out;
}

mmtimer_femtoperiod = ((unsigned long)1E15 + sn_rtc_cycles_per_second /
@@ -689,13 +690,13 @@ static int __init mmtimer_init(void)
if (request_irq(SGI_MMTIMER_VECTOR, mmtimer_interrupt, IRQF_PERCPU, MMTIMER_NAME, NULL)) {
printk(KERN_WARNING "%s: unable to allocate interrupt.",
MMTIMER_NAME);
- return -1;
+ goto out;
}

if (misc_register(&mmtimer_miscdev)) {
printk(KERN_ERR "%s: failed to register device\n",
MMTIMER_NAME);
- return -1;
+ goto out1;
}

/* Get max numbered node, calculate slots needed */
@@ -709,16 +710,18 @@ static int __init mmtimer_init(void)
if (timers == NULL) {
printk(KERN_ERR "%s: failed to allocate memory for device\n",
MMTIMER_NAME);
- return -1;
+ goto out2;
}

+ memset(timers,0,(sizeof(mmtimer_t *)*maxn));
+
/* Allocate mmtimer_t's for each online node */
for_each_online_node(node) {
timers[node] = kmalloc_node(sizeof(mmtimer_t)*NUM_COMPARATORS, GFP_KERNEL, node);
if (timers[node] == NULL) {
printk(KERN_ERR "%s: failed to allocate memory for device\n",
MMTIMER_NAME);
- return -1;
+ goto out3;
}
for (i=0; i< NUM_COMPARATORS; i++) {
mmtimer_t * base = timers[node] + i;
@@ -738,7 +741,20 @@ static int __init mmtimer_init(void)
printk(KERN_INFO "%s: v%s, %ld MHz\n", MMTIMER_DESC, MMTIMER_VERSION,
sn_rtc_cycles_per_second/(unsigned long)1E6);

- return 0;
+ err = 0;
+out:
+ return err;
+
+out3:
+ for_each_online_node(node) {
+ kfree(timers[node]);
+ }
+out2:
+ misc_deregister(&mmtimer_miscdev);
+out1:
+ free_irq(SGI_MMTIMER_VECTOR, NULL);
+ goto out;
+
}

module_init(mmtimer_init);
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1155,6 +1155,7 @@ #define DEVNAME_SIZE 7

if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
list_del(&chip->list);
+ misc_deregister(&chip->vendor.miscdev);
put_device(dev);
clear_bit(chip->dev_num, dev_mask);
kfree(chip);
diff --git a/drivers/input/misc/hp_sdc_rtc.c b/drivers/input/misc/hp_sdc_rtc.c
--- a/drivers/input/misc/hp_sdc_rtc.c
+++ b/drivers/input/misc/hp_sdc_rtc.c
@@ -693,9 +693,13 @@ static int __init hp_sdc_rtc_init(void)

init_MUTEX(&i8042tregs);

+ INIT_LIST_HEAD(&hp_sdc_rtc_dev.list);
+
if ((ret = hp_sdc_request_timer_irq(&hp_sdc_rtc_isr)))
return ret;
- misc_register(&hp_sdc_rtc_dev);
+ if (misc_register(&hp_sdc_rtc_dev) != 0)
+ printk(KERN_INFO "Could not register misc. dev for i8042 rtc\n");
+
create_proc_read_entry ("driver/rtc", 0, NULL,
hp_sdc_rtc_read_proc, NULL);

diff --git a/drivers/macintosh/apm_emu.c b/drivers/macintosh/apm_emu.c
--- a/drivers/macintosh/apm_emu.c
+++ b/drivers/macintosh/apm_emu.c
@@ -520,6 +520,8 @@ static int __init apm_emu_init(void)
{
struct proc_dir_entry *apm_proc;

+ INIT_LIST_HEAD(&apm_device.list);
+
if (sys_ctrler != SYS_CTRLER_PMU) {
printk(KERN_INFO "apm_emu: Requires a machine with a PMU.\n");
return -ENODEV;
@@ -529,7 +531,8 @@ static int __init apm_emu_init(void)
if (apm_proc)
apm_proc->owner = THIS_MODULE;

- misc_register(&apm_device);
+ if (misc_register(&apm_device) != 0)
+ printk(KERN_INFO "Could not create misc. device for apm\n");

pmu_register_sleep_notifier(&apm_sleep_notifier);

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -856,6 +856,8 @@ static int __init tun_init(void)
printk(KERN_INFO "tun: %s, %s\n", DRV_DESCRIPTION, DRV_VERSION);
printk(KERN_INFO "tun: %s\n", DRV_COPYRIGHT);

+ INIT_LIST_HEAD(&tun_miscdev.list);
+
ret = misc_register(&tun_miscdev);
if (ret)
printk(KERN_ERR "tun: Can't register misc device %d\n", TUN_MINOR);
diff --git a/fs/dlm/user.c b/fs/dlm/user.c
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -773,6 +773,7 @@ int dlm_user_init(void)
ctl_device.name = "dlm-control";
ctl_device.fops = &ctl_device_fops;
ctl_device.minor = MISC_DYNAMIC_MINOR;
+ INIT_LIST_HEAD(&ctl_device.list);

error = misc_register(&ctl_device);
if (error)
--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-10-23 18:01:40

by Dan Carpenter

[permalink] [raw]

Subject: Re: [KJ] [PATCH] Correct misc_register return code handling in several drivers

On 10/23/06, Neil Horman <[email protected]> wrote:
> +out3:
> + for_each_online_node(node) {
> + if(timers[node] != NULL)
> + kfree(timers[node]);
> + }

Tharindu is going to be unhappy out if he sees that. There is a
possibility that timers[node] is uninitialized. if node[0] is null
then node[1] is uninitialized and it's going to cause a crash.

regards,
dan carpenter

2006-10-23 18:17:47

[permalink] [raw]

Subject: Re: [KJ] [PATCH] Correct misc_register return code handling in several drivers

On Mon, Oct 23, 2006 at 11:01:36AM -0700, Dan Carpenter wrote:
> On 10/23/06, Neil Horman <[email protected]> wrote:
> >+out3:
> >+ for_each_online_node(node) {
> >+ if(timers[node] != NULL)
> >+ kfree(timers[node]);
> >+ }
>
> Tharindu is going to be unhappy out if he sees that. There is a
> possibility that timers[node] is uninitialized. if node[0] is null
> then node[1] is uninitialized and it's going to cause a crash.
>
> regards,
> dan carpenter

Theres a memset to ensure that all the timer pointers are initalized to NULL in
the patch:

@@ -709,16 +710,18 @@ static int __init mmtimer_init(void)
if (timers == NULL) {
printk(KERN_ERR "%s: failed to allocate memory for device\n",
MMTIMER_NAME);
- return -1;
+ goto out2;
}

+ memset(timers,0,(sizeof(mmtimer_t *)*maxn));
+

--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-10-23 18:23:26

by Kylene Jo Hall

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

For tpm:

Ack'ed-by: Kylene Hall <[email protected]>

On Mon, 2006-10-23 at 13:54 -0400, Neil Horman wrote:
> On Mon, Oct 23, 2006 at 06:39:24PM +0100, Alan Cox wrote:
> > Ar Llu, 2006-10-23 am 13:19 -0400, ysgrifennodd Neil Horman:
> > > + for_each_online_node(node) {
> > > + if(timers[node] != NULL)
> > > + kfree(timers[node]);
> > > + }
> >
> > The if test appears to be redundant as kfree(NULL) is a valid no-op.
> >
> > > + if (misc_register(&hp_sdc_rtc_dev) != 0)
> > > + printk(KERN_INFO "Could not register misc. dev for sdc\n");
> >
> > Many users will misconstrue this has a hard disk name - is there a
> > better name to use.
> >
> > > if (ret)
> > > printk(KERN_ERR "tun: Can't register misc device %d\n", TUN_MINOR);
> >
> > These all really show that we should pass the name into the misc
> > register and do the printk there to cut down on random name variants and
> > kernel size. Separate problem, just noting it in case someone feels
> > inspired 8)
> >
> > Alan
>
> New patch attached, taking Alan's suggestions into account. I'll consolidate
> the printk error messages into misc_register in a later patch when I have a
> moment. Thanks Alan.
>
> Regards
> Neil
>
>
> Signed-off-by: Neil Horman <[email protected]>
>
> drivers/char/mmtimer.c | 28 ++++++++++++++++++++++------
> drivers/char/tpm/tpm.c | 1 +
> drivers/input/misc/hp_sdc_rtc.c | 6 +++++-
> drivers/macintosh/apm_emu.c | 5 ++++-
> drivers/net/tun.c | 2 ++
> fs/dlm/user.c | 1 +
> 6 files changed, 35 insertions(+), 8 deletions(-)
>
>
>
> diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c
> --- a/drivers/char/mmtimer.c
> +++ b/drivers/char/mmtimer.c
> @@ -669,6 +669,7 @@ static struct k_clock sgi_clock = {
> static int __init mmtimer_init(void)
> {
> unsigned i;
> + int err = -1;
> cnodeid_t node, maxn = -1;
>
> if (!ia64_platform_is("sn2"))
> @@ -680,7 +681,7 @@ static int __init mmtimer_init(void)
> if (sn_rtc_cycles_per_second < 100000) {
> printk(KERN_ERR "%s: unable to determine clock frequency\n",
> MMTIMER_NAME);
> - return -1;
> + goto out;
> }
>
> mmtimer_femtoperiod = ((unsigned long)1E15 + sn_rtc_cycles_per_second /
> @@ -689,13 +690,13 @@ static int __init mmtimer_init(void)
> if (request_irq(SGI_MMTIMER_VECTOR, mmtimer_interrupt, IRQF_PERCPU, MMTIMER_NAME, NULL)) {
> printk(KERN_WARNING "%s: unable to allocate interrupt.",
> MMTIMER_NAME);
> - return -1;
> + goto out;
> }
>
> if (misc_register(&mmtimer_miscdev)) {
> printk(KERN_ERR "%s: failed to register device\n",
> MMTIMER_NAME);
> - return -1;
> + goto out1;
> }
>
> /* Get max numbered node, calculate slots needed */
> @@ -709,16 +710,18 @@ static int __init mmtimer_init(void)
> if (timers == NULL) {
> printk(KERN_ERR "%s: failed to allocate memory for device\n",
> MMTIMER_NAME);
> - return -1;
> + goto out2;
> }
>
> + memset(timers,0,(sizeof(mmtimer_t *)*maxn));
> +
> /* Allocate mmtimer_t's for each online node */
> for_each_online_node(node) {
> timers[node] = kmalloc_node(sizeof(mmtimer_t)*NUM_COMPARATORS, GFP_KERNEL, node);
> if (timers[node] == NULL) {
> printk(KERN_ERR "%s: failed to allocate memory for device\n",
> MMTIMER_NAME);
> - return -1;
> + goto out3;
> }
> for (i=0; i< NUM_COMPARATORS; i++) {
> mmtimer_t * base = timers[node] + i;
> @@ -738,7 +741,20 @@ static int __init mmtimer_init(void)
> printk(KERN_INFO "%s: v%s, %ld MHz\n", MMTIMER_DESC, MMTIMER_VERSION,
> sn_rtc_cycles_per_second/(unsigned long)1E6);
>
> - return 0;
> + err = 0;
> +out:
> + return err;
> +
> +out3:
> + for_each_online_node(node) {
> + kfree(timers[node]);
> + }
> +out2:
> + misc_deregister(&mmtimer_miscdev);
> +out1:
> + free_irq(SGI_MMTIMER_VECTOR, NULL);
> + goto out;
> +
> }
>
> module_init(mmtimer_init);
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1155,6 +1155,7 @@ #define DEVNAME_SIZE 7
>
> if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
> list_del(&chip->list);
> + misc_deregister(&chip->vendor.miscdev);
> put_device(dev);
> clear_bit(chip->dev_num, dev_mask);
> kfree(chip);
> diff --git a/drivers/input/misc/hp_sdc_rtc.c b/drivers/input/misc/hp_sdc_rtc.c
> --- a/drivers/input/misc/hp_sdc_rtc.c
> +++ b/drivers/input/misc/hp_sdc_rtc.c
> @@ -693,9 +693,13 @@ static int __init hp_sdc_rtc_init(void)
>
> init_MUTEX(&i8042tregs);
>
> + INIT_LIST_HEAD(&hp_sdc_rtc_dev.list);
> +
> if ((ret = hp_sdc_request_timer_irq(&hp_sdc_rtc_isr)))
> return ret;
> - misc_register(&hp_sdc_rtc_dev);
> + if (misc_register(&hp_sdc_rtc_dev) != 0)
> + printk(KERN_INFO "Could not register misc. dev for i8042 rtc\n");
> +
> create_proc_read_entry ("driver/rtc", 0, NULL,
> hp_sdc_rtc_read_proc, NULL);
>
> diff --git a/drivers/macintosh/apm_emu.c b/drivers/macintosh/apm_emu.c
> --- a/drivers/macintosh/apm_emu.c
> +++ b/drivers/macintosh/apm_emu.c
> @@ -520,6 +520,8 @@ static int __init apm_emu_init(void)
> {
> struct proc_dir_entry *apm_proc;
>
> + INIT_LIST_HEAD(&apm_device.list);
> +
> if (sys_ctrler != SYS_CTRLER_PMU) {
> printk(KERN_INFO "apm_emu: Requires a machine with a PMU.\n");
> return -ENODEV;
> @@ -529,7 +531,8 @@ static int __init apm_emu_init(void)
> if (apm_proc)
> apm_proc->owner = THIS_MODULE;
>
> - misc_register(&apm_device);
> + if (misc_register(&apm_device) != 0)
> + printk(KERN_INFO "Could not create misc. device for apm\n");
>
> pmu_register_sleep_notifier(&apm_sleep_notifier);
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -856,6 +856,8 @@ static int __init tun_init(void)
> printk(KERN_INFO "tun: %s, %s\n", DRV_DESCRIPTION, DRV_VERSION);
> printk(KERN_INFO "tun: %s\n", DRV_COPYRIGHT);
>
> + INIT_LIST_HEAD(&tun_miscdev.list);
> +
> ret = misc_register(&tun_miscdev);
> if (ret)
> printk(KERN_ERR "tun: Can't register misc device %d\n", TUN_MINOR);
> diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> --- a/fs/dlm/user.c
> +++ b/fs/dlm/user.c
> @@ -773,6 +773,7 @@ int dlm_user_init(void)
> ctl_device.name = "dlm-control";
> ctl_device.fops = &ctl_device_fops;
> ctl_device.minor = MISC_DYNAMIC_MINOR;
> + INIT_LIST_HEAD(&ctl_device.list);
>
> error = misc_register(&ctl_device);
> if (error)

2006-10-23 18:32:12

by Dan Carpenter

[permalink] [raw]

Subject: Re: [KJ] [PATCH] Correct misc_register return code handling in several drivers

On 10/23/06, Neil Horman <[email protected]> wrote:
> On Mon, Oct 23, 2006 at 11:01:36AM -0700, Dan Carpenter wrote:
> > On 10/23/06, Neil Horman <[email protected]> wrote:
> > >+out3:
> > >+ for_each_online_node(node) {
> > >+ if(timers[node] != NULL)
> > >+ kfree(timers[node]);
> > >+ }
> >
> > Tharindu is going to be unhappy out if he sees that. There is a
> > possibility that timers[node] is uninitialized. if node[0] is null
> > then node[1] is uninitialized and it's going to cause a crash.
> >
> > regards,
> > dan carpenter
>
>
> Theres a memset to ensure that all the timer pointers are initalized to NULL in
> the patch:
>
> @@ -709,16 +710,18 @@ static int __init mmtimer_init(void)
> if (timers == NULL) {
> printk(KERN_ERR "%s: failed to allocate memory for device\n",
> MMTIMER_NAME);
> - return -1;
> + goto out2;
> }
>
> + memset(timers,0,(sizeof(mmtimer_t *)*maxn));
> +
>
>

Ah. Great. Sorry, didn't notice that you'd taken care of that
already. Looks good.

regards,
dan carpenter

> --
> /***************************************************
> *Neil Horman
> *Software Engineer
> *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
> ***************************************************/
>

2006-10-23 18:45:41

[permalink] [raw]

Subject: Re: [KJ] [PATCH] Correct misc_register return code handling in several drivers

On Mon, Oct 23, 2006 at 11:32:08AM -0700, Dan Carpenter wrote:
> On 10/23/06, Neil Horman <[email protected]> wrote:
> >On Mon, Oct 23, 2006 at 11:01:36AM -0700, Dan Carpenter wrote:
> >> On 10/23/06, Neil Horman <[email protected]> wrote:
> >> >+out3:
> >> >+ for_each_online_node(node) {
> >> >+ if(timers[node] != NULL)
> >> >+ kfree(timers[node]);
> >> >+ }
> >>
> >> Tharindu is going to be unhappy out if he sees that. There is a
> >> possibility that timers[node] is uninitialized. if node[0] is null
> >> then node[1] is uninitialized and it's going to cause a crash.
> >>
> >> regards,
> >> dan carpenter
> >
> >
> >Theres a memset to ensure that all the timer pointers are initalized to
> >NULL in
> >the patch:
> >
> >@@ -709,16 +710,18 @@ static int __init mmtimer_init(void)
> > if (timers == NULL) {
> > printk(KERN_ERR "%s: failed to allocate memory for
> > device\n",
> > MMTIMER_NAME);
> >- return -1;
> >+ goto out2;
> > }
> >
> >+ memset(timers,0,(sizeof(mmtimer_t *)*maxn));
> >+
> >
> >
>
> Ah. Great. Sorry, didn't notice that you'd taken care of that
> already. Looks good.
>
No worries. Thanks for taking the time to look
Neil

> regards,
> dan carpenter
>
> >--
> >/***************************************************
> > *Neil Horman
> > *Software Engineer
> > *gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
> > ***************************************************/
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-10-23 19:21:21

by Alan

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

Ar Llu, 2006-10-23 am 13:54 -0400, ysgrifennodd Neil Horman:
> On Mon, Oct 23, 2006 at 06:39:24PM +0100, Alan Cox wrote:
> New patch attached, taking Alan's suggestions into account. I'll consolidate
> the printk error messages into misc_register in a later patch when I have a
> moment. Thanks Alan.
> Signed-off-by: Neil Horman <[email protected]>

Acked-by: Alan Cox <[email protected]>

2006-10-24 03:40:21

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Mon, 2006-10-23 at 13:19 -0400, Neil Horman wrote:
> Hey All-
> Janitor patch to clean up return code handling and exit from failed
> calls to misc_register accross several modules.

The patch doesn't match the description... What are those INIT_LIST_HEAD
things ? Is this something I've missed or is this a new requirement for
all misc devices ? Can't it be statically initialized instead ?

> Thanks & Regards
> Neil
>
> Signed-off-by: Neil Horman <[email protected]>
>
>
> drivers/char/mmtimer.c | 29 +++++++++++++++++++++++------
> drivers/char/tpm/tpm.c | 1 +
> drivers/input/misc/hp_sdc_rtc.c | 6 +++++-
> drivers/macintosh/apm_emu.c | 5 ++++-
> drivers/net/tun.c | 2 ++
> fs/dlm/user.c | 1 +
> 6 files changed, 36 insertions(+), 8 deletions(-)
>
>
> diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c
> --- a/drivers/char/mmtimer.c
> +++ b/drivers/char/mmtimer.c
> @@ -669,6 +669,7 @@ static struct k_clock sgi_clock = {
> static int __init mmtimer_init(void)
> {
> unsigned i;
> + int err = -1;
> cnodeid_t node, maxn = -1;
>
> if (!ia64_platform_is("sn2"))
> @@ -680,7 +681,7 @@ static int __init mmtimer_init(void)
> if (sn_rtc_cycles_per_second < 100000) {
> printk(KERN_ERR "%s: unable to determine clock frequency\n",
> MMTIMER_NAME);
> - return -1;
> + goto out;
> }
>
> mmtimer_femtoperiod = ((unsigned long)1E15 + sn_rtc_cycles_per_second /
> @@ -689,13 +690,13 @@ static int __init mmtimer_init(void)
> if (request_irq(SGI_MMTIMER_VECTOR, mmtimer_interrupt, IRQF_PERCPU, MMTIMER_NAME, NULL)) {
> printk(KERN_WARNING "%s: unable to allocate interrupt.",
> MMTIMER_NAME);
> - return -1;
> + goto out;
> }
>
> if (misc_register(&mmtimer_miscdev)) {
> printk(KERN_ERR "%s: failed to register device\n",
> MMTIMER_NAME);
> - return -1;
> + goto out1;
> }
>
> /* Get max numbered node, calculate slots needed */
> @@ -709,16 +710,18 @@ static int __init mmtimer_init(void)
> if (timers == NULL) {
> printk(KERN_ERR "%s: failed to allocate memory for device\n",
> MMTIMER_NAME);
> - return -1;
> + goto out2;
> }
>
> + memset(timers,0,(sizeof(mmtimer_t *)*maxn));
> +
> /* Allocate mmtimer_t's for each online node */
> for_each_online_node(node) {
> timers[node] = kmalloc_node(sizeof(mmtimer_t)*NUM_COMPARATORS, GFP_KERNEL, node);
> if (timers[node] == NULL) {
> printk(KERN_ERR "%s: failed to allocate memory for device\n",
> MMTIMER_NAME);
> - return -1;
> + goto out3;
> }
> for (i=0; i< NUM_COMPARATORS; i++) {
> mmtimer_t * base = timers[node] + i;
> @@ -738,7 +741,21 @@ static int __init mmtimer_init(void)
> printk(KERN_INFO "%s: v%s, %ld MHz\n", MMTIMER_DESC, MMTIMER_VERSION,
> sn_rtc_cycles_per_second/(unsigned long)1E6);
>
> - return 0;
> + err = 0;
> +out:
> + return err;
> +
> +out3:
> + for_each_online_node(node) {
> + if(timers[node] != NULL)
> + kfree(timers[node]);
> + }
> +out2:
> + misc_deregister(&mmtimer_miscdev);
> +out1:
> + free_irq(SGI_MMTIMER_VECTOR, NULL);
> + goto out;
> +
> }
>
> module_init(mmtimer_init);
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1155,6 +1155,7 @@ #define DEVNAME_SIZE 7
>
> if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
> list_del(&chip->list);
> + misc_deregister(&chip->vendor.miscdev);
> put_device(dev);
> clear_bit(chip->dev_num, dev_mask);
> kfree(chip);
> diff --git a/drivers/input/misc/hp_sdc_rtc.c b/drivers/input/misc/hp_sdc_rtc.c
> --- a/drivers/input/misc/hp_sdc_rtc.c
> +++ b/drivers/input/misc/hp_sdc_rtc.c
> @@ -693,9 +693,13 @@ static int __init hp_sdc_rtc_init(void)
>
> init_MUTEX(&i8042tregs);
>
> + INIT_LIST_HEAD(&hp_sdc_rtc_dev.list);
> +
> if ((ret = hp_sdc_request_timer_irq(&hp_sdc_rtc_isr)))
> return ret;
> - misc_register(&hp_sdc_rtc_dev);
> + if (misc_register(&hp_sdc_rtc_dev) != 0)
> + printk(KERN_INFO "Could not register misc. dev for sdc\n");
> +
> create_proc_read_entry ("driver/rtc", 0, NULL,
> hp_sdc_rtc_read_proc, NULL);
>
> diff --git a/drivers/macintosh/apm_emu.c b/drivers/macintosh/apm_emu.c
> --- a/drivers/macintosh/apm_emu.c
> +++ b/drivers/macintosh/apm_emu.c
> @@ -520,6 +520,8 @@ static int __init apm_emu_init(void)
> {
> struct proc_dir_entry *apm_proc;
>
> + INIT_LIST_HEAD(&apm_device.list);
> +
> if (sys_ctrler != SYS_CTRLER_PMU) {
> printk(KERN_INFO "apm_emu: Requires a machine with a PMU.\n");
> return -ENODEV;
> @@ -529,7 +531,8 @@ static int __init apm_emu_init(void)
> if (apm_proc)
> apm_proc->owner = THIS_MODULE;
>
> - misc_register(&apm_device);
> + if (misc_register(&apm_device) != 0)
> + printk(KERN_INFO "Could not create misc. device for apm\n");
>
> pmu_register_sleep_notifier(&apm_sleep_notifier);
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -856,6 +856,8 @@ static int __init tun_init(void)
> printk(KERN_INFO "tun: %s, %s\n", DRV_DESCRIPTION, DRV_VERSION);
> printk(KERN_INFO "tun: %s\n", DRV_COPYRIGHT);
>
> + INIT_LIST_HEAD(&tun_miscdev.list);
> +
> ret = misc_register(&tun_miscdev);
> if (ret)
> printk(KERN_ERR "tun: Can't register misc device %d\n", TUN_MINOR);
> diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> --- a/fs/dlm/user.c
> +++ b/fs/dlm/user.c
> @@ -773,6 +773,7 @@ int dlm_user_init(void)
> ctl_device.name = "dlm-control";
> ctl_device.fops = &ctl_device_fops;
> ctl_device.minor = MISC_DYNAMIC_MINOR;
> + INIT_LIST_HEAD(&ctl_device.list);
>
> error = misc_register(&ctl_device);
> if (error)

2006-10-24 12:54:06

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Tue, Oct 24, 2006 at 01:34:34PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2006-10-23 at 13:19 -0400, Neil Horman wrote:
> > Hey All-
> > Janitor patch to clean up return code handling and exit from failed
> > calls to misc_register accross several modules.
>
> The patch doesn't match the description... What are those INIT_LIST_HEAD
> things ? Is this something I've missed or is this a new requirement for
> all misc devices ? Can't it be statically initialized instead ?
>

The INIT_LIST_HEAD is there to prevent a potential oops on module removal.
misc_register, if it fails, leaves miscdevice.list unchanged. That means its
next and prev pointers contain NULL or garbage, when both pointers should contain
&miscdevice.list. If we don't do that, then there is a chance we will oops on
module removal when we do a list_del in misc_deregister on the moudule_exit
routine. I could have done this statically, but I thought it looked cleaner to
do it with the macro in the code.

Thanks & Regards
Neil

> > Thanks & Regards
> > Neil
> >
> > Signed-off-by: Neil Horman <[email protected]>
> >
> >
> > drivers/char/mmtimer.c | 29 +++++++++++++++++++++++------
> > drivers/char/tpm/tpm.c | 1 +
> > drivers/input/misc/hp_sdc_rtc.c | 6 +++++-
> > drivers/macintosh/apm_emu.c | 5 ++++-
> > drivers/net/tun.c | 2 ++
> > fs/dlm/user.c | 1 +
> > 6 files changed, 36 insertions(+), 8 deletions(-)
> >
> >
> > diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c
> > --- a/drivers/char/mmtimer.c
> > +++ b/drivers/char/mmtimer.c
> > @@ -669,6 +669,7 @@ static struct k_clock sgi_clock = {
> > static int __init mmtimer_init(void)
> > {
> > unsigned i;
> > + int err = -1;
> > cnodeid_t node, maxn = -1;
> >
> > if (!ia64_platform_is("sn2"))
> > @@ -680,7 +681,7 @@ static int __init mmtimer_init(void)
> > if (sn_rtc_cycles_per_second < 100000) {
> > printk(KERN_ERR "%s: unable to determine clock frequency\n",
> > MMTIMER_NAME);
> > - return -1;
> > + goto out;
> > }
> >
> > mmtimer_femtoperiod = ((unsigned long)1E15 + sn_rtc_cycles_per_second /
> > @@ -689,13 +690,13 @@ static int __init mmtimer_init(void)
> > if (request_irq(SGI_MMTIMER_VECTOR, mmtimer_interrupt, IRQF_PERCPU, MMTIMER_NAME, NULL)) {
> > printk(KERN_WARNING "%s: unable to allocate interrupt.",
> > MMTIMER_NAME);
> > - return -1;
> > + goto out;
> > }
> >
> > if (misc_register(&mmtimer_miscdev)) {
> > printk(KERN_ERR "%s: failed to register device\n",
> > MMTIMER_NAME);
> > - return -1;
> > + goto out1;
> > }
> >
> > /* Get max numbered node, calculate slots needed */
> > @@ -709,16 +710,18 @@ static int __init mmtimer_init(void)
> > if (timers == NULL) {
> > printk(KERN_ERR "%s: failed to allocate memory for device\n",
> > MMTIMER_NAME);
> > - return -1;
> > + goto out2;
> > }
> >
> > + memset(timers,0,(sizeof(mmtimer_t *)*maxn));
> > +
> > /* Allocate mmtimer_t's for each online node */
> > for_each_online_node(node) {
> > timers[node] = kmalloc_node(sizeof(mmtimer_t)*NUM_COMPARATORS, GFP_KERNEL, node);
> > if (timers[node] == NULL) {
> > printk(KERN_ERR "%s: failed to allocate memory for device\n",
> > MMTIMER_NAME);
> > - return -1;
> > + goto out3;
> > }
> > for (i=0; i< NUM_COMPARATORS; i++) {
> > mmtimer_t * base = timers[node] + i;
> > @@ -738,7 +741,21 @@ static int __init mmtimer_init(void)
> > printk(KERN_INFO "%s: v%s, %ld MHz\n", MMTIMER_DESC, MMTIMER_VERSION,
> > sn_rtc_cycles_per_second/(unsigned long)1E6);
> >
> > - return 0;
> > + err = 0;
> > +out:
> > + return err;
> > +
> > +out3:
> > + for_each_online_node(node) {
> > + if(timers[node] != NULL)
> > + kfree(timers[node]);
> > + }
> > +out2:
> > + misc_deregister(&mmtimer_miscdev);
> > +out1:
> > + free_irq(SGI_MMTIMER_VECTOR, NULL);
> > + goto out;
> > +
> > }
> >
> > module_init(mmtimer_init);
> > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > --- a/drivers/char/tpm/tpm.c
> > +++ b/drivers/char/tpm/tpm.c
> > @@ -1155,6 +1155,7 @@ #define DEVNAME_SIZE 7
> >
> > if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
> > list_del(&chip->list);
> > + misc_deregister(&chip->vendor.miscdev);
> > put_device(dev);
> > clear_bit(chip->dev_num, dev_mask);
> > kfree(chip);
> > diff --git a/drivers/input/misc/hp_sdc_rtc.c b/drivers/input/misc/hp_sdc_rtc.c
> > --- a/drivers/input/misc/hp_sdc_rtc.c
> > +++ b/drivers/input/misc/hp_sdc_rtc.c
> > @@ -693,9 +693,13 @@ static int __init hp_sdc_rtc_init(void)
> >
> > init_MUTEX(&i8042tregs);
> >
> > + INIT_LIST_HEAD(&hp_sdc_rtc_dev.list);
> > +
> > if ((ret = hp_sdc_request_timer_irq(&hp_sdc_rtc_isr)))
> > return ret;
> > - misc_register(&hp_sdc_rtc_dev);
> > + if (misc_register(&hp_sdc_rtc_dev) != 0)
> > + printk(KERN_INFO "Could not register misc. dev for sdc\n");
> > +
> > create_proc_read_entry ("driver/rtc", 0, NULL,
> > hp_sdc_rtc_read_proc, NULL);
> >
> > diff --git a/drivers/macintosh/apm_emu.c b/drivers/macintosh/apm_emu.c
> > --- a/drivers/macintosh/apm_emu.c
> > +++ b/drivers/macintosh/apm_emu.c
> > @@ -520,6 +520,8 @@ static int __init apm_emu_init(void)
> > {
> > struct proc_dir_entry *apm_proc;
> >
> > + INIT_LIST_HEAD(&apm_device.list);
> > +
> > if (sys_ctrler != SYS_CTRLER_PMU) {
> > printk(KERN_INFO "apm_emu: Requires a machine with a PMU.\n");
> > return -ENODEV;
> > @@ -529,7 +531,8 @@ static int __init apm_emu_init(void)
> > if (apm_proc)
> > apm_proc->owner = THIS_MODULE;
> >
> > - misc_register(&apm_device);
> > + if (misc_register(&apm_device) != 0)
> > + printk(KERN_INFO "Could not create misc. device for apm\n");
> >
> > pmu_register_sleep_notifier(&apm_sleep_notifier);
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -856,6 +856,8 @@ static int __init tun_init(void)
> > printk(KERN_INFO "tun: %s, %s\n", DRV_DESCRIPTION, DRV_VERSION);
> > printk(KERN_INFO "tun: %s\n", DRV_COPYRIGHT);
> >
> > + INIT_LIST_HEAD(&tun_miscdev.list);
> > +
> > ret = misc_register(&tun_miscdev);
> > if (ret)
> > printk(KERN_ERR "tun: Can't register misc device %d\n", TUN_MINOR);
> > diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> > --- a/fs/dlm/user.c
> > +++ b/fs/dlm/user.c
> > @@ -773,6 +773,7 @@ int dlm_user_init(void)
> > ctl_device.name = "dlm-control";
> > ctl_device.fops = &ctl_device_fops;
> > ctl_device.minor = MISC_DYNAMIC_MINOR;
> > + INIT_LIST_HEAD(&ctl_device.list);
> >
> > error = misc_register(&ctl_device);
> > if (error)

--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-10-24 13:24:40

by Matthew Wilcox

[permalink] [raw]

Subject: Re: [KJ] [PATCH] Correct misc_register return code handling in several drivers

On Tue, Oct 24, 2006 at 08:53:06AM -0400, Neil Horman wrote:
> The INIT_LIST_HEAD is there to prevent a potential oops on module removal.
> misc_register, if it fails, leaves miscdevice.list unchanged. That means its
> next and prev pointers contain NULL or garbage, when both pointers should contain
> &miscdevice.list. If we don't do that, then there is a chance we will oops on
> module removal when we do a list_del in misc_deregister on the moudule_exit
> routine. I could have done this statically, but I thought it looked cleaner to
> do it with the macro in the code.

Maybe it would be better to have misc_register() call INIT_LIST_HEAD in
the failure case?

2006-10-24 15:10:48

[permalink] [raw]

Subject: Re: [KJ] [PATCH] Correct misc_register return code handling in several drivers

On Tue, Oct 24, 2006 at 07:24:37AM -0600, Matthew Wilcox wrote:
> On Tue, Oct 24, 2006 at 08:53:06AM -0400, Neil Horman wrote:
> > The INIT_LIST_HEAD is there to prevent a potential oops on module removal.
> > misc_register, if it fails, leaves miscdevice.list unchanged. That means its
> > next and prev pointers contain NULL or garbage, when both pointers should contain
> > &miscdevice.list. If we don't do that, then there is a chance we will oops on
> > module removal when we do a list_del in misc_deregister on the moudule_exit
> > routine. I could have done this statically, but I thought it looked cleaner to
> > do it with the macro in the code.
>
> Maybe it would be better to have misc_register() call INIT_LIST_HEAD in
> the failure case?

Hmm, that seems reasonable. If you don't mind, since there are other unrelated
clean-ups in this patch, I'll make that change in a follow on patch. But I
think thats a good idea.

Thanks & Regards
Neil

--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-10-24 22:43:30

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Tue, 2006-10-24 at 08:53 -0400, Neil Horman wrote:
> On Tue, Oct 24, 2006 at 01:34:34PM +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2006-10-23 at 13:19 -0400, Neil Horman wrote:
> > > Hey All-
> > > Janitor patch to clean up return code handling and exit from failed
> > > calls to misc_register accross several modules.
> >
> > The patch doesn't match the description... What are those INIT_LIST_HEAD
> > things ? Is this something I've missed or is this a new requirement for
> > all misc devices ? Can't it be statically initialized instead ?
> >
>
> The INIT_LIST_HEAD is there to prevent a potential oops on module removal.
> misc_register, if it fails, leaves miscdevice.list unchanged. That means its
> next and prev pointers contain NULL or garbage, when both pointers should contain
> &miscdevice.list. If we don't do that, then there is a chance we will oops on
> module removal when we do a list_del in misc_deregister on the moudule_exit
> routine. I could have done this statically, but I thought it looked cleaner to
> do it with the macro in the code.

Hrm... I see, but I still for some reason don't like it that much.. I'd
rather have misc_register() do the initialisation unconditionally before
it can fail, don't you think ?

We would theorically have a similar problem with any driver that does

xxxx_register(&static_struct)

and

xxxx_unregister(&static_struct)

(pci, usb, etc...)

As long as there are list heads involved. I think the proper solution
here is to have either the unregister be smart and test for NULL/NULL or
the register initialize those fields before it has a chance to fail.

Ben.

2006-10-25 13:21:36

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Wed, Oct 25, 2006 at 08:42:42AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2006-10-24 at 08:53 -0400, Neil Horman wrote:
> > On Tue, Oct 24, 2006 at 01:34:34PM +1000, Benjamin Herrenschmidt wrote:
> > > On Mon, 2006-10-23 at 13:19 -0400, Neil Horman wrote:
> > > > Hey All-
> > > > Janitor patch to clean up return code handling and exit from failed
> > > > calls to misc_register accross several modules.
> > >
> > > The patch doesn't match the description... What are those INIT_LIST_HEAD
> > > things ? Is this something I've missed or is this a new requirement for
> > > all misc devices ? Can't it be statically initialized instead ?
> > >
> >
> > The INIT_LIST_HEAD is there to prevent a potential oops on module removal.
> > misc_register, if it fails, leaves miscdevice.list unchanged. That means its
> > next and prev pointers contain NULL or garbage, when both pointers should contain
> > &miscdevice.list. If we don't do that, then there is a chance we will oops on
> > module removal when we do a list_del in misc_deregister on the moudule_exit
> > routine. I could have done this statically, but I thought it looked cleaner to
> > do it with the macro in the code.
>
> Hrm... I see, but I still for some reason don't like it that much.. I'd
> rather have misc_register() do the initialisation unconditionally before
> it can fail, don't you think ?
>
> We would theorically have a similar problem with any driver that does
>
>
> xxxx_register(&static_struct)
>
> and
>
> xxxx_unregister(&static_struct)
>
> (pci, usb, etc...)
>
> As long as there are list heads involved. I think the proper solution
> here is to have either the unregister be smart and test for NULL/NULL or
> the register initialize those fields before it has a chance to fail.
>
> Ben.
>

I agreed with you in my last note regarding this, I think moving the
INIT_LIST_HEAD inside the misc_register function is a good idea, but since this
is a cleanup patch with several other fixups in it, I'd just as soon get this
integrated, and make that change in a separate patch.

Regards
Neil

--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-11-01 14:04:09

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

Hey all-
Since Andrew hasn't incorporated this patch yet, and I had the time, I
redid the patch taking Benjamin's INIT_LIST_HEAD and Joes mmtimer cleanup into
account. New patch attached, replacing the old one, everything except the
aforementioned cleanups is identical.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <[email protected]>

drivers/char/misc.c | 2 ++
drivers/char/mmtimer.c | 23 ++++++++++++++++++-----
drivers/char/tpm/tpm.c | 1 +
drivers/input/misc/hp_sdc_rtc.c | 4 +++-
drivers/macintosh/apm_emu.c | 3 ++-
5 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 62ebe09..77c20f8 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -204,6 +204,8 @@ int misc_register(struct miscdevice * mi
dev_t dev;
int err = 0;

+ INIT_LIST_HEAD(&misc->list);
+
down(&misc_sem);
list_for_each_entry(c, &misc_list, list) {
if (c->minor == misc->minor) {
diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c
index 22b9905..dbc5503 100644
--- a/drivers/char/mmtimer.c
+++ b/drivers/char/mmtimer.c
@@ -680,7 +680,7 @@ static int __init mmtimer_init(void)
if (sn_rtc_cycles_per_second < 100000) {
printk(KERN_ERR "%s: unable to determine clock frequency\n",
MMTIMER_NAME);
- return -1;
+ goto out1;
}

mmtimer_femtoperiod = ((unsigned long)1E15 + sn_rtc_cycles_per_second /
@@ -689,13 +689,13 @@ static int __init mmtimer_init(void)
if (request_irq(SGI_MMTIMER_VECTOR, mmtimer_interrupt, IRQF_PERCPU, MMTIMER_NAME, NULL)) {
printk(KERN_WARNING "%s: unable to allocate interrupt.",
MMTIMER_NAME);
- return -1;
+ goto out1;
}

if (misc_register(&mmtimer_miscdev)) {
printk(KERN_ERR "%s: failed to register device\n",
MMTIMER_NAME);
- return -1;
+ goto out2;
}

/* Get max numbered node, calculate slots needed */
@@ -709,16 +709,18 @@ static int __init mmtimer_init(void)
if (timers == NULL) {
printk(KERN_ERR "%s: failed to allocate memory for device\n",
MMTIMER_NAME);
- return -1;
+ goto out3;
}

+ memset(timers,0,(sizeof(mmtimer_t *)*maxn));
+
/* Allocate mmtimer_t's for each online node */
for_each_online_node(node) {
timers[node] = kmalloc_node(sizeof(mmtimer_t)*NUM_COMPARATORS, GFP_KERNEL, node);
if (timers[node] == NULL) {
printk(KERN_ERR "%s: failed to allocate memory for device\n",
MMTIMER_NAME);
- return -1;
+ goto out4;
}
for (i=0; i< NUM_COMPARATORS; i++) {
mmtimer_t * base = timers[node] + i;
@@ -739,6 +741,17 @@ static int __init mmtimer_init(void)
sn_rtc_cycles_per_second/(unsigned long)1E6);

return 0;
+
+out4:
+ for_each_online_node(node) {
+ kfree(timers[node]);
+ }
+out3:
+ misc_deregister(&mmtimer_miscdev);
+out2:
+ free_irq(SGI_MMTIMER_VECTOR, NULL);
+out1:
+ return -1;
}

module_init(mmtimer_init);
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 6ad2d3b..f14bf8b 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1155,6 +1155,7 @@ #define DEVNAME_SIZE 7

if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
list_del(&chip->list);
+ misc_deregister(&chip->vendor.miscdev);
put_device(dev);
clear_bit(chip->dev_num, dev_mask);
kfree(chip);
diff --git a/drivers/input/misc/hp_sdc_rtc.c b/drivers/input/misc/hp_sdc_rtc.c
index ab4da79..31d5a13 100644
--- a/drivers/input/misc/hp_sdc_rtc.c
+++ b/drivers/input/misc/hp_sdc_rtc.c
@@ -695,7 +695,9 @@ static int __init hp_sdc_rtc_init(void)

if ((ret = hp_sdc_request_timer_irq(&hp_sdc_rtc_isr)))
return ret;
- misc_register(&hp_sdc_rtc_dev);
+ if (misc_register(&hp_sdc_rtc_dev) != 0)
+ printk(KERN_INFO "Could not register misc. dev for i8042 rtc\n");
+
create_proc_read_entry ("driver/rtc", 0, NULL,
hp_sdc_rtc_read_proc, NULL);

diff --git a/drivers/macintosh/apm_emu.c b/drivers/macintosh/apm_emu.c
index 1293876..8862a83 100644
--- a/drivers/macintosh/apm_emu.c
+++ b/drivers/macintosh/apm_emu.c
@@ -529,7 +529,8 @@ static int __init apm_emu_init(void)
if (apm_proc)
apm_proc->owner = THIS_MODULE;

- misc_register(&apm_device);
+ if (misc_register(&apm_device) != 0)
+ printk(KERN_INFO "Could not create misc. device for apm\n");

pmu_register_sleep_notifier(&apm_sleep_notifier);

--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-11-01 23:56:44

by Andrew Morton

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Wed, 1 Nov 2006 08:56:19 -0500
Neil Horman <[email protected]> wrote:

> Since Andrew hasn't incorporated this patch yet, and I had the time, I
> redid the patch taking Benjamin's INIT_LIST_HEAD and Joes mmtimer cleanup into
> account. New patch attached, replacing the old one, everything except the
> aforementioned cleanups is identical.

Please prepare a description for this patch. The INIT_LIST_HEAD() in
misc_register() is mysterious.

2006-11-02 00:05:54

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On 01/11/06, Neil Horman <[email protected]> wrote:
> Hey all-
> Since Andrew hasn't incorporated this patch yet, and I had the time, I
> redid the patch taking Benjamin's INIT_LIST_HEAD and Joes mmtimer cleanup into
> account. New patch attached, replacing the old one, everything except the
> aforementioned cleanups is identical.
>
> Thanks & Regards
> Neil
>
> Signed-off-by: Neil Horman <[email protected]>
>
> +out4:
> + for_each_online_node(node) {
> + kfree(timers[node]);
> + }
> +out3:
> + misc_deregister(&mmtimer_miscdev);
> +out2:
> + free_irq(SGI_MMTIMER_VECTOR, NULL);
> +out1:
> + return -1;

Very nitpicky little thing, but shouldn't the labels start at column
1, not column 0 ?
I thought that was standard practice (apparently labels at column 0
can confuse 'patch').

Hmm, I guess that should be defined once and for all in
Documentation/CodingStyle

--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

2006-11-02 00:17:34

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Thu, 2 Nov 2006 01:05:49 +0100 Jesper Juhl wrote:

> On 01/11/06, Neil Horman <[email protected]> wrote:
> > Hey all-
> > Since Andrew hasn't incorporated this patch yet, and I had the time, I
> > redid the patch taking Benjamin's INIT_LIST_HEAD and Joes mmtimer cleanup into
> > account. New patch attached, replacing the old one, everything except the
> > aforementioned cleanups is identical.
> >
> > Thanks & Regards
> > Neil
> >
> > Signed-off-by: Neil Horman <[email protected]>
> >
> > +out4:
> > + for_each_online_node(node) {
> > + kfree(timers[node]);
> > + }
> > +out3:
> > + misc_deregister(&mmtimer_miscdev);
> > +out2:
> > + free_irq(SGI_MMTIMER_VECTOR, NULL);
> > +out1:
> > + return -1;
>
> Very nitpicky little thing, but shouldn't the labels start at column
> 1, not column 0 ?
> I thought that was standard practice (apparently labels at column 0
> can confuse 'patch').
>
> Hmm, I guess that should be defined once and for all in
> Documentation/CodingStyle

I have some other CodingStyle changes to submit, but feel free
to write this one up.

However, I didn't know that we had a known style for this, other
than "not indented so far that it's hidden".

If a label in column 0 [0-based :] confuses patch, then that's
a reason, I suppose. I wasn't aware of that one...
In a case like that, we usually say "fix the tool then."

---
~Randy

2006-11-02 00:24:13

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On 02/11/06, Randy Dunlap <[email protected]> wrote:
> On Thu, 2 Nov 2006 01:05:49 +0100 Jesper Juhl wrote:
>
> > On 01/11/06, Neil Horman <[email protected]> wrote:
> > > Hey all-
> > > Since Andrew hasn't incorporated this patch yet, and I had the time, I
> > > redid the patch taking Benjamin's INIT_LIST_HEAD and Joes mmtimer cleanup into
> > > account. New patch attached, replacing the old one, everything except the
> > > aforementioned cleanups is identical.
> > >
> > > Thanks & Regards
> > > Neil
> > >
> > > Signed-off-by: Neil Horman <[email protected]>
> > >
> > > +out4:
> > > + for_each_online_node(node) {
> > > + kfree(timers[node]);
> > > + }
> > > +out3:
> > > + misc_deregister(&mmtimer_miscdev);
> > > +out2:
> > > + free_irq(SGI_MMTIMER_VECTOR, NULL);
> > > +out1:
> > > + return -1;
> >
> > Very nitpicky little thing, but shouldn't the labels start at column
> > 1, not column 0 ?
> > I thought that was standard practice (apparently labels at column 0
> > can confuse 'patch').
> >
> > Hmm, I guess that should be defined once and for all in
> > Documentation/CodingStyle
>
> I have some other CodingStyle changes to submit, but feel free
> to write this one up.
>
> However, I didn't know that we had a known style for this, other
> than "not indented so far that it's hidden".
>
> If a label in column 0 [0-based :] confuses patch, then that's
> a reason, I suppose. I wasn't aware of that one...
> In a case like that, we usually say "fix the tool then."
>

Well, I am not entirely sure what confusion it is supposed to cause,
but comments such as this one suggests that there are some known
cases: http://lkml.org/lkml/2005/9/16/213
And in addition, it something that I recall having seen lots of
comments on on LKML, so I just assumed that there is some truth to it.
That is again backed by the circumstantial evidence that many files
actually do place labels at column 1 currently.

(lots of hand waving going on here, I know)

--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

2006-11-02 00:28:38

by Andrew Morton

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Wed, 1 Nov 2006 16:11:55 -0800
Randy Dunlap <[email protected]> wrote:

> > Hmm, I guess that should be defined once and for all in
> > Documentation/CodingStyle
>
> I have some other CodingStyle changes to submit, but feel free
> to write this one up.

Starting labels in column 2 gives me the creeps, personally. But there's a
decent justification for it.

> However, I didn't know that we had a known style for this, other
> than "not indented so far that it's hidden".
>
> If a label in column 0 [0-based :] confuses patch, then that's
> a reason, I suppose. I wasn't aware of that one...
> In a case like that, we usually say "fix the tool then."

The problem is that `diff -p' screws up and displays the label: in the
place where it should be displaying the function name.

Of course, lots of people forget the -p anyway... Maybe we can fix those
tools ;)

2006-11-02 00:43:12

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Thursday 02 November 2006 01:27, Andrew Morton wrote:
> On Wed, 1 Nov 2006 16:11:55 -0800
> Randy Dunlap <[email protected]> wrote:
>
> > > Hmm, I guess that should be defined once and for all in
> > > Documentation/CodingStyle
> >
> > I have some other CodingStyle changes to submit, but feel free
> > to write this one up.
>
> Starting labels in column 2 gives me the creeps, personally. But there's a
> decent justification for it.
>
> > However, I didn't know that we had a known style for this, other
> > than "not indented so far that it's hidden".
> >
> > If a label in column 0 [0-based :] confuses patch, then that's
> > a reason, I suppose. I wasn't aware of that one...
> > In a case like that, we usually say "fix the tool then."
>
> The problem is that `diff -p' screws up and displays the label: in the
> place where it should be displaying the function name.
>
> Of course, lots of people forget the -p anyway... Maybe we can fix those
> tools ;)
>
Until the tools get fixed, how about applying this patch ?

Add CodngStyle info on labels.

Signed-off-by: Jesper Juhl <[email protected]>
---

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 29c1896..f8a3abb 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -566,6 +566,18 @@ result. Typical examples would be funct
NULL or the ERR_PTR mechanism to report failure.

+ Chapter 17: Labels
+
+Label names should be lowercase.
+
+Label names should start with a letter [a-z].
+
+Labels should not be placed at column 0. Doing so confuses some tools, most
+notably 'diff' and 'patch'. Instead place labels at column 1 (indented 1
+space). In some cases it's OK to indent labels one or more tabs, but
+generally it is prefered that labels be placed at column 1.
+
+

Appendix I: References

2006-11-02 01:17:54

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

Jesper Juhl wrote:
> On Thursday 02 November 2006 01:27, Andrew Morton wrote:
>> On Wed, 1 Nov 2006 16:11:55 -0800
>> Randy Dunlap <[email protected]> wrote:
>>
>>>> Hmm, I guess that should be defined once and for all in
>>>> Documentation/CodingStyle
>>> I have some other CodingStyle changes to submit, but feel free
>>> to write this one up.
>> Starting labels in column 2 gives me the creeps, personally. But there's a
>> decent justification for it.
>>
>>> However, I didn't know that we had a known style for this, other
>>> than "not indented so far that it's hidden".
>>>
>>> If a label in column 0 [0-based :] confuses patch, then that's
>>> a reason, I suppose. I wasn't aware of that one...
>>> In a case like that, we usually say "fix the tool then."
>> The problem is that `diff -p' screws up and displays the label: in the
>> place where it should be displaying the function name.
>>
>> Of course, lots of people forget the -p anyway... Maybe we can fix those
>> tools ;)
>>
> Until the tools get fixed, how about applying this patch ?
>
>
> Add CodngStyle info on labels.
>
> Signed-off-by: Jesper Juhl <[email protected]>
> ---
>
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 29c1896..f8a3abb 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -566,6 +566,18 @@ result. Typical examples would be funct
> NULL or the ERR_PTR mechanism to report failure.
>
>
> + Chapter 17: Labels
> +
> +Label names should be lowercase.
> +
> +Label names should start with a letter [a-z].
> +
> +Labels should not be placed at column 0. Doing so confuses some tools, most
> +notably 'diff' and 'patch'. Instead place labels at column 1 (indented 1
> +space). In some cases it's OK to indent labels one or more tabs, but
> +generally it is prefered that labels be placed at column 1.
~~~~~~~~~~~~
preferred

I would also say something like this:

Labels should stand out -- be easily visible. They should not be
indented so much that they are hidden or obscured by the surrounding
source code.

--
~Randy

2006-11-02 01:22:04

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Thursday 02 November 2006 02:12, Randy Dunlap wrote:
> Jesper Juhl wrote:
> > On Thursday 02 November 2006 01:27, Andrew Morton wrote:
> >> On Wed, 1 Nov 2006 16:11:55 -0800
> >> Randy Dunlap <[email protected]> wrote:
> >>
> >>>> Hmm, I guess that should be defined once and for all in
> >>>> Documentation/CodingStyle
> >>> I have some other CodingStyle changes to submit, but feel free
> >>> to write this one up.
> >> Starting labels in column 2 gives me the creeps, personally. But there's a
> >> decent justification for it.
> >>
> >>> However, I didn't know that we had a known style for this, other
> >>> than "not indented so far that it's hidden".
> >>>
> >>> If a label in column 0 [0-based :] confuses patch, then that's
> >>> a reason, I suppose. I wasn't aware of that one...
> >>> In a case like that, we usually say "fix the tool then."
> >> The problem is that `diff -p' screws up and displays the label: in the
> >> place where it should be displaying the function name.
> >>
> >> Of course, lots of people forget the -p anyway... Maybe we can fix those
> >> tools ;)
> >>
> > Until the tools get fixed, how about applying this patch ?
> >
> >
> > Add CodngStyle info on labels.
> >
[snip]
> > +generally it is prefered that labels be placed at column 1.
> ~~~~~~~~~~~~
> preferred
>
> I would also say something like this:
>
> Labels should stand out -- be easily visible. They should not be
> indented so much that they are hidden or obscured by the surrounding
> source code.
>
Ok, how's this :

Signed-off-by: Jesper Juhl <[email protected]>
---

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 29c1896..4f6b2d5 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -566,6 +566,21 @@ result. Typical examples would be funct
NULL or the ERR_PTR mechanism to report failure.

+ Chapter 17: Labels
+
+Label names should be lowercase.
+
+Label names should start with a letter [a-z].
+
+Labels should not be placed at column 0. Doing so confuses some tools, most
+notably 'diff' and 'patch'. Instead place labels at column 1 (indented 1
+space). In some cases it's OK to indent labels one or more tabs, but
+generally it is preferred that labels be placed at column 1.
+
+Labels should stand out - be easily visible. They should not be indented so
+much that they are hidden or obscured by the surrounding source code.
+
+

Appendix I: References

2006-11-02 01:28:03

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Thu, 2 Nov 2006 02:23:42 +0100 Jesper Juhl wrote:

> On Thursday 02 November 2006 02:12, Randy Dunlap wrote:
> > Jesper Juhl wrote:
> > > On Thursday 02 November 2006 01:27, Andrew Morton wrote:
> > >> On Wed, 1 Nov 2006 16:11:55 -0800
> > >> Randy Dunlap <[email protected]> wrote:
> > >>
> > >>>> Hmm, I guess that should be defined once and for all in
> > >>>> Documentation/CodingStyle
> > >>> I have some other CodingStyle changes to submit, but feel free
> > >>> to write this one up.
> > >> Starting labels in column 2 gives me the creeps, personally. But there's a
> > >> decent justification for it.
> > >>
> > >>> However, I didn't know that we had a known style for this, other
> > >>> than "not indented so far that it's hidden".
> > >>>
> > >>> If a label in column 0 [0-based :] confuses patch, then that's
> > >>> a reason, I suppose. I wasn't aware of that one...
> > >>> In a case like that, we usually say "fix the tool then."
> > >> The problem is that `diff -p' screws up and displays the label: in the
> > >> place where it should be displaying the function name.
> > >>
> > >> Of course, lots of people forget the -p anyway... Maybe we can fix those
> > >> tools ;)
> > >>
> > > Until the tools get fixed, how about applying this patch ?
> > >
> > >
> > > Add CodngStyle info on labels.
> > >
> [snip]
> > > +generally it is prefered that labels be placed at column 1.
> > ~~~~~~~~~~~~
> > preferred
> >
> > I would also say something like this:
> >
> > Labels should stand out -- be easily visible. They should not be
> > indented so much that they are hidden or obscured by the surrounding
> > source code.
> >
> Ok, how's this :
>
> Signed-off-by: Jesper Juhl <[email protected]>
> ---
>
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 29c1896..4f6b2d5 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -566,6 +566,21 @@ result. Typical examples would be funct
> NULL or the ERR_PTR mechanism to report failure.
>
>
> + Chapter 17: Labels
> +
> +Label names should be lowercase.
> +
> +Label names should start with a letter [a-z].
> +
> +Labels should not be placed at column 0. Doing so confuses some tools, most
> +notably 'diff' and 'patch'. Instead place labels at column 1 (indented 1
> +space). In some cases it's OK to indent labels one or more tabs, but
> +generally it is preferred that labels be placed at column 1.
> +
> +Labels should stand out - be easily visible. They should not be indented so
> +much that they are hidden or obscured by the surrounding source code.
> +
> +
>
> Appendix I: References

Yep, OK with me. (Ack)

---
~Randy

2006-11-02 14:22:35

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Wed, Nov 01, 2006 at 03:55:41PM -0800, Andrew Morton wrote:
> On Wed, 1 Nov 2006 08:56:19 -0500
> Neil Horman <[email protected]> wrote:
>
> > Since Andrew hasn't incorporated this patch yet, and I had the time, I
> > redid the patch taking Benjamin's INIT_LIST_HEAD and Joes mmtimer cleanup into
> > account. New patch attached, replacing the old one, everything except the
> > aforementioned cleanups is identical.
>
> Please prepare a description for this patch. The INIT_LIST_HEAD() in
> misc_register() is mysterious.

Andrew-
This is a patch to clean up several code points in which the return code
from misc_register is not handled properly. Several modules failed to
deregister various hooks when misc_register fails, and this patch cleans them
up. Also there are a few modules that legitimately don't care about the failure
status of misc register. These drivers however unilaterally call
misc_deregister on module unload. Since misc_register doesn't initialize the
list_head in the init_routine if it fails, the deregister operation is at risk
for oopsing when list_del is called. The initial solution was to manually init
the list in the miscdev structure in each of those modules, but the consensus in
this thread was to consolodate and do that universally inside misc_register.

Thanks & Regards
Neil

--
/***************************************************
*Neil Horman
*Software Engineer
*gpg keyid: 1024D / 0x92A74FA1 - http://pgp.mit.edu
***************************************************/

2006-11-15 00:05:25

[permalink] [raw]

Subject: Re: [KJ][PATCH] Correct misc_register return code handling in several drivers

On Wed, 1 Nov 2006 17:22:41 -0800 Randy Dunlap wrote:

> On Thu, 2 Nov 2006 02:23:42 +0100 Jesper Juhl wrote:
>
> > On Thursday 02 November 2006 02:12, Randy Dunlap wrote:
> > > Jesper Juhl wrote:
> > > > On Thursday 02 November 2006 01:27, Andrew Morton wrote:
> > > >> On Wed, 1 Nov 2006 16:11:55 -0800
> > > >> Randy Dunlap <[email protected]> wrote:
> > > >>
> > > >>>> Hmm, I guess that should be defined once and for all in
> > > >>>> Documentation/CodingStyle
> > > >>> I have some other CodingStyle changes to submit, but feel free
> > > >>> to write this one up.
> > > >> Starting labels in column 2 gives me the creeps, personally. But there's a
> > > >> decent justification for it.
> > > >>
> > > >>> However, I didn't know that we had a known style for this, other
> > > >>> than "not indented so far that it's hidden".
> > > >>>
> > > >>> If a label in column 0 [0-based :] confuses patch, then that's
> > > >>> a reason, I suppose. I wasn't aware of that one...
> > > >>> In a case like that, we usually say "fix the tool then."
> > > >> The problem is that `diff -p' screws up and displays the label: in the
> > > >> place where it should be displaying the function name.
> > > >>
> > > >> Of course, lots of people forget the -p anyway... Maybe we can fix those
> > > >> tools ;)
> > > >>
> > > > Until the tools get fixed, how about applying this patch ?
> > > >
> > > >
> > > > Add CodngStyle info on labels.
> > > >
> > [snip]
> > > > +generally it is prefered that labels be placed at column 1.
> > > ~~~~~~~~~~~~
> > > preferred
> > >
> > > I would also say something like this:
> > >
> > > Labels should stand out -- be easily visible. They should not be
> > > indented so much that they are hidden or obscured by the surrounding
> > > source code.
> > >
> > Ok, how's this :
> >
> > Signed-off-by: Jesper Juhl <[email protected]>
> > ---
> >
> > diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> > index 29c1896..4f6b2d5 100644
> > --- a/Documentation/CodingStyle
> > +++ b/Documentation/CodingStyle
> > @@ -566,6 +566,21 @@ result. Typical examples would be funct
> > NULL or the ERR_PTR mechanism to report failure.
> >
> >
> > + Chapter 17: Labels
> > +
> > +Label names should be lowercase.
> > +
> > +Label names should start with a letter [a-z].
> > +
> > +Labels should not be placed at column 0. Doing so confuses some tools, most
> > +notably 'diff' and 'patch'. Instead place labels at column 1 (indented 1
> > +space). In some cases it's OK to indent labels one or more tabs, but
> > +generally it is preferred that labels be placed at column 1.
> > +
> > +Labels should stand out - be easily visible. They should not be indented so
> > +much that they are hidden or obscured by the surrounding source code.
> > +
> > +
> >
> > Appendix I: References
>
> Yep, OK with me. (Ack)
> ---

Did Andrew ever pick up this doc. patch?

Anyway, I wanted to see the problem with 'diff' and labels in column 0
causing 'diff -p' @@ tags to be confused, but when I tested it,
it Works For Me. No difference in diff @@ tags if I indent the
labels or not. or is this too simple?
---

not-sob: just a patch to test diff -p and labels in column 0:

---
lib/vsprintf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

--- linux-2.6.19-rc3-git6.orig/lib/vsprintf.c
+++ linux-2.6.19-rc3-git6/lib/vsprintf.c
@@ -279,8 +279,9 @@ int vsnprintf(char *buf, size_t size, co
static int warn = 1;
WARN_ON(warn);
warn = 0;
- return 0;
+ goto ret0;
}
+notret0:

str = buf;
end = buf + size;
@@ -493,6 +494,8 @@ int vsnprintf(char *buf, size_t size, co
}
/* the trailing null byte doesn't count towards the total */
return str-buf;
+ret0:
+ return 0;
}

EXPORT_SYMBOL(vsnprintf);

2006-11-15 09:05:38