[PATCH 1/2] input: make serio_register_driver() return error code

[PATCH 1/2] input: make serio_register_driver() return error code

[Akinobu Mita]

serio_register_driver() may fail under memory shortage.

When serio_register_driver() called, it queues SERIO_REGISTER_DRIVER
event into global serio_event_list, and then kseriod kernel thread
handles that event and do driver_register().

But event allocation by serio_register_driver() may fail.
Because it is GFP_ATOMIC allocation. It will cause the problem
by serio_unregister_driver() with not being registered driver
at module_exit() time 

This patch makes serio_register_driver() call driver_register()
directly instead of kseriod so that it can check whether
driver_register() is succeeded or not.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 drivers/input/serio/serio.c |   18 ++++--------------
 include/linux/serio.h       |    7 +------
 2 files changed, 5 insertions(+), 20 deletions(-)

Index: work-fault-inject/drivers/input/serio/serio.c
===================================================================
--- work-fault-inject.orig/drivers/input/serio/serio.c
+++ work-fault-inject/drivers/input/serio/serio.c
@@ -46,7 +46,7 @@ EXPORT_SYMBOL(__serio_register_port);
 EXPORT_SYMBOL(serio_unregister_port);
 EXPORT_SYMBOL(serio_unregister_child_port);
 EXPORT_SYMBOL(__serio_unregister_port_delayed);
-EXPORT_SYMBOL(__serio_register_driver);
+EXPORT_SYMBOL(serio_register_driver);
 EXPORT_SYMBOL(serio_unregister_driver);
 EXPORT_SYMBOL(serio_open);
 EXPORT_SYMBOL(serio_close);
@@ -175,7 +175,6 @@ enum serio_event_type {
 	SERIO_RECONNECT,
 	SERIO_REGISTER_PORT,
 	SERIO_UNREGISTER_PORT,
-	SERIO_REGISTER_DRIVER,
 };
 
 struct serio_event {
@@ -322,10 +321,6 @@ static void serio_handle_event(void)
 				serio_find_driver(event->object);
 				break;
 
-			case SERIO_REGISTER_DRIVER:
-				serio_add_driver(event->object);
-				break;
-
 			default:
 				break;
 		}
@@ -791,22 +786,17 @@ static struct bus_type serio_bus = {
 	.remove = serio_driver_remove,
 };
 
-static void serio_add_driver(struct serio_driver *drv)
+int serio_register_driver(struct serio_driver *drv)
 {
 	int error;
 
+	drv->driver.bus = &serio_bus;
 	error = driver_register(&drv->driver);
 	if (error)
 		printk(KERN_ERR
 			"serio: driver_register() failed for %s, error: %d\n",
 			drv->driver.name, error);
-}
-
-void __serio_register_driver(struct serio_driver *drv, struct module *owner)
-{
-	drv->driver.bus = &serio_bus;
-
-	serio_queue_event(drv, owner, SERIO_REGISTER_DRIVER);
+	return error;
 }
 
 void serio_unregister_driver(struct serio_driver *drv)
Index: work-fault-inject/include/linux/serio.h
===================================================================
--- work-fault-inject.orig/include/linux/serio.h
+++ work-fault-inject/include/linux/serio.h
@@ -91,12 +91,7 @@ static inline void serio_unregister_port
 	__serio_unregister_port_delayed(serio, THIS_MODULE);
 }
 
-void __serio_register_driver(struct serio_driver *drv, struct module *owner);
-static inline void serio_register_driver(struct serio_driver *drv)
-{
-	__serio_register_driver(drv, THIS_MODULE);
-}
-
+int serio_register_driver(struct serio_driver *drv);
 void serio_unregister_driver(struct serio_driver *drv);
 
 static inline int serio_write(struct serio *serio, unsigned char data)

Re: [PATCH 1/2] input: make serio_register_driver() return error code

[Dmitry Torokhov]

On 11/7/06, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> serio_register_driver() may fail under memory shortage.
>
> When serio_register_driver() called, it queues SERIO_REGISTER_DRIVER
> event into global serio_event_list, and then kseriod kernel thread
> handles that event and do driver_register().
>
> But event allocation by serio_register_driver() may fail.
> Because it is GFP_ATOMIC allocation. It will cause the problem
> by serio_unregister_driver() with not being registered driver
> at module_exit() time
>
> This patch makes serio_register_driver() call driver_register()
> directly instead of kseriod so that it can check whether
> driver_register() is succeeded or not.
>

This slows down boot process because probing for mice and keyboards
takes too long (for some touchpads it takes about 4 seconds to do
reset). We could change allocation from GFP_ATOMIC to GFP_KERNEL for
SERIO_REGISTER_DRIVER events to make it more robust but otherwise I'd
leave serio_register_driver return void. You could also add a flag to
serio driver indicating whether registration is complete and check
that flag in serio_unregister_driver so it does not do stupid things.

-- 
Dmitry

Re: [PATCH 1/2] input: make serio_register_driver() return error code

[Akinobu Mita]

