[PATCH 1/3] auxdisplay: ht16k33: do not try to free fbdev

[PATCH 1/3] auxdisplay: ht16k33: do not try to free fbdev

[Dmitry Torokhov]

'fbdev' is allocated as part of larger ht16k33_priv structure; trying to
free it will cause troubles.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Note that the patches have not been tried on a real hardware.

I am pretty confident in #1 and #3, but #2 is larger and it would be great
if someone with hardware tried running it before it gets applied.

Thanks!

 drivers/auxdisplay/ht16k33.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index eeb323f56c07..f2f304b3f061 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -378,7 +378,7 @@ static int ht16k33_probe(struct i2c_client *client,
 	fbdev->buffer = (unsigned char *) get_zeroed_page(GFP_KERNEL);
 	if (!fbdev->buffer) {
 		err = -ENOMEM;
-		goto err_free_fbdev;
+		goto err_destroy_wq;
 	}
 
 	fbdev->cache = devm_kmalloc(&client->dev, HT16K33_FB_SIZE, GFP_KERNEL);
@@ -510,8 +510,6 @@ static int ht16k33_probe(struct i2c_client *client,
 	framebuffer_release(fbdev->info);
 err_fbdev_buffer:
 	free_page((unsigned long) fbdev->buffer);
-err_free_fbdev:
-	kfree(fbdev);
 err_destroy_wq:
 	destroy_workqueue(priv->workqueue);
 
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 2/3] auxdisplay: ht16k33: rework input device initialization

[Dmitry Torokhov]

This patch fixes following issues in input device (keypad) handling:

- requesting IRQ before allocating and initializing parts of the device
  that can be referenced from IRQ handler is racy, even if we try to
  disable interrupt after requesting it. Let's move allocations around
  so that everything is ready by the time we request IRQ.

- using threaded interrupt handler to schedule a work item it sub-optimal.
  Disabling and then re-enabling interrupts in work item and in open/close
  methods is prone to races and exactly the reason theraded interrupts were
  introduced. Let's use the infrastructure properly and keep scanning the
  matrix array in IRQ thread, stopping when there are no keys, or when told
  to do so.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/auxdisplay/ht16k33.c | 298 +++++++++++++++++++++----------------------
 1 file changed, 145 insertions(+), 153 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index f2f304b3f061..b2fbe68c7fa9 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -56,14 +56,16 @@
 #define HT16K33_FB_SIZE		(HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW)
 
 struct ht16k33_keypad {
+	struct i2c_client *client;
 	struct input_dev *dev;
-	spinlock_t lock;
-	struct delayed_work work;
 	uint32_t cols;
 	uint32_t rows;
 	uint32_t row_shift;
 	uint32_t debounce_ms;
 	uint16_t last_key_state[HT16K33_MATRIX_KEYPAD_MAX_COLS];
+
+	wait_queue_head_t wait;
+	bool stopped;
 };
 
 struct ht16k33_fbdev {
@@ -128,14 +130,6 @@ static void ht16k33_fb_queue(struct ht16k33_priv *priv)
 		msecs_to_jiffies(HZ / fbdev->refresh_rate));
 }
 
-static void ht16k33_keypad_queue(struct ht16k33_priv *priv)
-{
-	struct ht16k33_keypad *keypad = &priv->keypad;
-
-	queue_delayed_work(priv->workqueue, &keypad->work,
-		msecs_to_jiffies(keypad->debounce_ms));
-}
-
 /*
  * This gets the fb data from cache and copies it to ht16k33 display RAM
  */
@@ -182,32 +176,6 @@ static void ht16k33_fb_update(struct work_struct *work)
 	ht16k33_fb_queue(priv);
 }
 
