[PATCH] Input: q40kbd - convert driver to the split model

[PATCH] Input: q40kbd - convert driver to the split model

[Dmitry Torokhov]

Convert the driver to standard spilt model arch-specific code registers
platform device to which driver code can bind later.

Also request IRQ immediately upon binding to the device instead of doing
this when serio port is being opened.

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

Not tested as I don't have the hardware...

 arch/m68k/q40/config.c       |   12 ++++
 drivers/input/serio/q40kbd.c |  134 ++++++++++++++++++++++++------------------
 2 files changed, 87 insertions(+), 59 deletions(-)

diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
index ad10fec..27d3f76 100644
--- a/arch/m68k/q40/config.c
+++ b/arch/m68k/q40/config.c
@@ -24,6 +24,7 @@
 #include <linux/rtc.h>
 #include <linux/vt_kern.h>
 #include <linux/bcd.h>
+#include <linux/platform_device.h>
 
 #include <asm/io.h>
 #include <asm/rtc.h>
@@ -329,3 +330,14 @@ static int q40_set_rtc_pll(struct rtc_pll_info *pll)
 	} else
 		return -EINVAL;
 }
+
+static struct platform_device q40_kbd_pdev = {
+	.name	= "q40kbd",
+	.id	= -1,
+};
+
+static __init int q40_add_kbd_device(void)
+{
+	return platform_device_register(&q40_kbd_pdev);
+}
+arch_initcall(q40_add_kbd_device);
diff --git a/drivers/input/serio/q40kbd.c b/drivers/input/serio/q40kbd.c
index 5eb84b3..1e61dd4 100644
--- a/drivers/input/serio/q40kbd.c
+++ b/drivers/input/serio/q40kbd.c
@@ -44,26 +44,31 @@
 #include <asm/irq.h>
 #include <asm/q40ints.h>
 