On Tue, Nov 07, 2006 at 09:20:07AM -0500, Dmitry Torokhov wrote:
> >This patch makes serio_register_driver() call driver_register()
> >directly instead of kseriod so that it can check whether
> >driver_register() is succeeded or not.
> >
> 
> This slows down boot process because probing for mice and keyboards
> takes too long (for some touchpads it takes about 4 seconds to do

I understand the reason why driver_register() is done by kseriod.

> reset). We could change allocation from GFP_ATOMIC to GFP_KERNEL for
> SERIO_REGISTER_DRIVER events to make it more robust but otherwise I'd
> leave serio_register_driver return void. You could also add a flag to
> serio driver indicating whether registration is complete and check
> that flag in serio_unregister_driver so it does not do stupid things.

I reorganzed the patch set.

serio driver registration can fail in two different ways.

1) serio_event allocation failure by serio_register_driver().

   It happens in module_init() context. It is possible to check this
   allocation failure by making serio_register_driver() return error.

2) driver_register() failure by kseriod.

   This failure cannot be checked by serio_register_driver().
   But it is necessary to prevent serio_unregister_driver() from
   trying to call driver_unregister() with not registered driver
   by adding flag to serio driver indicating whether registration is
   complete.

1/4: make serio_register_driver() return error -- 1)
2/4: check serio_register_driver() error -- 1)
3/4: check whether serio dirver registration is completed -- 2)
4/4: change to GFP_KERNEL for SERIO_REGISTER_DRIVER event allocation

[PATCH 1/4] input: make serio_register_driver() return error

[Akinobu Mita]

This patch makes serio_register_driver() return error
when serio_event allocation is failed or unable to get module reference.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 drivers/input/serio/serio.c |   12 ++++++++----
 include/linux/serio.h       |    6 +++---
 2 files changed, 11 insertions(+), 7 deletions(-)

Index: work-fault-inject/drivers/input/serio/serio.c
===================================================================
--- work-fault-inject.orig/drivers/input/serio/serio.c
+++ work-fault-inject/drivers/input/serio/serio.c
@@ -190,11 +190,12 @@ static LIST_HEAD(serio_event_list);
 static DECLARE_WAIT_QUEUE_HEAD(serio_wait);
 static struct task_struct *serio_task;
 