-static int ht16k33_keypad_start(struct input_dev *dev)
-{
-	struct ht16k33_priv *priv = input_get_drvdata(dev);
-	struct ht16k33_keypad *keypad = &priv->keypad;
-
-	/*
-	 * Schedule an immediate key scan to capture current key state;
-	 * columns will be activated and IRQs be enabled after the scan.
-	 */
-	queue_delayed_work(priv->workqueue, &keypad->work, 0);
-	return 0;
-}
-
-static void ht16k33_keypad_stop(struct input_dev *dev)
-{
-	struct ht16k33_priv *priv = input_get_drvdata(dev);
-	struct ht16k33_keypad *keypad = &priv->keypad;
-
-	cancel_delayed_work(&keypad->work);
-	/*
-	 * ht16k33_keypad_scan() will leave IRQs enabled;
-	 * we should disable them now.
-	 */
-	disable_irq_nosync(priv->client->irq);
-}
-
 static int ht16k33_initialize(struct ht16k33_priv *priv)
 {
 	uint8_t byte;
@@ -233,61 +201,6 @@ static int ht16k33_initialize(struct ht16k33_priv *priv)
 	return i2c_smbus_write_byte(priv->client, byte);
 }
 
-/*
- * This gets the keys from keypad and reports it to input subsystem
- */
-static void ht16k33_keypad_scan(struct work_struct *work)
-{
-	struct ht16k33_keypad *keypad =
-		container_of(work, struct ht16k33_keypad, work.work);
-	struct ht16k33_priv *priv =
-		container_of(keypad, struct ht16k33_priv, keypad);
-	const unsigned short *keycodes = keypad->dev->keycode;
-	uint16_t bits_changed, new_state[HT16K33_MATRIX_KEYPAD_MAX_COLS];
-	uint8_t data[HT16K33_MATRIX_KEYPAD_MAX_COLS * 2];
-	int row, col, code;
-	bool reschedule = false;
-
-	if (i2c_smbus_read_i2c_block_data(priv->client, 0x40, 6, data) != 6) {
-		dev_err(&priv->client->dev, "Failed to read key data\n");
-		goto end;
-	}
-
-	for (col = 0; col < keypad->cols; col++) {
-		new_state[col] = (data[col * 2 + 1] << 8) | data[col * 2];
-		if (new_state[col])
-			reschedule = true;
-		bits_changed = keypad->last_key_state[col] ^ new_state[col];
-
-		while (bits_changed) {
-			row = ffs(bits_changed) - 1;
-			code = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
-			input_event(keypad->dev, EV_MSC, MSC_SCAN, code);
-			input_report_key(keypad->dev, keycodes[code],
-					 new_state[col] & BIT(row));
-			bits_changed &= ~BIT(row);
-		}
-	}
-	input_sync(keypad->dev);
-	memcpy(keypad->last_key_state, new_state, sizeof(new_state));
-
-end:
-	if (reschedule)
-		ht16k33_keypad_queue(priv);
-	else
-		enable_irq(priv->client->irq);
-}
-
-static irqreturn_t ht16k33_irq_thread(int irq, void *dev)
-{
-	struct ht16k33_priv *priv = dev;
-
-	disable_irq_nosync(priv->client->irq);
-	ht16k33_keypad_queue(priv);
-
-	return IRQ_HANDLED;
-}
-
 static int ht16k33_bl_update_status(struct backlight_device *bl)
 {
 	int brightness = bl->props.brightness;
@@ -334,15 +247,152 @@ static struct fb_ops ht16k33_fb_ops = {
 	.fb_mmap = ht16k33_mmap,
 };
 
+/*
+ * This gets the keys from keypad and reports it to input subsystem.
+ * Returns true if a key is pressed.
+ */
+static bool ht16k33_keypad_scan(struct ht16k33_keypad *keypad)
+{
+	const unsigned short *keycodes = keypad->dev->keycode;
+	u16 new_state[HT16K33_MATRIX_KEYPAD_MAX_COLS];
+	u8 data[HT16K33_MATRIX_KEYPAD_MAX_COLS * 2];
+	unsigned long bits_changed;
+	int row, col, code;
+	bool pressed = false;
+
+	if (i2c_smbus_read_i2c_block_data(keypad->client, 0x40, 6, data) != 6) {
+		dev_err(&keypad->client->dev, "Failed to read key data\n");
+		return false;
+	}
+
+	for (col = 0; col < keypad->cols; col++) {
+		new_state[col] = (data[col * 2 + 1] << 8) | data[col * 2];
+		if (new_state[col])
+			pressed = true;
+		bits_changed = keypad->last_key_state[col] ^ new_state[col];
+
+		for_each_set_bit(row, &bits_changed, BITS_PER_LONG) {
+			code = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
+			input_event(keypad->dev, EV_MSC, MSC_SCAN, code);
+			input_report_key(keypad->dev, keycodes[code],
+					 new_state[col] & BIT(row));
+		}
+	}
+	input_sync(keypad->dev);
+	memcpy(keypad->last_key_state, new_state, sizeof(new_state));
+
+	return pressed;
+}
+
+static irqreturn_t ht16k33_keypad_irq_thread(int irq, void *dev)
+{
+	struct ht16k33_keypad *keypad = dev;
+
+	do {
+		wait_event_timeout(keypad->wait, keypad->stopped,
+				    msecs_to_jiffies(keypad->debounce_ms));
+		if (keypad->stopped)
+			break;
+	} while (ht16k33_keypad_scan(keypad));
+
+	return IRQ_HANDLED;
+}
+
+static int ht16k33_keypad_start(struct input_dev *dev)
+{
+	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
+
+	keypad->stopped = false;
+	mb();
+	enable_irq(keypad->client->irq);
+
+	return 0;
+}
+
+static void ht16k33_keypad_stop(struct input_dev *dev)
+{
+	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
+
+	keypad->stopped = true;
+	mb();
+	wake_up(&keypad->wait);
+	disable_irq(keypad->client->irq);
+}
+
+static int ht16k33_keypad_probe(struct i2c_client *client,
+				struct ht16k33_keypad *keypad)
+{
+	struct device_node *node = client->dev.of_node;
+	u32 rows = HT16K33_MATRIX_KEYPAD_MAX_ROWS;
+	u32 cols = HT16K33_MATRIX_KEYPAD_MAX_COLS;
+	int err;
+
+	keypad->client = client;
+	init_waitqueue_head(&keypad->wait);
+
+	keypad->dev = devm_input_allocate_device(&client->dev);
+	if (!keypad->dev)
+		return -ENOMEM;
+
+	input_set_drvdata(keypad->dev, keypad);
+
+	keypad->dev->name = DRIVER_NAME"-keypad";
+	keypad->dev->id.bustype = BUS_I2C;
+	keypad->dev->open = ht16k33_keypad_start;
+	keypad->dev->close = ht16k33_keypad_stop;
+
+	if (!of_get_property(node, "linux,no-autorepeat", NULL))
+		__set_bit(EV_REP, keypad->dev->evbit);
+
+	err = of_property_read_u32(node, "debounce-delay-ms",
+				   &keypad->debounce_ms);
+	if (err) {
+		dev_err(&client->dev, "key debounce delay not specified\n");
+		return err;
+	}
+
+	err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols);
+	if (err)
+		return err;
+
+	keypad->rows = rows;
+	keypad->cols = cols;
+	keypad->row_shift = get_count_order(cols);
+
+	err = matrix_keypad_build_keymap(NULL, NULL, rows, cols, NULL,
+					 keypad->dev);
+	if (err) {
+		dev_err(&client->dev, "failed to build keymap\n");
+		return err;
+	}
+
+	err = devm_request_threaded_irq(&client->dev, client->irq,
+					NULL, ht16k33_keypad_irq_thread,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					DRIVER_NAME, keypad);
+	if (err) {
+		dev_err(&client->dev, "irq request failed %d, error %d\n",
+			client->irq, err);
+		return err;
+	}
+
+	ht16k33_keypad_stop(keypad->dev);
+
+	err = input_register_device(keypad->dev);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int ht16k33_probe(struct i2c_client *client,
 				  const struct i2c_device_id *id)
 {
 	int err;
-	uint32_t rows, cols, dft_brightness;
+	uint32_t dft_brightness;
 	struct backlight_device *bl;
 	struct backlight_properties bl_props;
 	struct ht16k33_priv *priv;
-	struct ht16k33_keypad *keypad;
 	struct ht16k33_fbdev *fbdev;
 	struct device_node *node = client->dev.of_node;
 
@@ -363,7 +413,6 @@ static int ht16k33_probe(struct i2c_client *client,
 	priv->client = client;
 	i2c_set_clientdata(client, priv);
 	fbdev = &priv->fbdev;
-	keypad = &priv->keypad;
 
 	priv->workqueue = create_singlethread_workqueue(DRIVER_NAME "-wq");
 	if (priv->workqueue == NULL)
@@ -415,59 +464,7 @@ static int ht16k33_probe(struct i2c_client *client,
 	if (err)
 		goto err_fbdev_info;
 
-	/* Keypad */
-	keypad->dev = devm_input_allocate_device(&client->dev);
-	if (!keypad->dev) {
-		err = -ENOMEM;
-		goto err_fbdev_unregister;
-	}
-
-	keypad->dev->name = DRIVER_NAME"-keypad";
-	keypad->dev->id.bustype = BUS_I2C;
-	keypad->dev->open = ht16k33_keypad_start;
-	keypad->dev->close = ht16k33_keypad_stop;
-
-	if (!of_get_property(node, "linux,no-autorepeat", NULL))
-		__set_bit(EV_REP, keypad->dev->evbit);
-
-	err = of_property_read_u32(node, "debounce-delay-ms",
-				   &keypad->debounce_ms);
-	if (err) {
-		dev_err(&client->dev, "key debounce delay not specified\n");
-		goto err_fbdev_unregister;
-	}
-
-	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
-					ht16k33_irq_thread,
-					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-					DRIVER_NAME, priv);
-	if (err) {
-		dev_err(&client->dev, "irq request failed %d, error %d\n",
-			client->irq, err);
-		goto err_fbdev_unregister;
-	}
-
-	disable_irq_nosync(client->irq);
-	rows = HT16K33_MATRIX_KEYPAD_MAX_ROWS;
-	cols = HT16K33_MATRIX_KEYPAD_MAX_COLS;
-	err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols);
-	if (err)
-		goto err_fbdev_unregister;
-
-	err = matrix_keypad_build_keymap(NULL, NULL, rows, cols, NULL,
-					 keypad->dev);
-	if (err) {
-		dev_err(&client->dev, "failed to build keymap\n");
-		goto err_fbdev_unregister;
-	}
-
-	input_set_drvdata(keypad->dev, priv);
-	keypad->rows = rows;
-	keypad->cols = cols;
-	keypad->row_shift = get_count_order(cols);
-	INIT_DELAYED_WORK(&keypad->work, ht16k33_keypad_scan);
-
-	err = input_register_device(keypad->dev);
+	err = ht16k33_keypad_probe(client, &priv->keypad);
 	if (err)
 		goto err_fbdev_unregister;
 
@@ -482,7 +479,7 @@ static int ht16k33_probe(struct i2c_client *client,
 	if (IS_ERR(bl)) {
 		dev_err(&client->dev, "failed to register backlight\n");
 		err = PTR_ERR(bl);
-		goto err_keypad_unregister;
+		goto err_fbdev_unregister;
 	}
 
 	err = of_property_read_u32(node, "default-brightness-level",
@@ -502,8 +499,6 @@ static int ht16k33_probe(struct i2c_client *client,
 	ht16k33_fb_queue(priv);
 	return 0;
 
-err_keypad_unregister:
-	input_unregister_device(keypad->dev);
 err_fbdev_unregister:
 	unregister_framebuffer(fbdev->info);
 err_fbdev_info:
@@ -519,11 +514,8 @@ static int ht16k33_probe(struct i2c_client *client,
 static int ht16k33_remove(struct i2c_client *client)
 {
 	struct ht16k33_priv *priv = i2c_get_clientdata(client);
-	struct ht16k33_keypad *keypad = &priv->keypad;
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 
-	ht16k33_keypad_stop(keypad->dev);
-
 	cancel_delayed_work(&fbdev->work);
 	unregister_framebuffer(fbdev->info);
 	framebuffer_release(fbdev->info);
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 3/3] auxdisplay: ht16k33: remove private workqueue

[Dmitry Torokhov]

There is no need for the driver to user private workqueue, standard system
workqueue should suffice as they going to use the same worker pool anyway.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/auxdisplay/ht16k33.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index b2fbe68c7fa9..533886a59cec 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -80,7 +80,6 @@ struct ht16k33_priv {
 	struct i2c_client *client;
 	struct ht16k33_keypad keypad;
 	struct ht16k33_fbdev fbdev;
-	struct workqueue_struct *workqueue;
 };
 
 static struct fb_fix_screeninfo ht16k33_fb_fix = {
@@ -126,8 +125,8 @@ static void ht16k33_fb_queue(struct ht16k33_priv *priv)
 {
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 
-	queue_delayed_work(priv->workqueue, &fbdev->work,
-		msecs_to_jiffies(HZ / fbdev->refresh_rate));
+	schedule_delayed_work(&fbdev->work,
+			      msecs_to_jiffies(HZ / fbdev->refresh_rate));
 }
 
 /*
@@ -414,21 +413,15 @@ static int ht16k33_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, priv);
 	fbdev = &priv->fbdev;
 
-	priv->workqueue = create_singlethread_workqueue(DRIVER_NAME "-wq");
-	if (priv->workqueue == NULL)
-		return -ENOMEM;
-
 	err = ht16k33_initialize(priv);
 	if (err)
-		goto err_destroy_wq;
+		return err;
 
 	/* Framebuffer (2 bytes per column) */
 	BUILD_BUG_ON(PAGE_SIZE < HT16K33_FB_SIZE);
 	fbdev->buffer = (unsigned char *) get_zeroed_page(GFP_KERNEL);
-	if (!fbdev->buffer) {
-		err = -ENOMEM;
-		goto err_destroy_wq;
-	}
+	if (!fbdev->buffer)
+		return -ENOMEM;
 
 	fbdev->cache = devm_kmalloc(&client->dev, HT16K33_FB_SIZE, GFP_KERNEL);
 	if (!fbdev->cache) {
@@ -505,8 +498,6 @@ static int ht16k33_probe(struct i2c_client *client,
 	framebuffer_release(fbdev->info);
 err_fbdev_buffer:
 	free_page((unsigned long) fbdev->buffer);
-err_destroy_wq:
-	destroy_workqueue(priv->workqueue);
 
 	return err;
 }
@@ -521,7 +512,6 @@ static int ht16k33_remove(struct i2c_client *client)
 	framebuffer_release(fbdev->info);
 	free_page((unsigned long) fbdev->buffer);
 
-	destroy_workqueue(priv->workqueue);
 	return 0;
 }
 
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH 1/3] auxdisplay: ht16k33: do not try to free fbdev

[Robin van der Gracht]

Hello Dmitry,

Thank you for submitting your changes. I was out of office last week
and I'll try to test and review your changes on my hardware this week.

Best regards,
Robin van der Gracht

Re: [PATCH 1/3] auxdisplay: ht16k33: do not try to free fbdev

[Robin van der Gracht]

On Tue, 31 Jan 2017 12:54:36 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> 'fbdev' is allocated as part of larger ht16k33_priv structure; trying to
> free it will cause troubles.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> Note that the patches have not been tried on a real hardware.
> 
> I am pretty confident in #1 and #3, but #2 is larger and it would be great
> if someone with hardware tried running it before it gets applied.
> 
> Thanks!
> 
>  drivers/auxdisplay/ht16k33.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index eeb323f56c07..f2f304b3f061 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -378,7 +378,7 @@ static int ht16k33_probe(struct i2c_client *client,
>  	fbdev->buffer = (unsigned char *) get_zeroed_page(GFP_KERNEL);
>  	if (!fbdev->buffer) {
>  		err = -ENOMEM;
> -		goto err_free_fbdev;
> +		goto err_destroy_wq;
>  	}
>  
>  	fbdev->cache = devm_kmalloc(&client->dev, HT16K33_FB_SIZE, GFP_KERNEL);
> @@ -510,8 +510,6 @@ static int ht16k33_probe(struct i2c_client *client,
>  	framebuffer_release(fbdev->info);
>  err_fbdev_buffer:
>  	free_page((unsigned long) fbdev->buffer);
> -err_free_fbdev:
> -	kfree(fbdev);
>  err_destroy_wq:
>  	destroy_workqueue(priv->workqueue);
>  

Acked-by: Robin van der Gracht <robin@protonic.nl>

Re: [PATCH 2/3] auxdisplay: ht16k33: rework input device initialization

[Robin van der Gracht]

On Tue, 31 Jan 2017 12:54:37 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

...

> +
> +static irqreturn_t ht16k33_keypad_irq_thread(int irq, void *dev)
> +{
> +	struct ht16k33_keypad *keypad = dev;
> +
> +	do {
> +		wait_event_timeout(keypad->wait, keypad->stopped,
> +				    msecs_to_jiffies(keypad->debounce_ms));
> +		if (keypad->stopped)
> +			break;
> +	} while (ht16k33_keypad_scan(keypad));

I like how you worked this one out! Its way cleaner and simpler.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ht16k33_keypad_start(struct input_dev *dev)
> +{
> +	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
> +
> +	keypad->stopped = false;
> +	mb();
> +	enable_irq(keypad->client->irq);
> +
> +	return 0;
> +}
> +
> +static void ht16k33_keypad_stop(struct input_dev *dev)
> +{
> +	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
> +
> +	keypad->stopped = true;
> +	mb();
> +	wake_up(&keypad->wait);
> +	disable_irq(keypad->client->irq);
> +}

Nice!

> +
> +static int ht16k33_keypad_probe(struct i2c_client *client,
> +				struct ht16k33_keypad *keypad)
> +{

...

> +
> +	err = devm_request_threaded_irq(&client->dev, client->irq,
> +					NULL, ht16k33_keypad_irq_thread,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					DRIVER_NAME, keypad);

I believe the interrupt trigger should be switched to IRQF_TRIGGER_HIGH
combined with IRQF_ONESHOT, for this new setup. This is causing a race
condition.

In the previous situation a 'key data' read was triggered by
ht16k33_keypad_start(). This cleared the IRQ and gave a (somewhat racy)
known state. Now, when the IRQ line is HIGH during probe keyscan
wont work. 

Also, when ht16k33_keypad_stop() is run while the irq thread is waiting
for debounce, the scan will be skipped and irq will never be cleared.
This also breaks keyscan.

Regards,
Robin

Re: [PATCH 3/3] auxdisplay: ht16k33: remove private workqueue

[Robin van der Gracht]

On Tue, 31 Jan 2017 12:54:38 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> There is no need for the driver to user private workqueue, standard system
> workqueue should suffice as they going to use the same worker pool anyway.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/auxdisplay/ht16k33.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index b2fbe68c7fa9..533886a59cec 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -80,7 +80,6 @@ struct ht16k33_priv {
>  	struct i2c_client *client;
>  	struct ht16k33_keypad keypad;
>  	struct ht16k33_fbdev fbdev;
> -	struct workqueue_struct *workqueue;
>  };
>  
>  static struct fb_fix_screeninfo ht16k33_fb_fix = {
> @@ -126,8 +125,8 @@ static void ht16k33_fb_queue(struct ht16k33_priv *priv)
>  {
>  	struct ht16k33_fbdev *fbdev = &priv->fbdev;
>  
> -	queue_delayed_work(priv->workqueue, &fbdev->work,
> -		msecs_to_jiffies(HZ / fbdev->refresh_rate));
> +	schedule_delayed_work(&fbdev->work,
> +			      msecs_to_jiffies(HZ / fbdev->refresh_rate));
>  }
>  
>  /*
> @@ -414,21 +413,15 @@ static int ht16k33_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, priv);
>  	fbdev = &priv->fbdev;
>  
> -	priv->workqueue = create_singlethread_workqueue(DRIVER_NAME "-wq");
> -	if (priv->workqueue == NULL)
> -		return -ENOMEM;
> -
>  	err = ht16k33_initialize(priv);
>  	if (err)
> -		goto err_destroy_wq;
> +		return err;
>  
>  	/* Framebuffer (2 bytes per column) */
>  	BUILD_BUG_ON(PAGE_SIZE < HT16K33_FB_SIZE);
>  	fbdev->buffer = (unsigned char *) get_zeroed_page(GFP_KERNEL);
> -	if (!fbdev->buffer) {
> -		err = -ENOMEM;
> -		goto err_destroy_wq;
> -	}
> +	if (!fbdev->buffer)
> +		return -ENOMEM;
>  
>  	fbdev->cache = devm_kmalloc(&client->dev, HT16K33_FB_SIZE, GFP_KERNEL);
>  	if (!fbdev->cache) {
> @@ -505,8 +498,6 @@ static int ht16k33_probe(struct i2c_client *client,
>  	framebuffer_release(fbdev->info);
>  err_fbdev_buffer:
>  	free_page((unsigned long) fbdev->buffer);
> -err_destroy_wq:
> -	destroy_workqueue(priv->workqueue);
>  
>  	return err;
>  }
> @@ -521,7 +512,6 @@ static int ht16k33_remove(struct i2c_client *client)
>  	framebuffer_release(fbdev->info);
>  	free_page((unsigned long) fbdev->buffer);
>  
> -	destroy_workqueue(priv->workqueue);
>  	return 0;
>  }
>  

Acked-by: Robin van der Gracht <robin@protonic.nl>

Re: [PATCH 2/3] auxdisplay: ht16k33: rework input device initialization

[Dmitry Torokhov]

Hi Robin,

On Wed, Feb 08, 2017 at 05:48:02PM +0100, Robin van der Gracht wrote:
> On Tue, 31 Jan 2017 12:54:37 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > +
> > +	err = devm_request_threaded_irq(&client->dev, client->irq,
> > +					NULL, ht16k33_keypad_irq_thread,
> > +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > +					DRIVER_NAME, keypad);
> 
> I believe the interrupt trigger should be switched to IRQF_TRIGGER_HIGH
> combined with IRQF_ONESHOT, for this new setup. This is causing a race
> condition.
> 
> In the previous situation a 'key data' read was triggered by
> ht16k33_keypad_start(). This cleared the IRQ and gave a (somewhat racy)
> known state. Now, when the IRQ line is HIGH during probe keyscan
> wont work. 
> 
> Also, when ht16k33_keypad_stop() is run while the irq thread is waiting
> for debounce, the scan will be skipped and irq will never be cleared.
> This also breaks keyscan.


You are absolutely right, I shoudl have changed this to
IRQF_TRIGGER_HIGH. I'll resend updated patch series in a minute.

Thanks.

-- 
Dmitry