+#define DRV_NAME	"q40kbd"
+
 MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
 MODULE_DESCRIPTION("Q40 PS/2 keyboard controller driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
 
-static DEFINE_SPINLOCK(q40kbd_lock);
-static struct serio *q40kbd_port;
-static struct platform_device *q40kbd_device;
+struct q40kbd {
+	struct serio *port;
+	spinlock_t lock;
+};
 
 static irqreturn_t q40kbd_interrupt(int irq, void *dev_id)
 {
+	struct q40kbd *q40kbd = dev_id;
 	unsigned long flags;
 
-	spin_lock_irqsave(&q40kbd_lock, flags);
+	spin_lock_irqsave(&q40kbd->lock, flags);
 
 	if (Q40_IRQ_KEYB_MASK & master_inb(INTERRUPT_REG))
-		serio_interrupt(q40kbd_port, master_inb(KEYCODE_REG), 0);
+		serio_interrupt(q40kbd->port, master_inb(KEYCODE_REG), 0);
 
 	master_outb(-1, KEYBOARD_UNLOCK_REG);
 
-	spin_unlock_irqrestore(&q40kbd_lock, flags);
+	spin_unlock_irqrestore(&q40kbd->lock, flags);
 
 	return IRQ_HANDLED;
 }
@@ -72,17 +77,23 @@ static irqreturn_t q40kbd_interrupt(int irq, void *dev_id)
  * q40kbd_flush() flushes all data that may be in the keyboard buffers
  */
 
-static void q40kbd_flush(void)
+static void q40kbd_flush(struct q40kbd *q40kbd)
 {
 	int maxread = 100;
 	unsigned long flags;
 
-	spin_lock_irqsave(&q40kbd_lock, flags);
+	spin_lock_irqsave(&q40kbd->lock, flags);
 
 	while (maxread-- && (Q40_IRQ_KEYB_MASK & master_inb(INTERRUPT_REG)))
 		master_inb(KEYCODE_REG);
 
-	spin_unlock_irqrestore(&q40kbd_lock, flags);
+	spin_unlock_irqrestore(&q40kbd->lock, flags);
+}
+
+static void q40kbd_stop(struct q40kbd *q40kbd)
+{
+	master_outb(0, KEY_IRQ_ENABLE_REG);
+	master_outb(-1, KEYBOARD_UNLOCK_REG);
 }
 
 /*
@@ -92,12 +103,9 @@ static void q40kbd_flush(void)
 
 static int q40kbd_open(struct serio *port)
 {
-	q40kbd_flush();
+	struct q40kbd *q40kbd = port->port_data;
 
-	if (request_irq(Q40_IRQ_KEYBOARD, q40kbd_interrupt, 0, "q40kbd", NULL)) {
-		printk(KERN_ERR "q40kbd.c: Can't get irq %d.\n", Q40_IRQ_KEYBOARD);
-		return -EBUSY;
-	}
+	q40kbd_flush(q40kbd);
 
 	/* off we go */
 	master_outb(-1, KEYBOARD_UNLOCK_REG);
@@ -108,35 +116,68 @@ static int q40kbd_open(struct serio *port)
 
 static void q40kbd_close(struct serio *port)
 {
-	master_outb(0, KEY_IRQ_ENABLE_REG);
-	master_outb(-1, KEYBOARD_UNLOCK_REG);
-	free_irq(Q40_IRQ_KEYBOARD, NULL);
+	struct q40kbd *q40kbd = port->port_data;
 
-	q40kbd_flush();
+	q40kbd_stop(q40kbd);
+	q40kbd_flush(q40kbd);
 }
 
-static int __devinit q40kbd_probe(struct platform_device *dev)
+static int __devinit q40kbd_probe(struct platform_device *pdev)
 {
-	q40kbd_port = kzalloc(sizeof(struct serio), GFP_KERNEL);
-	if (!q40kbd_port)
-		return -ENOMEM;
-
-	q40kbd_port->id.type	= SERIO_8042;
-	q40kbd_port->open	= q40kbd_open;
-	q40kbd_port->close	= q40kbd_close;
-	q40kbd_port->dev.parent	= &dev->dev;
-	strlcpy(q40kbd_port->name, "Q40 Kbd Port", sizeof(q40kbd_port->name));
-	strlcpy(q40kbd_port->phys, "Q40", sizeof(q40kbd_port->phys));
-
-	serio_register_port(q40kbd_port);
+	struct q40kbd *q40kbd;
+	struct serio *port;
+	int error;
+
+	q40kbd = kzalloc(sizeof(struct q40kbd), GFP_KERNEL);
+	port = kzalloc(sizeof(struct serio), GFP_KERNEL);
+	if (!q40kbd || !port) {
+		error = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	q40kbd->port = port;
+	spin_lock_init(&q40kbd->lock);
+
+	port->id.type = SERIO_8042;
+	port->open = q40kbd_open;
+	port->close = q40kbd_close;
+	port->port_data = q40kbd;
+	port->dev.parent = &pdev->dev;
+	strlcpy(port->name, "Q40 Kbd Port", sizeof(port->name));
+	strlcpy(port->phys, "Q40", sizeof(port->phys));
+
+	q40kbd_stop(q40kbd);
+
+	error = request_irq(Q40_IRQ_KEYBOARD, q40kbd_interrupt, 0,
+			    DRV_NAME, q40kbd);
+	if (error) {
+		dev_err(&pdev->dev, "Can't get irq %d.\n", Q40_IRQ_KEYBOARD);
+		goto err_free_mem;
+	}
+
+	serio_register_port(q40kbd->port);
+
+	platform_set_drvdata(pdev, q40kbd);
 	printk(KERN_INFO "serio: Q40 kbd registered\n");
 
 	return 0;
+
+err_free_mem:
+	kfree(port);
+	kfree(q40kbd);
+	return error;
 }
 
-static int __devexit q40kbd_remove(struct platform_device *dev)
+static int __devexit q40kbd_remove(struct platform_device *pdev)
 {
-	serio_unregister_port(q40kbd_port);
+	struct q40kbd *q40kbd = platform_get_drvdata(pdev);
+
+	free_irq(Q40_IRQ_KEYBOARD, q40kbd);
+
+	serio_unregister_port(q40kbd->port);
+	kfree(q40kbd);
+
+	platform_set_drvdata(pdev, NULL);
 
 	return 0;
 }
@@ -146,41 +187,16 @@ static struct platform_driver q40kbd_driver = {
 		.name	= "q40kbd",
 		.owner	= THIS_MODULE,
 	},
-	.probe		= q40kbd_probe,
 	.remove		= __devexit_p(q40kbd_remove),
 };
 
 static int __init q40kbd_init(void)
 {
-	int error;
-
-	if (!MACH_IS_Q40)
-		return -ENODEV;
-
-	error = platform_driver_register(&q40kbd_driver);
-	if (error)
-		return error;
-
-	q40kbd_device = platform_device_alloc("q40kbd", -1);
-	if (!q40kbd_device)
-		goto err_unregister_driver;
-
-	error = platform_device_add(q40kbd_device);
-	if (error)
-		goto err_free_device;
-
-	return 0;
-
- err_free_device:
-	platform_device_put(q40kbd_device);
- err_unregister_driver:
-	platform_driver_unregister(&q40kbd_driver);
-	return error;
+	return platform_driver_probe(&q40kbd_driver, q40kbd_probe);
 }
 
 static void __exit q40kbd_exit(void)
 {
-	platform_device_unregister(q40kbd_device);
 	platform_driver_unregister(&q40kbd_driver);
 }
 

Re: [PATCH] Input: q40kbd - convert driver to the split model

[Dmitry Torokhov]

On Fri, Dec 30, 2011 at 05:11:11PM -0800, Dmitry Torokhov wrote:
> Convert the driver to standard spilt model arch-specific code registers
> platform device to which driver code can bind later.
> 
> Also request IRQ immediately upon binding to the device instead of doing
> this when serio port is being opened.
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
> Not tested as I don't have the hardware...

If anyone could give this patch a spin it would be most appreciated.

Thanks!

> 
>  arch/m68k/q40/config.c       |   12 ++++
>  drivers/input/serio/q40kbd.c |  134 ++++++++++++++++++++++++------------------
>  2 files changed, 87 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
> index ad10fec..27d3f76 100644
> --- a/arch/m68k/q40/config.c
> +++ b/arch/m68k/q40/config.c
> @@ -24,6 +24,7 @@
>  #include <linux/rtc.h>
>  #include <linux/vt_kern.h>
>  #include <linux/bcd.h>
> +#include <linux/platform_device.h>
>  
>  #include <asm/io.h>
>  #include <asm/rtc.h>
> @@ -329,3 +330,14 @@ static int q40_set_rtc_pll(struct rtc_pll_info *pll)
>  	} else
>  		return -EINVAL;
>  }
> +
> +static struct platform_device q40_kbd_pdev = {
> +	.name	= "q40kbd",
> +	.id	= -1,
> +};
> +
> +static __init int q40_add_kbd_device(void)
> +{
> +	return platform_device_register(&q40_kbd_pdev);
> +}
> +arch_initcall(q40_add_kbd_device);
> diff --git a/drivers/input/serio/q40kbd.c b/drivers/input/serio/q40kbd.c
> index 5eb84b3..1e61dd4 100644
> --- a/drivers/input/serio/q40kbd.c
> +++ b/drivers/input/serio/q40kbd.c
> @@ -44,26 +44,31 @@
>  #include <asm/irq.h>
>  #include <asm/q40ints.h>
>  
> +#define DRV_NAME	"q40kbd"
> +
>  MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
>  MODULE_DESCRIPTION("Q40 PS/2 keyboard controller driver");
>  MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
>  
> -static DEFINE_SPINLOCK(q40kbd_lock);
> -static struct serio *q40kbd_port;
> -static struct platform_device *q40kbd_device;
> +struct q40kbd {
> +	struct serio *port;
> +	spinlock_t lock;
> +};
>  
>  static irqreturn_t q40kbd_interrupt(int irq, void *dev_id)
>  {
> +	struct q40kbd *q40kbd = dev_id;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&q40kbd_lock, flags);
> +	spin_lock_irqsave(&q40kbd->lock, flags);
>  
>  	if (Q40_IRQ_KEYB_MASK & master_inb(INTERRUPT_REG))
> -		serio_interrupt(q40kbd_port, master_inb(KEYCODE_REG), 0);
> +		serio_interrupt(q40kbd->port, master_inb(KEYCODE_REG), 0);
>  
>  	master_outb(-1, KEYBOARD_UNLOCK_REG);
>  
> -	spin_unlock_irqrestore(&q40kbd_lock, flags);
> +	spin_unlock_irqrestore(&q40kbd->lock, flags);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -72,17 +77,23 @@ static irqreturn_t q40kbd_interrupt(int irq, void *dev_id)
>   * q40kbd_flush() flushes all data that may be in the keyboard buffers
>   */
>  
> -static void q40kbd_flush(void)
> +static void q40kbd_flush(struct q40kbd *q40kbd)
>  {
>  	int maxread = 100;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&q40kbd_lock, flags);
> +	spin_lock_irqsave(&q40kbd->lock, flags);
>  
>  	while (maxread-- && (Q40_IRQ_KEYB_MASK & master_inb(INTERRUPT_REG)))
>  		master_inb(KEYCODE_REG);
>  
> -	spin_unlock_irqrestore(&q40kbd_lock, flags);
> +	spin_unlock_irqrestore(&q40kbd->lock, flags);
> +}
> +
> +static void q40kbd_stop(struct q40kbd *q40kbd)
> +{
> +	master_outb(0, KEY_IRQ_ENABLE_REG);
> +	master_outb(-1, KEYBOARD_UNLOCK_REG);
>  }
>  
>  /*
> @@ -92,12 +103,9 @@ static void q40kbd_flush(void)
>  
>  static int q40kbd_open(struct serio *port)
>  {
> -	q40kbd_flush();
> +	struct q40kbd *q40kbd = port->port_data;
>  
> -	if (request_irq(Q40_IRQ_KEYBOARD, q40kbd_interrupt, 0, "q40kbd", NULL)) {
> -		printk(KERN_ERR "q40kbd.c: Can't get irq %d.\n", Q40_IRQ_KEYBOARD);
> -		return -EBUSY;
> -	}
> +	q40kbd_flush(q40kbd);
>  
>  	/* off we go */
>  	master_outb(-1, KEYBOARD_UNLOCK_REG);
> @@ -108,35 +116,68 @@ static int q40kbd_open(struct serio *port)
>  
>  static void q40kbd_close(struct serio *port)
>  {
> -	master_outb(0, KEY_IRQ_ENABLE_REG);
> -	master_outb(-1, KEYBOARD_UNLOCK_REG);
> -	free_irq(Q40_IRQ_KEYBOARD, NULL);
> +	struct q40kbd *q40kbd = port->port_data;
>  
> -	q40kbd_flush();
> +	q40kbd_stop(q40kbd);
> +	q40kbd_flush(q40kbd);
>  }
>  
> -static int __devinit q40kbd_probe(struct platform_device *dev)
> +static int __devinit q40kbd_probe(struct platform_device *pdev)
>  {
> -	q40kbd_port = kzalloc(sizeof(struct serio), GFP_KERNEL);
> -	if (!q40kbd_port)
> -		return -ENOMEM;
> -
> -	q40kbd_port->id.type	= SERIO_8042;
> -	q40kbd_port->open	= q40kbd_open;
> -	q40kbd_port->close	= q40kbd_close;
> -	q40kbd_port->dev.parent	= &dev->dev;
> -	strlcpy(q40kbd_port->name, "Q40 Kbd Port", sizeof(q40kbd_port->name));
> -	strlcpy(q40kbd_port->phys, "Q40", sizeof(q40kbd_port->phys));
> -
> -	serio_register_port(q40kbd_port);
> +	struct q40kbd *q40kbd;
> +	struct serio *port;
> +	int error;
> +
> +	q40kbd = kzalloc(sizeof(struct q40kbd), GFP_KERNEL);
> +	port = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +	if (!q40kbd || !port) {
> +		error = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	q40kbd->port = port;
> +	spin_lock_init(&q40kbd->lock);
> +
> +	port->id.type = SERIO_8042;
> +	port->open = q40kbd_open;
> +	port->close = q40kbd_close;
> +	port->port_data = q40kbd;
> +	port->dev.parent = &pdev->dev;
> +	strlcpy(port->name, "Q40 Kbd Port", sizeof(port->name));
> +	strlcpy(port->phys, "Q40", sizeof(port->phys));
> +
> +	q40kbd_stop(q40kbd);
> +
> +	error = request_irq(Q40_IRQ_KEYBOARD, q40kbd_interrupt, 0,
> +			    DRV_NAME, q40kbd);
> +	if (error) {
> +		dev_err(&pdev->dev, "Can't get irq %d.\n", Q40_IRQ_KEYBOARD);
> +		goto err_free_mem;
> +	}
> +
> +	serio_register_port(q40kbd->port);
> +
> +	platform_set_drvdata(pdev, q40kbd);
>  	printk(KERN_INFO "serio: Q40 kbd registered\n");
>  
>  	return 0;
> +
> +err_free_mem:
> +	kfree(port);
> +	kfree(q40kbd);
> +	return error;
>  }
>  
> -static int __devexit q40kbd_remove(struct platform_device *dev)
> +static int __devexit q40kbd_remove(struct platform_device *pdev)
>  {
> -	serio_unregister_port(q40kbd_port);
> +	struct q40kbd *q40kbd = platform_get_drvdata(pdev);
> +
> +	free_irq(Q40_IRQ_KEYBOARD, q40kbd);
> +
> +	serio_unregister_port(q40kbd->port);
> +	kfree(q40kbd);
> +
> +	platform_set_drvdata(pdev, NULL);
>  
>  	return 0;
>  }
> @@ -146,41 +187,16 @@ static struct platform_driver q40kbd_driver = {
>  		.name	= "q40kbd",
>  		.owner	= THIS_MODULE,
>  	},
> -	.probe		= q40kbd_probe,
>  	.remove		= __devexit_p(q40kbd_remove),
>  };
>  
>  static int __init q40kbd_init(void)
>  {
> -	int error;
> -
> -	if (!MACH_IS_Q40)
> -		return -ENODEV;
> -
> -	error = platform_driver_register(&q40kbd_driver);
> -	if (error)
> -		return error;
> -
> -	q40kbd_device = platform_device_alloc("q40kbd", -1);
> -	if (!q40kbd_device)
> -		goto err_unregister_driver;
> -
> -	error = platform_device_add(q40kbd_device);
> -	if (error)
> -		goto err_free_device;
> -
> -	return 0;
> -
> - err_free_device:
> -	platform_device_put(q40kbd_device);
> - err_unregister_driver:
> -	platform_driver_unregister(&q40kbd_driver);
> -	return error;
> +	return platform_driver_probe(&q40kbd_driver, q40kbd_probe);
>  }
>  
>  static void __exit q40kbd_exit(void)
>  {
> -	platform_device_unregister(q40kbd_device);
>  	platform_driver_unregister(&q40kbd_driver);
>  }
>  
> 