-static void serio_queue_event(void *object, struct module *owner,
-			      enum serio_event_type event_type)
+static int serio_queue_event(void *object, struct module *owner,
+			     enum serio_event_type event_type)
 {
 	unsigned long flags;
 	struct serio_event *event;
+	int err = 0;
 
 	spin_lock_irqsave(&serio_event_lock, flags);
 
@@ -215,6 +216,7 @@ static void serio_queue_event(void *obje
 
 	if ((event = kmalloc(sizeof(struct serio_event), GFP_ATOMIC))) {
 		if (!try_module_get(owner)) {
+			err = -EINVAL;
 			printk(KERN_WARNING "serio: Can't get module reference, dropping event %d\n", event_type);
 			kfree(event);
 			goto out;
@@ -227,10 +229,12 @@ static void serio_queue_event(void *obje
 		list_add_tail(&event->node, &serio_event_list);
 		wake_up(&serio_wait);
 	} else {
+		err = -ENOMEM;
 		printk(KERN_ERR "serio: Not enough memory to queue event %d\n", event_type);
 	}
 out:
 	spin_unlock_irqrestore(&serio_event_lock, flags);
+	return err;
 }
 
 static void serio_free_event(struct serio_event *event)
@@ -802,11 +806,11 @@ static void serio_add_driver(struct seri
 			drv->driver.name, error);
 }
 
-void __serio_register_driver(struct serio_driver *drv, struct module *owner)
+int __serio_register_driver(struct serio_driver *drv, struct module *owner)
 {
 	drv->driver.bus = &serio_bus;
 
-	serio_queue_event(drv, owner, SERIO_REGISTER_DRIVER);
+	return serio_queue_event(drv, owner, SERIO_REGISTER_DRIVER);
 }
 
 void serio_unregister_driver(struct serio_driver *drv)
Index: work-fault-inject/include/linux/serio.h
===================================================================
--- work-fault-inject.orig/include/linux/serio.h
+++ work-fault-inject/include/linux/serio.h
@@ -91,10 +91,10 @@ static inline void serio_unregister_port
 	__serio_unregister_port_delayed(serio, THIS_MODULE);
 }
 
-void __serio_register_driver(struct serio_driver *drv, struct module *owner);
-static inline void serio_register_driver(struct serio_driver *drv)
+int __serio_register_driver(struct serio_driver *drv, struct module *owner);
+static inline int serio_register_driver(struct serio_driver *drv)
 {
-	__serio_register_driver(drv, THIS_MODULE);
+	return __serio_register_driver(drv, THIS_MODULE);
 }
 
 void serio_unregister_driver(struct serio_driver *drv);

[PATCH 2/4] input: check serio_register_driver() error

[Akinobu Mita]

Now serio_register_driver() returns error.
This patch checks serio_register_driver() error

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 drivers/input/joystick/iforce/iforce-main.c |   14 +++++++++++---
 drivers/input/joystick/magellan.c           |    3 +--
 drivers/input/joystick/spaceball.c          |    3 +--
 drivers/input/joystick/spaceorb.c           |    3 +--
 drivers/input/joystick/stinger.c            |    3 +--
 drivers/input/joystick/twidjoy.c            |    3 +--
 drivers/input/joystick/warrior.c            |    3 +--
 drivers/input/keyboard/atkbd.c              |    3 +--
 drivers/input/keyboard/hil_kbd.c            |    3 +--
 drivers/input/keyboard/lkkbd.c              |    3 +--
 drivers/input/keyboard/newtonkbd.c          |    3 +--
 drivers/input/keyboard/stowaway.c           |    3 +--
 drivers/input/keyboard/sunkbd.c             |    3 +--
 drivers/input/keyboard/xtkbd.c              |    3 +--
 drivers/input/mouse/hil_ptr.c               |    3 +--
 drivers/input/mouse/psmouse-base.c          |    8 ++++++--
 drivers/input/mouse/sermouse.c              |    3 +--
 drivers/input/mouse/vsxxxaa.c               |    3 +--
 drivers/input/touchscreen/elo.c             |    3 +--
 drivers/input/touchscreen/gunze.c           |    3 +--
 drivers/input/touchscreen/h3600_ts_input.c  |    3 +--
 drivers/input/touchscreen/mtouch.c          |    3 +--
 drivers/input/touchscreen/penmount.c        |    3 +--
 drivers/input/touchscreen/touchright.c      |    3 +--
 drivers/input/touchscreen/touchwin.c        |    3 +--
 25 files changed, 40 insertions(+), 51 deletions(-)

Index: work-fault-inject/drivers/input/mouse/vsxxxaa.c
===================================================================
--- work-fault-inject.orig/drivers/input/mouse/vsxxxaa.c
+++ work-fault-inject/drivers/input/mouse/vsxxxaa.c
@@ -571,8 +571,7 @@ static struct serio_driver vsxxxaa_drv =
 static int __init
 vsxxxaa_init (void)
 {
-	serio_register_driver(&vsxxxaa_drv);
-	return 0;
+	return serio_register_driver(&vsxxxaa_drv);
 }
 
 static void __exit
Index: work-fault-inject/drivers/input/mouse/psmouse-base.c
===================================================================
--- work-fault-inject.orig/drivers/input/mouse/psmouse-base.c
+++ work-fault-inject/drivers/input/mouse/psmouse-base.c
@@ -1509,15 +1509,19 @@ static int psmouse_get_maxproto(char *bu
 
 static int __init psmouse_init(void)
 {
+	int err;
+
 	kpsmoused_wq = create_singlethread_workqueue("kpsmoused");
 	if (!kpsmoused_wq) {
 		printk(KERN_ERR "psmouse: failed to create kpsmoused workqueue\n");
 		return -ENOMEM;
 	}
 
-	serio_register_driver(&psmouse_drv);
+	err = serio_register_driver(&psmouse_drv);
+	if (err)
+		destroy_workqueue(kpsmoused_wq);
 
-	return 0;
+	return err;
 }
 
 static void __exit psmouse_exit(void)
Index: work-fault-inject/drivers/input/touchscreen/elo.c
===================================================================
--- work-fault-inject.orig/drivers/input/touchscreen/elo.c
+++ work-fault-inject/drivers/input/touchscreen/elo.c
@@ -397,8 +397,7 @@ static struct serio_driver elo_drv = {
 
 static int __init elo_init(void)
 {
-	serio_register_driver(&elo_drv);
-	return 0;
+	return serio_register_driver(&elo_drv);
 }
 
 static void __exit elo_exit(void)
Index: work-fault-inject/drivers/input/touchscreen/gunze.c
===================================================================
--- work-fault-inject.orig/drivers/input/touchscreen/gunze.c
+++ work-fault-inject/drivers/input/touchscreen/gunze.c
@@ -190,8 +190,7 @@ static struct serio_driver gunze_drv = {
 
 static int __init gunze_init(void)
 {
-	serio_register_driver(&gunze_drv);
-	return 0;
+	return serio_register_driver(&gunze_drv);
 }
 
 static void __exit gunze_exit(void)
Index: work-fault-inject/drivers/input/touchscreen/h3600_ts_input.c
===================================================================
--- work-fault-inject.orig/drivers/input/touchscreen/h3600_ts_input.c
+++ work-fault-inject/drivers/input/touchscreen/h3600_ts_input.c
@@ -478,8 +478,7 @@ static struct serio_driver h3600ts_drv =
 
 static int __init h3600ts_init(void)
 {
-	serio_register_driver(&h3600ts_drv);
-	return 0;
+	return serio_register_driver(&h3600ts_drv);
 }
 
 static void __exit h3600ts_exit(void)
Index: work-fault-inject/drivers/input/touchscreen/mtouch.c
===================================================================
--- work-fault-inject.orig/drivers/input/touchscreen/mtouch.c
+++ work-fault-inject/drivers/input/touchscreen/mtouch.c
@@ -205,8 +205,7 @@ static struct serio_driver mtouch_drv = 
 
 static int __init mtouch_init(void)
 {
-	serio_register_driver(&mtouch_drv);
-	return 0;
+	return serio_register_driver(&mtouch_drv);
 }
 
 static void __exit mtouch_exit(void)
Index: work-fault-inject/drivers/input/touchscreen/penmount.c
===================================================================
--- work-fault-inject.orig/drivers/input/touchscreen/penmount.c
+++ work-fault-inject/drivers/input/touchscreen/penmount.c
@@ -171,8 +171,7 @@ static struct serio_driver pm_drv = {
 
 static int __init pm_init(void)
 {
-	serio_register_driver(&pm_drv);
-	return 0;
+	return serio_register_driver(&pm_drv);
 }
 
 static void __exit pm_exit(void)
Index: work-fault-inject/drivers/input/touchscreen/touchright.c
===================================================================
--- work-fault-inject.orig/drivers/input/touchscreen/touchright.c
+++ work-fault-inject/drivers/input/touchscreen/touchright.c
@@ -182,8 +182,7 @@ static struct serio_driver tr_drv = {
 
 static int __init tr_init(void)
 {
-	serio_register_driver(&tr_drv);
-	return 0;
+	return serio_register_driver(&tr_drv);
 }
 
 static void __exit tr_exit(void)
Index: work-fault-inject/drivers/input/touchscreen/touchwin.c
===================================================================
--- work-fault-inject.orig/drivers/input/touchscreen/touchwin.c
+++ work-fault-inject/drivers/input/touchscreen/touchwin.c
@@ -189,8 +189,7 @@ static struct serio_driver tw_drv = {
 
 static int __init tw_init(void)
 {
-	serio_register_driver(&tw_drv);
-	return 0;
+	return serio_register_driver(&tw_drv);
 }
 
 static void __exit tw_exit(void)
Index: work-fault-inject/drivers/input/keyboard/atkbd.c
===================================================================
--- work-fault-inject.orig/drivers/input/keyboard/atkbd.c
+++ work-fault-inject/drivers/input/keyboard/atkbd.c
@@ -1293,8 +1293,7 @@ static ssize_t atkbd_show_err_count(stru
 
 static int __init atkbd_init(void)
 {
-	serio_register_driver(&atkbd_drv);
-	return 0;
+	return serio_register_driver(&atkbd_drv);
 }
 
 static void __exit atkbd_exit(void)
Index: work-fault-inject/drivers/input/keyboard/hil_kbd.c
===================================================================
--- work-fault-inject.orig/drivers/input/keyboard/hil_kbd.c
+++ work-fault-inject/drivers/input/keyboard/hil_kbd.c
@@ -381,8 +381,7 @@ struct serio_driver hil_kbd_serio_drv = 
 
 static int __init hil_kbd_init(void)
 {
-	serio_register_driver(&hil_kbd_serio_drv);
-        return 0;
+	return serio_register_driver(&hil_kbd_serio_drv);
 }
                 
 static void __exit hil_kbd_exit(void)
Index: work-fault-inject/drivers/input/keyboard/lkkbd.c
===================================================================
--- work-fault-inject.orig/drivers/input/keyboard/lkkbd.c
+++ work-fault-inject/drivers/input/keyboard/lkkbd.c
@@ -754,8 +754,7 @@ static struct serio_driver lkkbd_drv = {
 static int __init
 lkkbd_init (void)
 {
-	serio_register_driver(&lkkbd_drv);
-	return 0;
+	return serio_register_driver(&lkkbd_drv);
 }
 
 static void __exit
Index: work-fault-inject/drivers/input/keyboard/newtonkbd.c
===================================================================
--- work-fault-inject.orig/drivers/input/keyboard/newtonkbd.c
+++ work-fault-inject/drivers/input/keyboard/newtonkbd.c
@@ -165,8 +165,7 @@ static struct serio_driver nkbd_drv = {
 
 static int __init nkbd_init(void)
 {
-	serio_register_driver(&nkbd_drv);
-	return 0;
+	return serio_register_driver(&nkbd_drv);
 }
 
 static void __exit nkbd_exit(void)
Index: work-fault-inject/drivers/input/keyboard/stowaway.c
===================================================================
--- work-fault-inject.orig/drivers/input/keyboard/stowaway.c
+++ work-fault-inject/drivers/input/keyboard/stowaway.c
@@ -173,8 +173,7 @@ static struct serio_driver skbd_drv = {
 
 static int __init skbd_init(void)
 {
-	serio_register_driver(&skbd_drv);
-	return 0;
+	return serio_register_driver(&skbd_drv);
 }
 
 static void __exit skbd_exit(void)
Index: work-fault-inject/drivers/input/keyboard/sunkbd.c
===================================================================
--- work-fault-inject.orig/drivers/input/keyboard/sunkbd.c
+++ work-fault-inject/drivers/input/keyboard/sunkbd.c
@@ -346,8 +346,7 @@ static struct serio_driver sunkbd_drv = 
 
 static int __init sunkbd_init(void)
 {
-	serio_register_driver(&sunkbd_drv);
-	return 0;
+	return serio_register_driver(&sunkbd_drv);
 }
 
 static void __exit sunkbd_exit(void)
Index: work-fault-inject/drivers/input/keyboard/xtkbd.c
===================================================================
--- work-fault-inject.orig/drivers/input/keyboard/xtkbd.c
+++ work-fault-inject/drivers/input/keyboard/xtkbd.c
@@ -170,8 +170,7 @@ static struct serio_driver xtkbd_drv = {
 
 static int __init xtkbd_init(void)
 {
-	serio_register_driver(&xtkbd_drv);
-	return 0;
+	return serio_register_driver(&xtkbd_drv);
 }
 
 static void __exit xtkbd_exit(void)
Index: work-fault-inject/drivers/input/mouse/hil_ptr.c
===================================================================
--- work-fault-inject.orig/drivers/input/mouse/hil_ptr.c
+++ work-fault-inject/drivers/input/mouse/hil_ptr.c
@@ -417,8 +417,7 @@ static struct serio_driver hil_ptr_serio
 
 static int __init hil_ptr_init(void)
 {
-	serio_register_driver(&hil_ptr_serio_driver);
-        return 0;
+	return serio_register_driver(&hil_ptr_serio_driver);
 }
                 
 static void __exit hil_ptr_exit(void)
Index: work-fault-inject/drivers/input/mouse/sermouse.c
===================================================================
--- work-fault-inject.orig/drivers/input/mouse/sermouse.c
+++ work-fault-inject/drivers/input/mouse/sermouse.c
@@ -348,8 +348,7 @@ static struct serio_driver sermouse_drv 
 
 static int __init sermouse_init(void)
 {
-	serio_register_driver(&sermouse_drv);
-	return 0;
+	return serio_register_driver(&sermouse_drv);
 }
 
 static void __exit sermouse_exit(void)
Index: work-fault-inject/drivers/input/joystick/iforce/iforce-main.c
===================================================================
--- work-fault-inject.orig/drivers/input/joystick/iforce/iforce-main.c
+++ work-fault-inject/drivers/input/joystick/iforce/iforce-main.c
@@ -464,13 +464,21 @@ int iforce_init_device(struct iforce *if
 
 static int __init iforce_init(void)
 {
+	int err = 0;
+
 #ifdef CONFIG_JOYSTICK_IFORCE_USB
-	usb_register(&iforce_usb_driver);
+	err = usb_register(&iforce_usb_driver);
+	if (err)
+		return err;
 #endif
 #ifdef CONFIG_JOYSTICK_IFORCE_232
-	serio_register_driver(&iforce_serio_drv);
+	err = serio_register_driver(&iforce_serio_drv);
+#ifdef CONFIG_JOYSTICK_IFORCE_USB
+	if (err)
+		usb_deregister(&iforce_usb_driver);
+#endif
 #endif
-	return 0;
+	return err;
 }
 
 static void __exit iforce_exit(void)
Index: work-fault-inject/drivers/input/joystick/magellan.c
===================================================================
--- work-fault-inject.orig/drivers/input/joystick/magellan.c
+++ work-fault-inject/drivers/input/joystick/magellan.c
@@ -227,8 +227,7 @@ static struct serio_driver magellan_drv 
 
 static int __init magellan_init(void)
 {
-	serio_register_driver(&magellan_drv);
-	return 0;
+	return serio_register_driver(&magellan_drv);
 }
 
 static void __exit magellan_exit(void)
Index: work-fault-inject/drivers/input/joystick/spaceball.c
===================================================================
--- work-fault-inject.orig/drivers/input/joystick/spaceball.c
+++ work-fault-inject/drivers/input/joystick/spaceball.c
@@ -296,8 +296,7 @@ static struct serio_driver spaceball_drv
 
 static int __init spaceball_init(void)
 {
-	serio_register_driver(&spaceball_drv);
-	return 0;
+	return serio_register_driver(&spaceball_drv);
 }
 
 static void __exit spaceball_exit(void)
Index: work-fault-inject/drivers/input/joystick/spaceorb.c
===================================================================
--- work-fault-inject.orig/drivers/input/joystick/spaceorb.c
+++ work-fault-inject/drivers/input/joystick/spaceorb.c
@@ -242,8 +242,7 @@ static struct serio_driver spaceorb_drv 
 
 static int __init spaceorb_init(void)
 {
-	serio_register_driver(&spaceorb_drv);
-	return 0;
+	return serio_register_driver(&spaceorb_drv);
 }
 
 static void __exit spaceorb_exit(void)
Index: work-fault-inject/drivers/input/joystick/stinger.c
===================================================================
--- work-fault-inject.orig/drivers/input/joystick/stinger.c
+++ work-fault-inject/drivers/input/joystick/stinger.c
@@ -212,8 +212,7 @@ static struct serio_driver stinger_drv =
 
 static int __init stinger_init(void)
 {
-	serio_register_driver(&stinger_drv);
-	return 0;
+	return serio_register_driver(&stinger_drv);
 }
 
 static void __exit stinger_exit(void)
Index: work-fault-inject/drivers/input/joystick/twidjoy.c
===================================================================
--- work-fault-inject.orig/drivers/input/joystick/twidjoy.c
+++ work-fault-inject/drivers/input/joystick/twidjoy.c
@@ -265,8 +265,7 @@ static struct serio_driver twidjoy_drv =
 
 static int __init twidjoy_init(void)
 {
-	serio_register_driver(&twidjoy_drv);
-	return 0;
+	return serio_register_driver(&twidjoy_drv);
 }
 
 static void __exit twidjoy_exit(void)
Index: work-fault-inject/drivers/input/joystick/warrior.c
===================================================================
--- work-fault-inject.orig/drivers/input/joystick/warrior.c
+++ work-fault-inject/drivers/input/joystick/warrior.c
@@ -220,8 +220,7 @@ static struct serio_driver warrior_drv =
 
 static int __init warrior_init(void)
 {
-	serio_register_driver(&warrior_drv);
-	return 0;
+	return serio_register_driver(&warrior_drv);
 }
 
 static void __exit warrior_exit(void)

[PATCH 3/4] input: check whether serio dirver registration is completed

[Akinobu Mita]

This patch adds a flag to serio driver indicating whether registration is
complete and check that flag in serio_unregister_driver.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 drivers/input/serio/serio.c |    7 ++++++-
 include/linux/serio.h       |    1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Index: work-fault-inject/include/linux/serio.h
===================================================================
--- work-fault-inject.orig/include/linux/serio.h
+++ work-fault-inject/include/linux/serio.h
@@ -68,6 +68,7 @@ struct serio_driver {
 	void (*cleanup)(struct serio *);
 
 	struct device_driver driver;
+	int registered;
 };
 #define to_serio_driver(d)	container_of(d, struct serio_driver, driver)
 
Index: work-fault-inject/drivers/input/serio/serio.c
===================================================================
--- work-fault-inject.orig/drivers/input/serio/serio.c
+++ work-fault-inject/drivers/input/serio/serio.c
@@ -804,6 +804,8 @@ static void serio_add_driver(struct seri
 		printk(KERN_ERR
 			"serio: driver_register() failed for %s, error: %d\n",
 			drv->driver.name, error);
+	else
+		drv->registered = 1;
 }
 
 int __serio_register_driver(struct serio_driver *drv, struct module *owner)
@@ -830,7 +832,10 @@ start_over:
 		}
 	}
 
-	driver_unregister(&drv->driver);
+	if (drv->registered) {
+		driver_unregister(&drv->driver);
+		drv->registered = 0;
+	}
 	mutex_unlock(&serio_mutex);
 }
 

Re: [PATCH 3/4] input: check whether serio dirver registration is completed

[Alexey Dobriyan]

On Wed, Nov 08, 2006 at 09:40:10PM +0900, Akinobu Mita wrote:
> This patch adds a flag to serio driver indicating whether registration is
> complete and check that flag in serio_unregister_driver.

> --- work-fault-inject.orig/include/linux/serio.h
> +++ work-fault-inject/include/linux/serio.h
> @@ -68,6 +68,7 @@ struct serio_driver {
>  	void (*cleanup)(struct serio *);
>
>  	struct device_driver driver;
> +	int registered;

bitfield please.

> --- work-fault-inject.orig/drivers/input/serio/serio.c
> +++ work-fault-inject/drivers/input/serio/serio.c
> @@ -804,6 +804,8 @@ static void serio_add_driver(struct seri
>  		printk(KERN_ERR
>  			"serio: driver_register() failed for %s, error: %d\n",
>  			drv->driver.name, error);
> +	else
> +		drv->registered = 1;
>  }
>
>  int __serio_register_driver(struct serio_driver *drv, struct module *owne
> @@ -830,7 +832,10 @@ start_over:
>  		}
>  	}
>
> -	driver_unregister(&drv->driver);
> +	if (drv->registered) {
> +		driver_unregister(&drv->driver);
> +		drv->registered = 0;
> +	}

[PATCH 4/4] input: change to GFP_KERNEL for SERIO_REGISTER_DRIVER event allocation

[Akinobu Mita]

This patch changes allocation from GFP_ATOMIC to GFP_KERNEL for
SERIO_REGISTER_DRIVER events to make it more robust.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 drivers/input/serio/serio.c |   31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Index: work-fault-inject/drivers/input/serio/serio.c
===================================================================
--- work-fault-inject.orig/drivers/input/serio/serio.c
+++ work-fault-inject/drivers/input/serio/serio.c
@@ -191,12 +191,15 @@ static DECLARE_WAIT_QUEUE_HEAD(serio_wai
 static struct task_struct *serio_task;
 
 static int serio_queue_event(void *object, struct module *owner,
-			     enum serio_event_type event_type)
+			     enum serio_event_type event_type, gfp_t gfp_flags)
 {
 	unsigned long flags;
 	struct serio_event *event;
+	struct serio_event *new_event;
 	int err = 0;
 
+	new_event = kmalloc(sizeof(struct serio_event), gfp_flags);
+
 	spin_lock_irqsave(&serio_event_lock, flags);
 
 	/*
@@ -208,25 +211,27 @@ static int serio_queue_event(void *objec
 	 */
 	list_for_each_entry_reverse(event, &serio_event_list, node) {
 		if (event->object == object) {
-			if (event->type == event_type)
+			if (event->type == event_type) {
+				kfree(new_event);
 				goto out;
+			}
 			break;
 		}
 	}
 
-	if ((event = kmalloc(sizeof(struct serio_event), GFP_ATOMIC))) {
+	if (new_event) {
 		if (!try_module_get(owner)) {
 			err = -EINVAL;
 			printk(KERN_WARNING "serio: Can't get module reference, dropping event %d\n", event_type);
-			kfree(event);
+			kfree(new_event);
 			goto out;
 		}
 
-		event->type = event_type;
-		event->object = object;
-		event->owner = owner;
+		new_event->type = event_type;
+		new_event->object = object;
+		new_event->owner = owner;
 
-		list_add_tail(&event->node, &serio_event_list);
+		list_add_tail(&new_event->node, &serio_event_list);
 		wake_up(&serio_wait);
 	} else {
 		err = -ENOMEM;
@@ -679,12 +684,12 @@ static void serio_disconnect_port(struct
 
 void serio_rescan(struct serio *serio)
 {
-	serio_queue_event(serio, NULL, SERIO_RESCAN);
+	serio_queue_event(serio, NULL, SERIO_RESCAN, GFP_ATOMIC);
 }
 
 void serio_reconnect(struct serio *serio)
 {
-	serio_queue_event(serio, NULL, SERIO_RECONNECT);
+	serio_queue_event(serio, NULL, SERIO_RECONNECT, GFP_ATOMIC);
 }
 
 /*
@@ -694,7 +699,7 @@ void serio_reconnect(struct serio *serio
 void __serio_register_port(struct serio *serio, struct module *owner)
 {
 	serio_init_port(serio);
-	serio_queue_event(serio, owner, SERIO_REGISTER_PORT);
+	serio_queue_event(serio, owner, SERIO_REGISTER_PORT, GFP_ATOMIC);
 }
 
 /*
@@ -728,7 +733,7 @@ void serio_unregister_child_port(struct 
  */
 void __serio_unregister_port_delayed(struct serio *serio, struct module *owner)
 {
-	serio_queue_event(serio, owner, SERIO_UNREGISTER_PORT);
+	serio_queue_event(serio, owner, SERIO_UNREGISTER_PORT, GFP_ATOMIC);
 }
 
 
@@ -812,7 +817,7 @@ int __serio_register_driver(struct serio
 {
 	drv->driver.bus = &serio_bus;
 
-	return serio_queue_event(drv, owner, SERIO_REGISTER_DRIVER);
+	return serio_queue_event(drv, owner, SERIO_REGISTER_DRIVER, GFP_KERNEL);
 }
 
 void serio_unregister_driver(struct serio_driver *drv)

Re: [PATCH 1/2] input: make serio_register_driver() return error code

[Dmitry Torokhov]

On Wednesday 08 November 2006 07:36, Akinobu Mita wrote:
> 
> 2) driver_register() failure by kseriod.
> 
>    This failure cannot be checked by serio_register_driver().
>    But it is necessary to prevent serio_unregister_driver() from
>    trying to call driver_unregister() with not registered driver
>    by adding flag to serio driver indicating whether registration is
>    complete.
> 

Hi Akinobu,

I think I found a way to handle all errors when registering serio driver.
What do you think about the patch below?

-- 
Dmitry

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/serio/serio.c |  109 ++++++++++++++++++++++++++++++--------------
 include/linux/serio.h       |    7 --
 2 files changed, 76 insertions(+), 40 deletions(-)

Index: work/drivers/input/serio/serio.c
===================================================================
--- work.orig/drivers/input/serio/serio.c
+++ work/drivers/input/serio/serio.c
@@ -44,7 +44,7 @@ EXPORT_SYMBOL(serio_interrupt);
 EXPORT_SYMBOL(__serio_register_port);
 EXPORT_SYMBOL(serio_unregister_port);
 EXPORT_SYMBOL(serio_unregister_child_port);
-EXPORT_SYMBOL(__serio_register_driver);
+EXPORT_SYMBOL(serio_register_driver);
 EXPORT_SYMBOL(serio_unregister_driver);
 EXPORT_SYMBOL(serio_open);
 EXPORT_SYMBOL(serio_close);
@@ -61,10 +61,10 @@ static LIST_HEAD(serio_list);
 
 static struct bus_type serio_bus;
 
-static void serio_add_driver(struct serio_driver *drv);
 static void serio_add_port(struct serio *serio);
 static void serio_reconnect_port(struct serio *serio);
 static void serio_disconnect_port(struct serio *serio);
+static void serio_attach_driver(struct serio_driver *drv);
 
 static int serio_connect_driver(struct serio *serio, struct serio_driver *drv)
 {
@@ -168,10 +168,10 @@ static void serio_find_driver(struct ser
  */
 
 enum serio_event_type {
-	SERIO_RESCAN,
-	SERIO_RECONNECT,
+	SERIO_RESCAN_PORT,
+	SERIO_RECONNECT_PORT,
 	SERIO_REGISTER_PORT,
-	SERIO_REGISTER_DRIVER,
+	SERIO_ATTACH_DRIVER,
 };
 
 struct serio_event {
@@ -186,11 +186,12 @@ static LIST_HEAD(serio_event_list);
 static DECLARE_WAIT_QUEUE_HEAD(serio_wait);
 static struct task_struct *serio_task;
 
-static void serio_queue_event(void *object, struct module *owner,
-			      enum serio_event_type event_type)
+static int serio_queue_event(void *object, struct module *owner,
+			     enum serio_event_type event_type)
 {
 	unsigned long flags;
 	struct serio_event *event;
+	int retval = 0;
 
 	spin_lock_irqsave(&serio_event_lock, flags);
 
@@ -209,24 +210,34 @@ static void serio_queue_event(void *obje
 		}
 	}
 
-	if ((event = kmalloc(sizeof(struct serio_event), GFP_ATOMIC))) {
-		if (!try_module_get(owner)) {
-			printk(KERN_WARNING "serio: Can't get module reference, dropping event %d\n", event_type);
-			kfree(event);
-			goto out;
-		}
-
-		event->type = event_type;
-		event->object = object;
-		event->owner = owner;
+	event = kmalloc(sizeof(struct serio_event), GFP_ATOMIC);
+	if (!event) {
+		printk(KERN_ERR
+			"serio: Not enough memory to queue event %d\n",
+			event_type);
+		retval = -ENOMEM;
+		goto out;
+	}
 
-		list_add_tail(&event->node, &serio_event_list);
-		wake_up(&serio_wait);
-	} else {
-		printk(KERN_ERR "serio: Not enough memory to queue event %d\n", event_type);
+	if (!try_module_get(owner)) {
+		printk(KERN_WARNING
+			"serio: Can't get module reference, dropping event %d\n",
+			event_type);
+		kfree(event);
+		retval = -EINVAL;
+		goto out;
 	}
+
+	event->type = event_type;
+	event->object = object;
+	event->owner = owner;
+
+	list_add_tail(&event->node, &serio_event_list);
+	wake_up(&serio_wait);
+
 out:
 	spin_unlock_irqrestore(&serio_event_lock, flags);
+	return retval;
 }
 
 static void serio_free_event(struct serio_event *event)
@@ -304,17 +315,17 @@ static void serio_handle_event(void)
 				serio_add_port(event->object);
 				break;
 
-			case SERIO_RECONNECT:
+			case SERIO_RECONNECT_PORT:
 				serio_reconnect_port(event->object);
 				break;
 
-			case SERIO_RESCAN:
+			case SERIO_RESCAN_PORT:
 				serio_disconnect_port(event->object);
 				serio_find_driver(event->object);
 				break;
 
-			case SERIO_REGISTER_DRIVER:
-				serio_add_driver(event->object);
+			case SERIO_ATTACH_DRIVER:
+				serio_attach_driver(event->object);
 				break;
 
 			default:
@@ -666,12 +677,12 @@ static void serio_disconnect_port(struct
 
 void serio_rescan(struct serio *serio)
 {
-	serio_queue_event(serio, NULL, SERIO_RESCAN);
+	serio_queue_event(serio, NULL, SERIO_RESCAN_PORT);
 }
 
 void serio_reconnect(struct serio *serio)
 {
-	serio_queue_event(serio, NULL, SERIO_RECONNECT);
+	serio_queue_event(serio, NULL, SERIO_RECONNECT_PORT);
 }
 
 /*
@@ -766,22 +777,52 @@ static int serio_driver_remove(struct de
 	return 0;
 }
 
-static void serio_add_driver(struct serio_driver *drv)
+static void serio_attach_driver(struct serio_driver *drv)
 {
 	int error;
 
-	error = driver_register(&drv->driver);
+	error = driver_attach(&drv->driver);
 	if (error)
-		printk(KERN_ERR
-			"serio: driver_register() failed for %s, error: %d\n",
-			drv->driver.name, error);
+		printk(KERN_WARNING
+			"serio: driver_attach() failed with error %d\n",
+			error);
 }
 
-void __serio_register_driver(struct serio_driver *drv, struct module *owner)
+int serio_register_driver(struct serio_driver *drv)
 {
+	int manual_bind = drv->manual_bind;
+	int error;
+
 	drv->driver.bus = &serio_bus;
 
-	serio_queue_event(drv, owner, SERIO_REGISTER_DRIVER);
+	/*
+	 * Temporarily disable automatic binding because probing
+	 * takes long time and we are better off doing it in kseriod
+	 */
+	drv->manual_bind = 1;
+
+	error = driver_register(&drv->driver);
+	if (error) {
+		printk(KERN_ERR
+			"serio: driver_register() failed for %s, error: %d\n",
+			drv->driver.name, error);
+		return error;
+	}
+
+	/*
+	 * Restore original bind mode and let kseriod bind the
+	 * driver to free ports
+	 */
+	if (!manual_bind) {
+		drv->manual_bind = 0;
+		error = serio_queue_event(drv, NULL, SERIO_ATTACH_DRIVER);
+		if (error) {
+			driver_unregister(&drv->driver);
+			return error;
+		}
+	}
+
+	return 0;
 }
 
 void serio_unregister_driver(struct serio_driver *drv)
Index: work/include/linux/serio.h
===================================================================
--- work.orig/include/linux/serio.h
+++ work/include/linux/serio.h
@@ -86,12 +86,7 @@ static inline void serio_register_port(s
 void serio_unregister_port(struct serio *serio);
 void serio_unregister_child_port(struct serio *serio);
 
-void __serio_register_driver(struct serio_driver *drv, struct module *owner);
-static inline void serio_register_driver(struct serio_driver *drv)
-{
-	__serio_register_driver(drv, THIS_MODULE);
-}
-
+int serio_register_driver(struct serio_driver *drv);
 void serio_unregister_driver(struct serio_driver *drv);
 
 static inline int serio_write(struct serio *serio, unsigned char data)

Re: [PATCH 1/2] input: make serio_register_driver() return error code

[Akinobu Mita]

On Fri, Nov 17, 2006 at 01:37:34AM -0500, Dmitry Torokhov wrote:
> I think I found a way to handle all errors when registering serio driver.
> What do you think about the patch below?
> 

Looks good to me.
I also tested this patch with my patch 2/4 (which actually checking
the return code of serio_register_driver() for each input driver).

Acked-by: Akinobu Mita <akinobu.mita@gmail.com>