[PATCH 1/3] sony-laptop - remove private workqueue, use keventd instead

[PATCH 1/3] sony-laptop - remove private workqueue, use keventd instead

[Dmitry Torokhov]

If we reschedule work instead of having work function sleep for 10 msecs
between reads from kfifo we can safely use the main workqueue (keventd)
and not bother with creating driver-private one.

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

These are the same paches that were sent previously, just adjusted to the
new kfifo API.


 drivers/platform/x86/sony-laptop.c |   71 ++++++++++++++++++++----------------
 1 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 2896ca4..5883cc8 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -144,7 +144,6 @@ struct sony_laptop_input_s {
 	struct input_dev	*key_dev;
 	struct kfifo		fifo;
 	spinlock_t		fifo_lock;
-	struct workqueue_struct	*wq;
 };
 
 static struct sony_laptop_input_s sony_laptop_input = {
@@ -298,18 +297,28 @@ static int sony_laptop_input_keycode_map[] = {
 /* release buttons after a short delay if pressed */
 static void do_sony_laptop_release_key(struct work_struct *work)
 {
+	struct delayed_work *dwork =
+			container_of(work, struct delayed_work, work);
 	struct sony_laptop_keypress kp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sony_laptop_input.fifo_lock, flags);
 
-	while (kfifo_out_locked(&sony_laptop_input.fifo, (unsigned char *)&kp,
-			sizeof(kp), &sony_laptop_input.fifo_lock)
-			== sizeof(kp)) {
-		msleep(10);
+	if (kfifo_out(&sony_laptop_input.fifo,
+		      (unsigned char *)&kp, sizeof(kp)) == sizeof(kp)) {
 		input_report_key(kp.dev, kp.key, 0);
 		input_sync(kp.dev);
 	}
+
+	/* If there is something in the fifo schedule next release. */
+	if (kfifo_len(&sony_laptop_input.fifo) != 0)
+		schedule_delayed_work(dwork, msecs_to_jiffies(10));
+
+	spin_unlock_irqrestore(&sony_laptop_input.fifo_lock, flags);
 }
-static DECLARE_WORK(sony_laptop_release_key_work,
-		do_sony_laptop_release_key);
+
+static DECLARE_DELAYED_WORK(sony_laptop_release_key_work,
+			    do_sony_laptop_release_key);
 
 /* forward event to the input subsystem */
 static void sony_laptop_report_input_event(u8 event)
@@ -363,13 +372,13 @@ static void sony_laptop_report_input_event(u8 event)
 		/* we emit the scancode so we can always remap the key */
 		input_event(kp.dev, EV_MSC, MSC_SCAN, event);
 		input_sync(kp.dev);
-		kfifo_in_locked(&sony_laptop_input.fifo,
-			  (unsigned char *)&kp, sizeof(kp),
-			  &sony_laptop_input.fifo_lock);
 
-		if (!work_pending(&sony_laptop_release_key_work))
-			queue_work(sony_laptop_input.wq,
-					&sony_laptop_release_key_work);
+		/* schedule key release */
+		kfifo_in_locked(&sony_laptop_input.fifo,
+				(unsigned char *)&kp, sizeof(kp),
+				&sony_laptop_input.fifo_lock);
+		schedule_delayed_work(&sony_laptop_release_key_work,
+				      msecs_to_jiffies(10));
 	} else
 		dprintk("unknown input event %.2x\n", event);
 }
@@ -387,27 +396,18 @@ static int sony_laptop_setup_input(struct acpi_device *acpi_device)
 
 	/* kfifo */
 	spin_lock_init(&sony_laptop_input.fifo_lock);
-	error =
-	 kfifo_alloc(&sony_laptop_input.fifo, SONY_LAPTOP_BUF_SIZE, GFP_KERNEL);
+	error = kfifo_alloc(&sony_laptop_input.fifo,
+			    SONY_LAPTOP_BUF_SIZE, GFP_KERNEL);
 	if (error) {
 		printk(KERN_ERR DRV_PFX "kfifo_alloc failed\n");
 		goto err_dec_users;
 	}
 
-	/* init workqueue */
-	sony_laptop_input.wq = create_singlethread_workqueue("sony-laptop");
-	if (!sony_laptop_input.wq) {
-		printk(KERN_ERR DRV_PFX
-				"Unable to create workqueue.\n");
-		error = -ENXIO;
-		goto err_free_kfifo;
-	}
-
 	/* input keys */
 	key_dev = input_allocate_device();
 	if (!key_dev) {
 		error = -ENOMEM;
-		goto err_destroy_wq;
+		goto err_free_kfifo;
 	}
 
 	key_dev->name = "Sony Vaio Keys";
@@ -470,9 +470,6 @@ err_unregister_keydev:
 err_free_keydev:
 	input_free_device(key_dev);
 
-err_destroy_wq:
-	destroy_workqueue(sony_laptop_input.wq);
-
 err_free_kfifo:
 	kfifo_free(&sony_laptop_input.fifo);
 
@@ -483,12 +480,23 @@ err_dec_users:
 
 static void sony_laptop_remove_input(void)
 {
-	/* cleanup only after the last user has gone */
+	struct sony_laptop_keypress kp = { NULL };
+
+	/* Cleanup only after the last user has gone */
 	if (!atomic_dec_and_test(&sony_laptop_input.users))
 		return;
 
-	/* flush workqueue first */
-	flush_workqueue(sony_laptop_input.wq);
+	cancel_delayed_work_sync(&sony_laptop_release_key_work);
+
+	/*
+	 * Generate key-up events for remaining keys. Note that we don't
+	 * need locking since nobody is adding new events to the kfifo.
+	 */
+	while (kfifo_out(&sony_laptop_input.fifo,
+			 (unsigned char *)&kp, sizeof(kp)) == sizeof(kp)) {
+		input_report_key(kp.dev, kp.key, 0);
+		input_sync(kp.dev);
+	}
 
 	/* destroy input devs */
 	input_unregister_device(sony_laptop_input.key_dev);
@@ -499,7 +507,6 @@ static void sony_laptop_remove_input(void)
 		sony_laptop_input.jog_dev = NULL;
 	}
 
-	destroy_workqueue(sony_laptop_input.wq);
 	kfifo_free(&sony_laptop_input.fifo);
 }
 

[PATCH 2/3] sony-laptop - simplify keymap initialization

[Dmitry Torokhov]

Also use input_set_capability() helper instead of manipulating
bits directly.

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

 drivers/platform/x86/sony-laptop.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 5883cc8..a6b7616 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -416,18 +416,15 @@ static int sony_laptop_setup_input(struct acpi_device *acpi_device)
 	key_dev->dev.parent = &acpi_device->dev;
 
 	/* Initialize the Input Drivers: special keys */
-	set_bit(EV_KEY, key_dev->evbit);
-	set_bit(EV_MSC, key_dev->evbit);
-	set_bit(MSC_SCAN, key_dev->mscbit);
+	input_set_capability(key_dev, EV_MSC, MSC_SCAN);
+
+	__set_bit(EV_KEY, key_dev->evbit);
 	key_dev->keycodesize = sizeof(sony_laptop_input_keycode_map[0]);
 	key_dev->keycodemax = ARRAY_SIZE(sony_laptop_input_keycode_map);
 	key_dev->keycode = &sony_laptop_input_keycode_map;
-	for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++) {
-		if (sony_laptop_input_keycode_map[i] != KEY_RESERVED) {
-			set_bit(sony_laptop_input_keycode_map[i],
-				key_dev->keybit);
-		}
-	}
+	for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++)
+		__set_bit(sony_laptop_input_keycode_map[i], key_dev->keybit);
+	__clear_bit(KEY_RESERVED, key_dev->keybit);
 
 	error = input_register_device(key_dev);
 	if (error)
@@ -447,9 +444,8 @@ static int sony_laptop_setup_input(struct acpi_device *acpi_device)
 	jog_dev->id.vendor = PCI_VENDOR_ID_SONY;
 	key_dev->dev.parent = &acpi_device->dev;
 
-	jog_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
-	jog_dev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_MIDDLE);
-	jog_dev->relbit[0] = BIT_MASK(REL_WHEEL);
+	input_set_capability(jog_dev, EV_KEY, BTN_MIDDLE);
+	input_set_capability(jog_dev, EV_REL, REL_WHEEL);
 
 	error = input_register_device(jog_dev);
 	if (error)

Re: [PATCH 2/3] sony-laptop - simplify keymap initialization

[Henrique de Moraes Holschuh]

On Thu, 24 Dec 2009, Dmitry Torokhov wrote:
> +	for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++)
> +		__set_bit(sony_laptop_input_keycode_map[i], key_dev->keybit);
> +	__clear_bit(KEY_RESERVED, key_dev->keybit);

Maybe:

for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++) {
	if (sony_laptop_input_keycode_map[i] != KEY_RESERVED) {
		input_set_capability(key_dev, EV_KEY,
				sony_laptop_input_keycode_map[i]);
	}
}

would be better, as it means the driver doesn't poke into the inputdev
struct innards directly?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 2/3] sony-laptop - simplify keymap initialization

[Dmitry Torokhov]

On Thu, Dec 24, 2009 at 07:01:46PM -0200, Henrique de Moraes Holschuh wrote:
> On Thu, 24 Dec 2009, Dmitry Torokhov wrote:
> > +	for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++)
> > +		__set_bit(sony_laptop_input_keycode_map[i], key_dev->keybit);
> > +	__clear_bit(KEY_RESERVED, key_dev->keybit);
> 
> Maybe:
> 
> for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++) {
> 	if (sony_laptop_input_keycode_map[i] != KEY_RESERVED) {
> 		input_set_capability(key_dev, EV_KEY,
> 				sony_laptop_input_keycode_map[i]);
> 	}
> }
> 
> would be better, as it means the driver doesn't poke into the inputdev
> struct innards directly?
> 

Given that you have to still poke there to set up open, close, name,
phys, id and so on and so forth I think the original is fine and that is
what most of keyboard-like drivers do.

The reason I came up with input_set_capability is because gpio-keys
wanted to set up siwtches and keys based on platform data so doing;

	for(...)
		input_set_capability(dev, pdata->type, pdata->code);

looked nice.

-- 
Dmitry

Re: [PATCH 2/3] sony-laptop - simplify keymap initialization

[Henrique de Moraes Holschuh]

On Thu, 24 Dec 2009, Dmitry Torokhov wrote:
> On Thu, Dec 24, 2009 at 07:01:46PM -0200, Henrique de Moraes Holschuh wrote:
> > On Thu, 24 Dec 2009, Dmitry Torokhov wrote:
> > > +	for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++)
> > > +		__set_bit(sony_laptop_input_keycode_map[i], key_dev->keybit);
> > > +	__clear_bit(KEY_RESERVED, key_dev->keybit);
> > 
> > Maybe:
> > 
> > for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++) {
> > 	if (sony_laptop_input_keycode_map[i] != KEY_RESERVED) {
> > 		input_set_capability(key_dev, EV_KEY,
> > 				sony_laptop_input_keycode_map[i]);
> > 	}
> > }
> > 
> > would be better, as it means the driver doesn't poke into the inputdev
> > struct innards directly?
> 
> Given that you have to still poke there to set up open, close, name,
> phys, id and so on and so forth I think the original is fine and that is
> what most of keyboard-like drivers do.

Never liked that much, either :-)

> The reason I came up with input_set_capability is because gpio-keys
> wanted to set up siwtches and keys based on platform data so doing;
> 
> 	for(...)
> 		input_set_capability(dev, pdata->type, pdata->code);
> 
> looked nice.

I see.  Well, it is a matter of differing tastes I think (none of them bad,
just different).

That said, is KEY_RESERVED bit left set a common error?  If it is, maybe it
make sense to __clear_bit it inside of input_register_device() just in case?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 2/3] sony-laptop - simplify keymap initialization

[Dmitry Torokhov]

On Fri, Dec 25, 2009 at 11:38:12AM -0200, Henrique de Moraes Holschuh wrote:
> On Thu, 24 Dec 2009, Dmitry Torokhov wrote:
> > On Thu, Dec 24, 2009 at 07:01:46PM -0200, Henrique de Moraes Holschuh wrote:
> > > On Thu, 24 Dec 2009, Dmitry Torokhov wrote:
> > > > +	for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++)
> > > > +		__set_bit(sony_laptop_input_keycode_map[i], key_dev->keybit);
> > > > +	__clear_bit(KEY_RESERVED, key_dev->keybit);
> > > 
> > > Maybe:
> > > 
> > > for (i = 0; i < ARRAY_SIZE(sony_laptop_input_keycode_map); i++) {
> > > 	if (sony_laptop_input_keycode_map[i] != KEY_RESERVED) {
> > > 		input_set_capability(key_dev, EV_KEY,
> > > 				sony_laptop_input_keycode_map[i]);
> > > 	}
> > > }
> > > 
> > > would be better, as it means the driver doesn't poke into the inputdev
> > > struct innards directly?
> > 
> > Given that you have to still poke there to set up open, close, name,
> > phys, id and so on and so forth I think the original is fine and that is
> > what most of keyboard-like drivers do.
> 
> Never liked that much, either :-)
> 
> > The reason I came up with input_set_capability is because gpio-keys
> > wanted to set up siwtches and keys based on platform data so doing;
> > 
> > 	for(...)
> > 		input_set_capability(dev, pdata->type, pdata->code);
> > 
> > looked nice.
> 
> I see.  Well, it is a matter of differing tastes I think (none of them bad,
> just different).
> 
> That said, is KEY_RESERVED bit left set a common error?  If it is, maybe it
> make sense to __clear_bit it inside of input_register_device() just in case?
> 

Something like below?

-- 
Dmitry


Input: automatically reset KEY_RESERVED bit for all input devices

KEY_RESERVED is not supposed to be reported to userspace but rather to
mark unused entries in keymaps.

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

 drivers/input/input.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)


diff --git a/drivers/input/input.c b/drivers/input/input.c
index ab06071..910d7be 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -613,12 +613,12 @@ static int input_default_setkeycode(struct input_dev *dev,
 		}
 	}
 
-	clear_bit(old_keycode, dev->keybit);
-	set_bit(keycode, dev->keybit);
+	__clear_bit(old_keycode, dev->keybit);
+	__set_bit(keycode, dev->keybit);
 
 	for (i = 0; i < dev->keycodemax; i++) {
 		if (input_fetch_keycode(dev, i) == old_keycode) {
-			set_bit(old_keycode, dev->keybit);
+			__set_bit(old_keycode, dev->keybit);
 			break; /* Setting the bit twice is useless, so break */
 		}
 	}
@@ -676,6 +676,9 @@ int input_set_keycode(struct input_dev *dev, int scancode, int keycode)
 	if (retval)
 		goto out;
 
+	/* Make sure KEY_RESERVED did not get enabled. */
+	__clear_bit(KEY_RESERVED, dev->keybit);
+
 	/*
 	 * Simulate keyup event if keycode is not present
 	 * in the keymap anymore
@@ -1513,13 +1516,16 @@ int input_register_device(struct input_dev *dev)
 	const char *path;
 	int error;
 
+	/* Every input device generates EV_SYN/SYN_REPORT events. */
 	__set_bit(EV_SYN, dev->evbit);
 
+	/* KEY_RESERVED is not supposed to be transmitted to userspace. */
+	__clear_bit(KEY_RESERVED, dev->keybit);
+
 	/*
 	 * If delay and period are pre-set by the driver, then autorepeating
 	 * is handled by the driver itself and we don't do it in input.c.
 	 */
-
 	init_timer(&dev->timer);
 	if (!dev->rep[REP_DELAY] && !dev->rep[REP_PERIOD]) {
 		dev->timer.data = (long) dev;

Re: [PATCH 2/3] sony-laptop - simplify keymap initialization

[Henrique de Moraes Holschuh]

On Tue, 29 Dec 2009, Dmitry Torokhov wrote:
> > That said, is KEY_RESERVED bit left set a common error?  If it is, maybe it
> > make sense to __clear_bit it inside of input_register_device() just in case?
> > 
> 
> Something like below?

Exactly.  Although it looks like you have some unrelated atomic versus
non-atomic set/clear_bit cleanups in it that might be better off in a comit
of their own?

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

[PATCH 3/3] sony-laptop - switch from workqueue to a timer

[Dmitry Torokhov]

The function that is executing in workqueue context does not need
to sleep so let's switch to a timer which is more lightweight.

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

 drivers/platform/x86/sony-laptop.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index a6b7616..8f0455b 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -144,6 +144,7 @@ struct sony_laptop_input_s {
 	struct input_dev	*key_dev;
 	struct kfifo		fifo;
 	spinlock_t		fifo_lock;
+	struct timer_list	release_key_timer;
 };
 
 static struct sony_laptop_input_s sony_laptop_input = {
@@ -295,10 +296,8 @@ static int sony_laptop_input_keycode_map[] = {
 };
 
 /* release buttons after a short delay if pressed */
-static void do_sony_laptop_release_key(struct work_struct *work)
+static void do_sony_laptop_release_key(unsigned long unused)
 {
-	struct delayed_work *dwork =
-			container_of(work, struct delayed_work, work);
 	struct sony_laptop_keypress kp;
 	unsigned long flags;
 
@@ -312,14 +311,12 @@ static void do_sony_laptop_release_key(struct work_struct *work)
 
 	/* If there is something in the fifo schedule next release. */
 	if (kfifo_len(&sony_laptop_input.fifo) != 0)
-		schedule_delayed_work(dwork, msecs_to_jiffies(10));
+		mod_timer(&sony_laptop_input.release_key_timer,
+			  jiffies + msecs_to_jiffies(10));
 
 	spin_unlock_irqrestore(&sony_laptop_input.fifo_lock, flags);
 }
 
-static DECLARE_DELAYED_WORK(sony_laptop_release_key_work,
-			    do_sony_laptop_release_key);
-
 /* forward event to the input subsystem */
 static void sony_laptop_report_input_event(u8 event)
 {
@@ -377,8 +374,8 @@ static void sony_laptop_report_input_event(u8 event)
 		kfifo_in_locked(&sony_laptop_input.fifo,
 				(unsigned char *)&kp, sizeof(kp),
 				&sony_laptop_input.fifo_lock);
-		schedule_delayed_work(&sony_laptop_release_key_work,
-				      msecs_to_jiffies(10));
+		mod_timer(&sony_laptop_input.release_key_timer,
+			  jiffies + msecs_to_jiffies(10));
 	} else
 		dprintk("unknown input event %.2x\n", event);
 }
@@ -403,6 +400,9 @@ static int sony_laptop_setup_input(struct acpi_device *acpi_device)
 		goto err_dec_users;
 	}
 
+	setup_timer(&sony_laptop_input.release_key_timer,
+		    do_sony_laptop_release_key, 0);
+
 	/* input keys */
 	key_dev = input_allocate_device();
 	if (!key_dev) {
@@ -482,7 +482,7 @@ static void sony_laptop_remove_input(void)
 	if (!atomic_dec_and_test(&sony_laptop_input.users))
 		return;
 
-	cancel_delayed_work_sync(&sony_laptop_release_key_work);
+	del_timer_sync(&sony_laptop_input.release_key_timer);
 
 	/*
 	 * Generate key-up events for remaining keys. Note that we don't

Re: [PATCH 1/3] sony-laptop - remove private workqueue, use keventd instead

[Len Brown]

1-3 applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 1/3] sony-laptop - remove private workqueue, use keventd instead

[Dmitry Torokhov]

On Thu, Dec 24, 2009 at 02:49:06PM -0500, Len Brown wrote:
> 1-3 applied to acpi-test
> 

Umm, can't seem to fnd them there, is it still in your private tree by
any chance?

Thanks.

-- 
Dmitry