-- 
Dmitry

Re: [PATCH] Input: q40kbd - convert driver to the split model

[Geert Uytterhoeven]

Hi Dmitry,

On Sat, Dec 31, 2011 at 02:11, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Convert the driver to standard spilt model arch-specific code registers
> platform device to which driver code can bind later.
>
> Also request IRQ immediately upon binding to the device instead of doing
> this when serio port is being opened.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
> Not tested as I don't have the hardware...

Unfortunately I also don't have the hardware.
But it compiles. A few comments below.

> diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
> index ad10fec..27d3f76 100644
> --- a/arch/m68k/q40/config.c
> +++ b/arch/m68k/q40/config.c

> +static struct platform_device q40_kbd_pdev = {
> +       .name   = "q40kbd",
> +       .id     = -1,
> +};
> +
> +static __init int q40_add_kbd_device(void)
> +{
> +       return platform_device_register(&q40_kbd_pdev);

If you would use platform_device_register_simple(), you don't need the
q40_kbd_pdev above, reducing memory consumption on non-Q40 platforms.

> +}
> +arch_initcall(q40_add_kbd_device);

For the Amiga platform drivers, I used device_initcall().

> diff --git a/drivers/input/serio/q40kbd.c b/drivers/input/serio/q40kbd.c
> index 5eb84b3..1e61dd4 100644
> --- a/drivers/input/serio/q40kbd.c
> +++ b/drivers/input/serio/q40kbd.c
>
> +static void q40kbd_stop(struct q40kbd *q40kbd)
> +{

The q40kbd parameters is unused and can be removed.

> -static int __devinit q40kbd_probe(struct platform_device *dev)
> +static int __devinit q40kbd_probe(struct platform_device *pdev)

> +       serio_register_port(q40kbd->port);

This cannot fail?
I see it returns void, but serio_queue_event() inside can fail and thus
won't propagate its error condition and code.

> -static int __devexit q40kbd_remove(struct platform_device *dev)
> +static int __devexit q40kbd_remove(struct platform_device *pdev)
>  {
> -       serio_unregister_port(q40kbd_port);
> +       struct q40kbd *q40kbd = platform_get_drvdata(pdev);
> +
> +       free_irq(Q40_IRQ_KEYBOARD, q40kbd);
> +
> +       serio_unregister_port(q40kbd->port);

Should the unregister be done before freeing the IRQ, i.e. reverse
order compared to probe?

> +       kfree(q40kbd);
> +
> +       platform_set_drvdata(pdev, NULL);
>
>        return 0;
>  }

Thanks, it's a nice cleanup!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: q40kbd - convert driver to the split model

[Dmitry Torokhov]

On Wed, Jan 11, 2012 at 08:55:54AM +0100, Geert Uytterhoeven wrote:
> Hi Dmitry,
> 
> On Sat, Dec 31, 2011 at 02:11, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Convert the driver to standard spilt model arch-specific code registers
> > platform device to which driver code can bind later.
> >
> > Also request IRQ immediately upon binding to the device instead of doing
> > this when serio port is being opened.
> >
> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> > ---
> >
> > Not tested as I don't have the hardware...
> 
> Unfortunately I also don't have the hardware.
> But it compiles. A few comments below.
> 
> > diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
> > index ad10fec..27d3f76 100644
> > --- a/arch/m68k/q40/config.c
> > +++ b/arch/m68k/q40/config.c
> 
> > +static struct platform_device q40_kbd_pdev = {
> > +       .name   = "q40kbd",
> > +       .id     = -1,
> > +};
> > +
> > +static __init int q40_add_kbd_device(void)
> > +{
> > +       return platform_device_register(&q40_kbd_pdev);
> 
> If you would use platform_device_register_simple(), you don't need the
> q40_kbd_pdev above, reducing memory consumption on non-Q40 platforms.

Isn't this file only compiled on q40 platforms?

> 
> > +}
> > +arch_initcall(q40_add_kbd_device);
> 
> For the Amiga platform drivers, I used device_initcall().

Won't it potentially race with initialization of q40kbd? It looks like
module_initcall is the same as device_initcall() when module is compiled
in. Given that q40kbd uses platform_dveice_probe() losing race might be
fatal.

> 
> > diff --git a/drivers/input/serio/q40kbd.c b/drivers/input/serio/q40kbd.c
> > index 5eb84b3..1e61dd4 100644
> > --- a/drivers/input/serio/q40kbd.c
> > +++ b/drivers/input/serio/q40kbd.c
> >
> > +static void q40kbd_stop(struct q40kbd *q40kbd)
> > +{
> 
> The q40kbd parameters is unused and can be removed.
> 
> > -static int __devinit q40kbd_probe(struct platform_device *dev)
> > +static int __devinit q40kbd_probe(struct platform_device *pdev)
> 
> > +       serio_register_port(q40kbd->port);
> 
> This cannot fail?
> I see it returns void, but serio_queue_event() inside can fail and thus
> won't propagate its error condition and code.

Yes, this is current limitation of serio.

> 
> > -static int __devexit q40kbd_remove(struct platform_device *dev)
> > +static int __devexit q40kbd_remove(struct platform_device *pdev)
> >  {
> > -       serio_unregister_port(q40kbd_port);
> > +       struct q40kbd *q40kbd = platform_get_drvdata(pdev);
> > +
> > +       free_irq(Q40_IRQ_KEYBOARD, q40kbd);
> > +
> > +       serio_unregister_port(q40kbd->port);
> 
> Should the unregister be done before freeing the IRQ, i.e. reverse
> order compared to probe?

Unregister will most likely cause memory being freed; you don't want to
chance IRQ firing here.

> 
> > +       kfree(q40kbd);
> > +
> > +       platform_set_drvdata(pdev, NULL);
> >
> >        return 0;
> >  }
> 
> Thanks, it's a nice cleanup!
> 

Thanks for lookign this over.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: q40kbd - convert driver to the split model

[Geert Uytterhoeven]

On Wed, Jan 11, 2012 at 09:11, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Jan 11, 2012 at 08:55:54AM +0100, Geert Uytterhoeven wrote:
>> On Sat, Dec 31, 2011 at 02:11, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:

>> > +static struct platform_device q40_kbd_pdev = {
>> > +       .name   = "q40kbd",
>> > +       .id     = -1,
>> > +};
>> > +
>> > +static __init int q40_add_kbd_device(void)
>> > +{
>> > +       return platform_device_register(&q40_kbd_pdev);
>>
>> If you would use platform_device_register_simple(), you don't need the
>> q40_kbd_pdev above, reducing memory consumption on non-Q40 platforms.
>
> Isn't this file only compiled on q40 platforms?

No, m68k does support multi-platform kernels.

>> > +}
>> > +arch_initcall(q40_add_kbd_device);
>>
>> For the Amiga platform drivers, I used device_initcall().
>
> Won't it potentially race with initialization of q40kbd? It looks like
> module_initcall is the same as device_initcall() when module is compiled
> in. Given that q40kbd uses platform_dveice_probe() losing race might be
> fatal.

So far I haven't encountered any problems on Amiga.
I'll look into this.

>> > -static int __devexit q40kbd_remove(struct platform_device *dev)
>> > +static int __devexit q40kbd_remove(struct platform_device *pdev)
>> >  {
>> > -       serio_unregister_port(q40kbd_port);
>> > +       struct q40kbd *q40kbd = platform_get_drvdata(pdev);
>> > +
>> > +       free_irq(Q40_IRQ_KEYBOARD, q40kbd);
>> > +
>> > +       serio_unregister_port(q40kbd->port);
>>
>> Should the unregister be done before freeing the IRQ, i.e. reverse
>> order compared to probe?
>
> Unregister will most likely cause memory being freed; you don't want to
> chance IRQ firing here.

And in the probe case that can't happen when request_irq() is called?

Can a call to q40kbd_stop() first fix that?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] Input: q40kbd - convert driver to the split model

[Dmitry Torokhov]

On Wed, Jan 11, 2012 at 10:25:20AM +0100, Geert Uytterhoeven wrote:
> On Wed, Jan 11, 2012 at 09:11, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Jan 11, 2012 at 08:55:54AM +0100, Geert Uytterhoeven wrote:
> >> On Sat, Dec 31, 2011 at 02:11, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> 
> >> > +static struct platform_device q40_kbd_pdev = {
> >> > +       .name   = "q40kbd",
> >> > +       .id     = -1,
> >> > +};
> >> > +
> >> > +static __init int q40_add_kbd_device(void)
> >> > +{
> >> > +       return platform_device_register(&q40_kbd_pdev);
> >>
> >> If you would use platform_device_register_simple(), you don't need the
> >> q40_kbd_pdev above, reducing memory consumption on non-Q40 platforms.
> >
> > Isn't this file only compiled on q40 platforms?
> 
> No, m68k does support multi-platform kernels.

OK, I'll do dynamic platform device allocation.

> 
> >> > +}
> >> > +arch_initcall(q40_add_kbd_device);
> >>
> >> For the Amiga platform drivers, I used device_initcall().
> >
> > Won't it potentially race with initialization of q40kbd? It looks like
> > module_initcall is the same as device_initcall() when module is compiled
> > in. Given that q40kbd uses platform_dveice_probe() losing race might be
> > fatal.
> 
> So far I haven't encountered any problems on Amiga.
> I'll look into this.

I guess link order (arch before drivers) saves you here but it is not
very clean.

>
> >> > -static int __devexit q40kbd_remove(struct platform_device *dev)
> >> > +static int __devexit q40kbd_remove(struct platform_device *pdev)
> >> >  {
> >> > -       serio_unregister_port(q40kbd_port);
> >> > +       struct q40kbd *q40kbd = platform_get_drvdata(pdev);
> >> > +
> >> > +       free_irq(Q40_IRQ_KEYBOARD, q40kbd);
> >> > +
> >> > +       serio_unregister_port(q40kbd->port);
> >>
> >> Should the unregister be done before freeing the IRQ, i.e. reverse
> >> order compared to probe?
> >
> > Unregister will most likely cause memory being freed; you don't want to
> > chance IRQ firing here.
> 
> And in the probe case that can't happen when request_irq() is called?

We explicitly do q40kbd_stop() before requesting IRQ. Frankly serio
will be stopped by the serio core (via serio->stop() which is
q40kbd_stop()) so freeing IRQ fisrt should be fine, it just was looking
scary. I'll revert the order and add a comment.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: q40kbd - convert driver to the split model

[Dmitry Torokhov]

On Wed, Jan 11, 2012 at 09:30:51AM -0800, Dmitry Torokhov wrote:
> On Wed, Jan 11, 2012 at 10:25:20AM +0100, Geert Uytterhoeven wrote:
> > On Wed, Jan 11, 2012 at 09:11, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Wed, Jan 11, 2012 at 08:55:54AM +0100, Geert Uytterhoeven wrote:
> > >> On Sat, Dec 31, 2011 at 02:11, Dmitry Torokhov
> > >> <dmitry.torokhov@gmail.com> wrote:
> > 
> > >> > +static struct platform_device q40_kbd_pdev = {
> > >> > +       .name   = "q40kbd",
> > >> > +       .id     = -1,
> > >> > +};
> > >> > +
> > >> > +static __init int q40_add_kbd_device(void)
> > >> > +{
> > >> > +       return platform_device_register(&q40_kbd_pdev);
> > >>
> > >> If you would use platform_device_register_simple(), you don't need the
> > >> q40_kbd_pdev above, reducing memory consumption on non-Q40 platforms.
> > >
> > > Isn't this file only compiled on q40 platforms?
> > 
> > No, m68k does support multi-platform kernels.
> 
> OK, I'll do dynamic platform device allocation.
> 
> > 
> > >> > +}
> > >> > +arch_initcall(q40_add_kbd_device);
> > >>
> > >> For the Amiga platform drivers, I used device_initcall().
> > >
> > > Won't it potentially race with initialization of q40kbd? It looks like
> > > module_initcall is the same as device_initcall() when module is compiled
> > > in. Given that q40kbd uses platform_dveice_probe() losing race might be
> > > fatal.
> > 
> > So far I haven't encountered any problems on Amiga.
> > I'll look into this.
> 
> I guess link order (arch before drivers) saves you here but it is not
> very clean.
> 
> >
> > >> > -static int __devexit q40kbd_remove(struct platform_device *dev)
> > >> > +static int __devexit q40kbd_remove(struct platform_device *pdev)
> > >> >  {
> > >> > -       serio_unregister_port(q40kbd_port);
> > >> > +       struct q40kbd *q40kbd = platform_get_drvdata(pdev);
> > >> > +
> > >> > +       free_irq(Q40_IRQ_KEYBOARD, q40kbd);
> > >> > +
> > >> > +       serio_unregister_port(q40kbd->port);
> > >>
> > >> Should the unregister be done before freeing the IRQ, i.e. reverse
> > >> order compared to probe?
> > >
> > > Unregister will most likely cause memory being freed; you don't want to
> > > chance IRQ firing here.
> > 
> > And in the probe case that can't happen when request_irq() is called?
> 
> We explicitly do q40kbd_stop() before requesting IRQ. Frankly serio
> will be stopped by the serio core (via serio->stop() which is
> q40kbd_stop()) so freeing IRQ fisrt should be fine, it just was looking
> scary. I'll revert the order and add a comment.
> 

OK, so here it is.

-- 
Dmitry


Input: q40kbd - convert driver to the split model

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Convert the driver to standard spilt model arch-specific code registers
platform device to which driver code can bind later.

Also request IRQ immediately upon binding to the device instead of doing
this when serio port is being opened.

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

 arch/m68k/q40/config.c       |    7 ++
 drivers/input/serio/q40kbd.c |  139 ++++++++++++++++++++++++------------------
 2 files changed, 86 insertions(+), 60 deletions(-)


diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
index ad10fec..c72a3ed 100644
--- a/arch/m68k/q40/config.c
+++ b/arch/m68k/q40/config.c
@@ -24,6 +24,7 @@
 #include <linux/rtc.h>
 #include <linux/vt_kern.h>
 #include <linux/bcd.h>
+#include <linux/platform_device.h>
 
 #include <asm/io.h>
 #include <asm/rtc.h>
@@ -329,3 +330,9 @@ static int q40_set_rtc_pll(struct rtc_pll_info *pll)
 	} else
 		return -EINVAL;
 }
+
+static __init int q40_add_kbd_device(void)
+{
+	return platform_device_register_simple("q40kbd", -1, NULL, 0);
+}
+arch_initcall(q40_add_kbd_device);
diff --git a/drivers/input/serio/q40kbd.c b/drivers/input/serio/q40kbd.c
index 5eb84b3..0c0df7f 100644
--- a/drivers/input/serio/q40kbd.c
+++ b/drivers/input/serio/q40kbd.c
@@ -44,26 +44,31 @@
 #include <asm/irq.h>
 #include <asm/q40ints.h>
 
+#define DRV_NAME	"q40kbd"
+
 MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
 MODULE_DESCRIPTION("Q40 PS/2 keyboard controller driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
 
-static DEFINE_SPINLOCK(q40kbd_lock);
-static struct serio *q40kbd_port;
-static struct platform_device *q40kbd_device;
+struct q40kbd {
+	struct serio *port;
+	spinlock_t lock;
+};
 
 static irqreturn_t q40kbd_interrupt(int irq, void *dev_id)
 {
+	struct q40kbd *q40kbd = dev_id;
 	unsigned long flags;
 
-	spin_lock_irqsave(&q40kbd_lock, flags);
+	spin_lock_irqsave(&q40kbd->lock, flags);
 
 	if (Q40_IRQ_KEYB_MASK & master_inb(INTERRUPT_REG))
-		serio_interrupt(q40kbd_port, master_inb(KEYCODE_REG), 0);
+		serio_interrupt(q40kbd->port, master_inb(KEYCODE_REG), 0);
 
 	master_outb(-1, KEYBOARD_UNLOCK_REG);
 
-	spin_unlock_irqrestore(&q40kbd_lock, flags);
+	spin_unlock_irqrestore(&q40kbd->lock, flags);
 
 	return IRQ_HANDLED;
 }
@@ -72,17 +77,23 @@ static irqreturn_t q40kbd_interrupt(int irq, void *dev_id)
  * q40kbd_flush() flushes all data that may be in the keyboard buffers
  */
 
-static void q40kbd_flush(void)
+static void q40kbd_flush(struct q40kbd *q40kbd)
 {
 	int maxread = 100;
 	unsigned long flags;
 
-	spin_lock_irqsave(&q40kbd_lock, flags);
+	spin_lock_irqsave(&q40kbd->lock, flags);
 
 	while (maxread-- && (Q40_IRQ_KEYB_MASK & master_inb(INTERRUPT_REG)))
 		master_inb(KEYCODE_REG);
 
-	spin_unlock_irqrestore(&q40kbd_lock, flags);
+	spin_unlock_irqrestore(&q40kbd->lock, flags);
+}
+
+static void q40kbd_stop(void)
+{
+	master_outb(0, KEY_IRQ_ENABLE_REG);
+	master_outb(-1, KEYBOARD_UNLOCK_REG);
 }
 
 /*
@@ -92,12 +103,9 @@ static void q40kbd_flush(void)
 
 static int q40kbd_open(struct serio *port)
 {
-	q40kbd_flush();
+	struct q40kbd *q40kbd = port->port_data;
 
-	if (request_irq(Q40_IRQ_KEYBOARD, q40kbd_interrupt, 0, "q40kbd", NULL)) {
-		printk(KERN_ERR "q40kbd.c: Can't get irq %d.\n", Q40_IRQ_KEYBOARD);
-		return -EBUSY;
-	}
+	q40kbd_flush(q40kbd);
 
 	/* off we go */
 	master_outb(-1, KEYBOARD_UNLOCK_REG);
@@ -108,36 +116,72 @@ static int q40kbd_open(struct serio *port)
 
 static void q40kbd_close(struct serio *port)
 {
-	master_outb(0, KEY_IRQ_ENABLE_REG);
-	master_outb(-1, KEYBOARD_UNLOCK_REG);
-	free_irq(Q40_IRQ_KEYBOARD, NULL);
+	struct q40kbd *q40kbd = port->port_data;
 
-	q40kbd_flush();
+	q40kbd_stop();
+	q40kbd_flush(q40kbd);
 }
 
-static int __devinit q40kbd_probe(struct platform_device *dev)
+static int __devinit q40kbd_probe(struct platform_device *pdev)
 {
-	q40kbd_port = kzalloc(sizeof(struct serio), GFP_KERNEL);
-	if (!q40kbd_port)
-		return -ENOMEM;
-
-	q40kbd_port->id.type	= SERIO_8042;
-	q40kbd_port->open	= q40kbd_open;
-	q40kbd_port->close	= q40kbd_close;
-	q40kbd_port->dev.parent	= &dev->dev;
-	strlcpy(q40kbd_port->name, "Q40 Kbd Port", sizeof(q40kbd_port->name));
-	strlcpy(q40kbd_port->phys, "Q40", sizeof(q40kbd_port->phys));
-
-	serio_register_port(q40kbd_port);
+	struct q40kbd *q40kbd;
+	struct serio *port;
+	int error;
+
+	q40kbd = kzalloc(sizeof(struct q40kbd), GFP_KERNEL);
+	port = kzalloc(sizeof(struct serio), GFP_KERNEL);
+	if (!q40kbd || !port) {
+		error = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	q40kbd->port = port;
+	spin_lock_init(&q40kbd->lock);
+
+	port->id.type = SERIO_8042;
+	port->open = q40kbd_open;
+	port->close = q40kbd_close;
+	port->port_data = q40kbd;
+	port->dev.parent = &pdev->dev;
+	strlcpy(port->name, "Q40 Kbd Port", sizeof(port->name));
+	strlcpy(port->phys, "Q40", sizeof(port->phys));
+
+	q40kbd_stop();
+
+	error = request_irq(Q40_IRQ_KEYBOARD, q40kbd_interrupt, 0,
+			    DRV_NAME, q40kbd);
+	if (error) {
+		dev_err(&pdev->dev, "Can't get irq %d.\n", Q40_IRQ_KEYBOARD);
+		goto err_free_mem;
+	}
+
+	serio_register_port(q40kbd->port);
+
+	platform_set_drvdata(pdev, q40kbd);
 	printk(KERN_INFO "serio: Q40 kbd registered\n");
 
 	return 0;
+
+err_free_mem:
+	kfree(port);
+	kfree(q40kbd);
+	return error;
 }
 
-static int __devexit q40kbd_remove(struct platform_device *dev)
+static int __devexit q40kbd_remove(struct platform_device *pdev)
 {
-	serio_unregister_port(q40kbd_port);
-
+	struct q40kbd *q40kbd = platform_get_drvdata(pdev);
+
+	/*
+	 * q40kbd_close() will be called as part of unregistering
+	 * and will ensure that IRQ is turned off, so it is safe
+	 * to unregister port first and free IRQ later.
+	 */
+	serio_unregister_port(q40kbd->port);
+	free_irq(Q40_IRQ_KEYBOARD, q40kbd);
+	kfree(q40kbd);
+
+	platform_set_drvdata(pdev, NULL);
 	return 0;
 }
 
@@ -146,41 +190,16 @@ static struct platform_driver q40kbd_driver = {
 		.name	= "q40kbd",
 		.owner	= THIS_MODULE,
 	},
-	.probe		= q40kbd_probe,
 	.remove		= __devexit_p(q40kbd_remove),
 };
 
 static int __init q40kbd_init(void)
 {
-	int error;
-
-	if (!MACH_IS_Q40)
-		return -ENODEV;
-
-	error = platform_driver_register(&q40kbd_driver);
-	if (error)
-		return error;
-
-	q40kbd_device = platform_device_alloc("q40kbd", -1);
-	if (!q40kbd_device)
-		goto err_unregister_driver;
-
-	error = platform_device_add(q40kbd_device);
-	if (error)
-		goto err_free_device;
-
-	return 0;
-
- err_free_device:
-	platform_device_put(q40kbd_device);
- err_unregister_driver:
-	platform_driver_unregister(&q40kbd_driver);
-	return error;
+	return platform_driver_probe(&q40kbd_driver, q40kbd_probe);
 }
 
 static void __exit q40kbd_exit(void)
 {
-	platform_device_unregister(q40kbd_device);
 	platform_driver_unregister(&q40kbd_driver);
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: q40kbd - convert driver to the split model

[Geert Uytterhoeven]

On Wed, 18 Jan 2012, Dmitry Torokhov wrote:
> Input: q40kbd - convert driver to the split model
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Convert the driver to standard spilt model arch-specific code registers
> platform device to which driver code can bind later.
> 
> Also request IRQ immediately upon binding to the device instead of doing
> this when serio port is being opened.
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  arch/m68k/q40/config.c       |    7 ++
>  drivers/input/serio/q40kbd.c |  139 ++++++++++++++++++++++++------------------
>  2 files changed, 86 insertions(+), 60 deletions(-)
> 
> 
> diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
> index ad10fec..c72a3ed 100644
> --- a/arch/m68k/q40/config.c
> +++ b/arch/m68k/q40/config.c
> @@ -24,6 +24,7 @@
>  #include <linux/rtc.h>
>  #include <linux/vt_kern.h>
>  #include <linux/bcd.h>
> +#include <linux/platform_device.h>
>  
>  #include <asm/io.h>
>  #include <asm/rtc.h>
> @@ -329,3 +330,9 @@ static int q40_set_rtc_pll(struct rtc_pll_info *pll)
>  	} else
>  		return -EINVAL;
>  }
> +
> +static __init int q40_add_kbd_device(void)
> +{
> +	return platform_device_register_simple("q40kbd", -1, NULL, 0);

arch/m68k/q40/config.c: In function ‘q40_add_kbd_device’:
arch/m68k/q40/config.c:336: warning: return makes integer from pointer without a cast

Either ignore the error and always return 0, or:

diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
index c72a3ed..be93648 100644
--- a/arch/m68k/q40/config.c
+++ b/arch/m68k/q40/config.c
@@ -333,6 +333,12 @@ static int q40_set_rtc_pll(struct rtc_pll_info *pll)
 
 static __init int q40_add_kbd_device(void)
 {
-	return platform_device_register_simple("q40kbd", -1, NULL, 0);
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_simple("q40kbd", -1, NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return 0;
 }
 arch_initcall(q40_add_kbd_device);

The rest is
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds