Linux Input/HID development
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 4.14 081/170] Input: rotary-encoder - don't log EPROBE_DEFER to kernel log
From: Sasha Levin @ 2019-01-28 16:10 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Uwe Kleine-König, Dmitry Torokhov, Sasha Levin, linux-input
In-Reply-To: <20190128161200.55107-1-sashal@kernel.org>

From: Uwe Kleine-König <uwe@kleine-koenig.org>

[ Upstream commit 0832e93632c61987d504e251b927a2be769dd21a ]

When a driver fails to bind because a resource it still missing it's not
helpful to report this as (usually) probing is repeated later.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/input/misc/rotary_encoder.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 1588aecafff7..72eee6d55527 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -240,8 +240,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 
 	encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
 	if (IS_ERR(encoder->gpios)) {
-		dev_err(dev, "unable to get gpios\n");
-		return PTR_ERR(encoder->gpios);
+		err = PTR_ERR(encoder->gpios);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "unable to get gpios: %d\n", err);
+		return err;
 	}
 	if (encoder->gpios->ndescs < 2) {
 		dev_err(dev, "not enough gpios found\n");
-- 
2.19.1

^ permalink raw reply related

* [PATCH 1/3] input: keyboard: allow to select IMX SNVS Power Key driver for i.MX 7D
From: Stefan Agner @ 2019-01-28 16:03 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: arnd, peng.fan, linux-imx, kernel, fabio.estevam, shawnguo,
	linux-input, linux-kernel, Stefan Agner

The i.MX SNVS Power Key driver supports the i.MX 7D SoC family too.
Allow to enable the i.MX SNVS Power Key driver even if only i.MX 7D
SoC is selected.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/input/keyboard/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 4713957b0cbb..a878351f1643 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -420,7 +420,7 @@ config KEYBOARD_MPR121
 
 config KEYBOARD_SNVS_PWRKEY
 	tristate "IMX SNVS Power Key Driver"
-	depends on SOC_IMX6SX
+	depends on SOC_IMX6SX || SOC_IMX7D
 	depends on OF
 	help
 	  This is the snvs powerkey driver for the Freescale i.MX application
-- 
2.20.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.19 249/258] HID: lenovo: Add checks to fix of_led_classdev_register
From: Sasha Levin @ 2019-01-28 15:59 UTC (permalink / raw)
  To: linux-kernel, stable; +Cc: Aditya Pakki, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190128155924.51521-1-sashal@kernel.org>

From: Aditya Pakki <pakki001@umn.edu>

[ Upstream commit 6ae16dfb61bce538d48b7fe98160fada446056c5 ]

In lenovo_probe_tpkbd(), the function of_led_classdev_register() could
return an error value that is unchecked. The fix adds these checks.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/hid/hid-lenovo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 643b6eb54442..eacc76d2ab96 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -743,7 +743,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
 	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_mute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_mute);
+	ret = led_classdev_register(dev, &data_pointer->led_mute);
+	if (ret < 0)
+		goto err;
 
 	data_pointer->led_micmute.name = name_micmute;
 	data_pointer->led_micmute.brightness_get =
@@ -751,7 +753,11 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_micmute.brightness_set =
 		lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_micmute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_micmute);
+	ret = led_classdev_register(dev, &data_pointer->led_micmute);
+	if (ret < 0) {
+		led_classdev_unregister(&data_pointer->led_mute);
+		goto err;
+	}
 
 	lenovo_features_set_tpkbd(hdev);
 
-- 
2.19.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.19 124/258] Input: rotary-encoder - don't log EPROBE_DEFER to kernel log
From: Sasha Levin @ 2019-01-28 15:57 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Uwe Kleine-König, Dmitry Torokhov, Sasha Levin, linux-input
In-Reply-To: <20190128155924.51521-1-sashal@kernel.org>

From: Uwe Kleine-König <uwe@kleine-koenig.org>

[ Upstream commit 0832e93632c61987d504e251b927a2be769dd21a ]

When a driver fails to bind because a resource it still missing it's not
helpful to report this as (usually) probing is repeated later.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/input/misc/rotary_encoder.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 30ec77ad32c6..d748897bf5e9 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -240,8 +240,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 
 	encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
 	if (IS_ERR(encoder->gpios)) {
-		dev_err(dev, "unable to get gpios\n");
-		return PTR_ERR(encoder->gpios);
+		err = PTR_ERR(encoder->gpios);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "unable to get gpios: %d\n", err);
+		return err;
 	}
 	if (encoder->gpios->ndescs < 2) {
 		dev_err(dev, "not enough gpios found\n");
-- 
2.19.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.20 294/304] HID: lenovo: Add checks to fix of_led_classdev_register
From: Sasha Levin @ 2019-01-28 15:43 UTC (permalink / raw)
  To: linux-kernel, stable; +Cc: Aditya Pakki, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190128154341.47195-1-sashal@kernel.org>

From: Aditya Pakki <pakki001@umn.edu>

[ Upstream commit 6ae16dfb61bce538d48b7fe98160fada446056c5 ]

In lenovo_probe_tpkbd(), the function of_led_classdev_register() could
return an error value that is unchecked. The fix adds these checks.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/hid/hid-lenovo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 643b6eb54442..eacc76d2ab96 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -743,7 +743,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
 	data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_mute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_mute);
+	ret = led_classdev_register(dev, &data_pointer->led_mute);
+	if (ret < 0)
+		goto err;
 
 	data_pointer->led_micmute.name = name_micmute;
 	data_pointer->led_micmute.brightness_get =
@@ -751,7 +753,11 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 	data_pointer->led_micmute.brightness_set =
 		lenovo_led_brightness_set_tpkbd;
 	data_pointer->led_micmute.dev = dev;
-	led_classdev_register(dev, &data_pointer->led_micmute);
+	ret = led_classdev_register(dev, &data_pointer->led_micmute);
+	if (ret < 0) {
+		led_classdev_unregister(&data_pointer->led_mute);
+		goto err;
+	}
 
 	lenovo_features_set_tpkbd(hdev);
 
-- 
2.19.1

^ permalink raw reply related

* [PATCH AUTOSEL 4.20 148/304] Input: rotary-encoder - don't log EPROBE_DEFER to kernel log
From: Sasha Levin @ 2019-01-28 15:41 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Uwe Kleine-König, Dmitry Torokhov, Sasha Levin, linux-input
In-Reply-To: <20190128154341.47195-1-sashal@kernel.org>

From: Uwe Kleine-König <uwe@kleine-koenig.org>

[ Upstream commit 0832e93632c61987d504e251b927a2be769dd21a ]

When a driver fails to bind because a resource it still missing it's not
helpful to report this as (usually) probing is repeated later.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/input/misc/rotary_encoder.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 30ec77ad32c6..d748897bf5e9 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -240,8 +240,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 
 	encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
 	if (IS_ERR(encoder->gpios)) {
-		dev_err(dev, "unable to get gpios\n");
-		return PTR_ERR(encoder->gpios);
+		err = PTR_ERR(encoder->gpios);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "unable to get gpios: %d\n", err);
+		return err;
 	}
 	if (encoder->gpios->ndescs < 2) {
 		dev_err(dev, "not enough gpios found\n");
-- 
2.19.1

^ permalink raw reply related

* Re: [PATCH v2] HID: debug: fix the ring buffer implementation
From: Oleg Nesterov @ 2019-01-28 11:18 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Vladis Dronov, Jiri Kosina, open list:HID CORE LAYER, lkml, 3.8+
In-Reply-To: <CAO-hwJJGux_5bQJmXgLrcj+v=n3n8hvqXfqPEu60aghYNJSfzw@mail.gmail.com>

On 01/28, Benjamin Tissoires wrote:
>
> Oleg, can you provide some feedback before I push this?

Looks good to me, feel free to add

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> > +               set_current_state(TASK_RUNNING);

I still think that

		    __set_current_state(TASK_RUNNING);

will look a bit better, but this is really minor.

Oleg.

^ permalink raw reply

* Re: [PATCH v2] drm/bridge: sil_sii8620: make remote control optional.
From: Andrzej Hajda @ 2019-01-28 10:53 UTC (permalink / raw)
  To: Inki Dae, Laurent Pinchart, Dmitry Torokhov, Lukas Wunner,
	dri-devel, linux-input, linux-kernel
In-Reply-To: <20190125013355.GA6722@innovation.ch>

On 25.01.2019 02:33, Ronald Tschalär wrote:
> commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency
> of RC_CORE) changed the driver to select both RC_CORE and INPUT.
> However, this causes problems with other drivers, in particular an input
> driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate
> commit):
>
>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
>
> According to the docs and general consensus, select should only be used
> for non user-visible symbols, but both RC_CORE and INPUT are
> user-visible. Furthermore almost all other references to INPUT
> throughout the kernel config are depends, not selects. For this reason
> the first part of this change reverts commit d6abe6df706c.
>
> In order to address the original reason for commit d6abe6df706c, namely
> that not all boards use the remote controller functionality and hence
> should not need have to deal with RC_CORE, the second part of this
> change now makes the remote control support in the driver optional and
> contingent on RC_CORE being defined. And with this the hard dependency
> on INPUT also goes away as that is only needed if RC_CORE is defined
> (which in turn already depends on INPUT).


Thanks for fixing it, this seems to be a best way to deal with it, more
comments below.


>
> CC: Inki Dae <inki.dae@samsung.com>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> ---
> Resending this, as I somehow managed to forget to cc dri-devel.
> Apologies for the duplication.
>
> Changes in v2:
>  - completely remove dependencies on both RC_CORE and INPUT in Kconfig,
>  - make remote control functionality in driver contingent on RC_CORE
>    being defined
>
>  drivers/gpu/drm/bridge/Kconfig       |  2 --
>  drivers/gpu/drm/bridge/sil-sii8620.c | 17 +++++++++++++++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 2fee47b0d50b..a11198a36005 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -85,8 +85,6 @@ config DRM_SIL_SII8620
>  	depends on OF
>  	select DRM_KMS_HELPER
>  	imply EXTCON
> -	select INPUT
> -	select RC_CORE


Shouldn't you put here "imply RC_CORE"? To avoid RC_CORE as module and
sii8620 built-in.


>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
>  
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 0cc293a6ac24..dee47752791e 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -66,7 +66,9 @@ enum sii8620_mt_state {
>  struct sii8620 {
>  	struct drm_bridge bridge;
>  	struct device *dev;
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  	struct rc_dev *rc_dev;
> +#endif
>  	struct clk *clk_xtal;
>  	struct gpio_desc *gpio_reset;
>  	struct gpio_desc *gpio_int;
> @@ -1756,6 +1758,7 @@ static void sii8620_send_features(struct sii8620 *ctx)
>  	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>  }
>  
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
>  {
>  	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
> @@ -1774,6 +1777,12 @@ static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
>  
>  	return true;
>  }
> +#else
> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> +{
> +	return false;
> +}
> +#endif
>  
>  static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>  {
> @@ -2097,6 +2106,7 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>  	enable_irq(to_i2c_client(ctx->dev)->irq);
>  }
>  
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
>  {
>  	struct rc_dev *rc_dev;
> @@ -2126,6 +2136,11 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
>  	}
>  	ctx->rc_dev = rc_dev;
>  }
> +#else
> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> +{
> +}
> +#endif
>  
>  static void sii8620_cable_out(struct sii8620 *ctx)
>  {
> @@ -2214,9 +2229,11 @@ static int sii8620_attach(struct drm_bridge *bridge)
>  
>  static void sii8620_detach(struct drm_bridge *bridge)
>  {
> +#if IS_ENABLED(CONFIG_RC_CORE)
>  	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>  
>  	rc_unregister_device(ctx->rc_dev);
> +#endif
>  }


In two cases you create stub functions, in the third you put ifdefs
inside function, some lack of consistency.

I wonder if it wouldn't be better to create stubs just for rc_core
functions, the best would be to put stubs into rc-core.h, but if the
case of 'optional rc-core' is too rare you can put stubs into the driver:

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c

index a6e8f4591e63..6ca838d30f93 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -35,6 +35,13 @@
 
 #include "sil-sii8620.h"
 
+#if !IS_ENABLED(CONFIG_RC_CORE)
+#define rc_unregister_device(dev) ({ (void)(dev); })
+#define rc_allocate_device(type) ({ NULL; })
+#define rc_keydown(...) ({ })
+#define rc_keyup(...) ({ })
+#endif
+
 #define SII8620_BURST_BUF_LEN 288
 #define VAL_RX_HDMI_CTRL2_DEFVAL VAL_RX_HDMI_CTRL2_IDLE_CNT(3)

Regards

Andrzej


>  
>  static int sii8620_is_packing_required(struct sii8620 *ctx,

^ permalink raw reply related

* Re: [PATCH v2] HID: debug: fix the ring buffer implementation
From: Benjamin Tissoires @ 2019-01-28  9:14 UTC (permalink / raw)
  To: Vladis Dronov
  Cc: Jiri Kosina, Oleg Nesterov, open list:HID CORE LAYER, lkml, 3.8+
In-Reply-To: <20190126170722.8035-1-vdronov@redhat.com>

On Sat, Jan 26, 2019 at 6:07 PM Vladis Dronov <vdronov@redhat.com> wrote:
>
> Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
> is strange allowing lost or corrupted data. After commit 717adfdaf147
> ("HID: debug: check length before copy_to_user()") it is possible to enter
> an infinite loop in hid_debug_events_read() by providing 0 as count, this
> locks up a system. Fix this by rewriting the ring buffer implementation
> with kfifo and simplify the code.
>
> This fixes CVE-2019-3819.
>
> v2: fix an execution logic and add a comment

Thanks for the respin.
v2 looks good to me.
Oleg, can you provide some feedback before I push this?

Cheers,
Benjamin

>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1669187
> Cc: stable@vger.kernel.org # v4.18+
> Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
> Fixes: 717adfdaf147 ("HID: debug: check length before copy_to_user()")
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
>  drivers/hid/hid-debug.c   | 116 ++++++++++++++------------------------
>  include/linux/hid-debug.h |   9 ++-
>  2 files changed, 47 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index c530476edba6..08870c909268 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -30,6 +30,7 @@
>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
> +#include <linux/kfifo.h>
>  #include <linux/sched/signal.h>
>  #include <linux/export.h>
>  #include <linux/slab.h>
> @@ -661,17 +662,12 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
>  /* enqueue string to 'events' ring buffer */
>  void hid_debug_event(struct hid_device *hdev, char *buf)
>  {
> -       unsigned i;
>         struct hid_debug_list *list;
>         unsigned long flags;
>
>         spin_lock_irqsave(&hdev->debug_list_lock, flags);
> -       list_for_each_entry(list, &hdev->debug_list, node) {
> -               for (i = 0; buf[i]; i++)
> -                       list->hid_debug_buf[(list->tail + i) % HID_DEBUG_BUFSIZE] =
> -                               buf[i];
> -               list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
> -        }
> +       list_for_each_entry(list, &hdev->debug_list, node)
> +               kfifo_in(&list->hid_debug_fifo, buf, strlen(buf));
>         spin_unlock_irqrestore(&hdev->debug_list_lock, flags);
>
>         wake_up_interruptible(&hdev->debug_wait);
> @@ -722,8 +718,7 @@ void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage, __s32 valu
>         hid_debug_event(hdev, buf);
>
>         kfree(buf);
> -        wake_up_interruptible(&hdev->debug_wait);
> -
> +       wake_up_interruptible(&hdev->debug_wait);
>  }
>  EXPORT_SYMBOL_GPL(hid_dump_input);
>
> @@ -1083,8 +1078,8 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
>                 goto out;
>         }
>
> -       if (!(list->hid_debug_buf = kzalloc(HID_DEBUG_BUFSIZE, GFP_KERNEL))) {
> -               err = -ENOMEM;
> +       err = kfifo_alloc(&list->hid_debug_fifo, HID_DEBUG_FIFOSIZE, GFP_KERNEL);
> +       if (err) {
>                 kfree(list);
>                 goto out;
>         }
> @@ -1104,77 +1099,57 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
>                 size_t count, loff_t *ppos)
>  {
>         struct hid_debug_list *list = file->private_data;
> -       int ret = 0, len;
> +       int ret = 0, copied;
>         DECLARE_WAITQUEUE(wait, current);
>
>         mutex_lock(&list->read_mutex);
> -       while (ret == 0) {
> -               if (list->head == list->tail) {
> -                       add_wait_queue(&list->hdev->debug_wait, &wait);
> -                       set_current_state(TASK_INTERRUPTIBLE);
> -
> -                       while (list->head == list->tail) {
> -                               if (file->f_flags & O_NONBLOCK) {
> -                                       ret = -EAGAIN;
> -                                       break;
> -                               }
> -                               if (signal_pending(current)) {
> -                                       ret = -ERESTARTSYS;
> -                                       break;
> -                               }
> -
> -                               if (!list->hdev || !list->hdev->debug) {
> -                                       ret = -EIO;
> -                                       set_current_state(TASK_RUNNING);
> -                                       goto out;
> -                               }
> -
> -                               /* allow O_NONBLOCK from other threads */
> -                               mutex_unlock(&list->read_mutex);
> -                               schedule();
> -                               mutex_lock(&list->read_mutex);
> -                               set_current_state(TASK_INTERRUPTIBLE);
> -                       }
> -
> -                       set_current_state(TASK_RUNNING);
> -                       remove_wait_queue(&list->hdev->debug_wait, &wait);
> -               }
> -
> -               if (ret)
> -                       goto out;
> +       if (kfifo_is_empty(&list->hid_debug_fifo)) {
> +               add_wait_queue(&list->hdev->debug_wait, &wait);
> +               set_current_state(TASK_INTERRUPTIBLE);
> +
> +               while (kfifo_is_empty(&list->hid_debug_fifo)) {
> +                       if (file->f_flags & O_NONBLOCK) {
> +                               ret = -EAGAIN;
> +                               break;
> +                       }
> +
> +                       if (signal_pending(current)) {
> +                               ret = -ERESTARTSYS;
> +                               break;
> +                       }
> +
> +                       /* if list->hdev is NULL we cannot remove_wait_queue().
> +                        * if list->hdev->debug is 0 then hid_debug_unregister()
> +                        * was already called and list->hdev is being destroyed.
> +                        * if we add remove_wait_queue() here we can hit a race.
> +                        */
> +                       if (!list->hdev || !list->hdev->debug) {
> +                               ret = -EIO;
> +                               set_current_state(TASK_RUNNING);
> +                               goto out;
> +                       }
> +
> +                       /* allow O_NONBLOCK from other threads */
> +                       mutex_unlock(&list->read_mutex);
> +                       schedule();
> +                       mutex_lock(&list->read_mutex);
> +                       set_current_state(TASK_INTERRUPTIBLE);
> +               }
> +
> +               set_current_state(TASK_RUNNING);
> +               remove_wait_queue(&list->hdev->debug_wait, &wait);
> +
> +               if (ret)
> +                       goto out;
> +       }
>
> -               /* pass the ringbuffer contents to userspace */
> -copy_rest:
> -               if (list->tail == list->head)
> -                       goto out;
> -               if (list->tail > list->head) {
> -                       len = list->tail - list->head;
> -                       if (len > count)
> -                               len = count;
> -
> -                       if (copy_to_user(buffer + ret, &list->hid_debug_buf[list->head], len)) {
> -                               ret = -EFAULT;
> -                               goto out;
> -                       }
> -                       ret += len;
> -                       list->head += len;
> -               } else {
> -                       len = HID_DEBUG_BUFSIZE - list->head;
> -                       if (len > count)
> -                               len = count;
> -
> -                       if (copy_to_user(buffer, &list->hid_debug_buf[list->head], len)) {
> -                               ret = -EFAULT;
> -                               goto out;
> -                       }
> -                       list->head = 0;
> -                       ret += len;
> -                       count -= len;
> -                       if (count > 0)
> -                               goto copy_rest;
> -               }
> -
> -       }
> +       /* pass the fifo content to userspace, locking is not needed with only
> +        * one concurrent reader and one concurrent writer
> +        */
> +       ret = kfifo_to_user(&list->hid_debug_fifo, buffer, count, &copied);
> +       if (ret)
> +               goto out;
> +       ret = copied;
>  out:
>         mutex_unlock(&list->read_mutex);
>         return ret;
> @@ -1185,7 +1160,7 @@ static __poll_t hid_debug_events_poll(struct file *file, poll_table *wait)
>         struct hid_debug_list *list = file->private_data;
>
>         poll_wait(file, &list->hdev->debug_wait, wait);
> -       if (list->head != list->tail)
> +       if (!kfifo_is_empty(&list->hid_debug_fifo))
>                 return EPOLLIN | EPOLLRDNORM;
>         if (!list->hdev->debug)
>                 return EPOLLERR | EPOLLHUP;
> @@ -1200,7 +1175,7 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
>         spin_lock_irqsave(&list->hdev->debug_list_lock, flags);
>         list_del(&list->node);
>         spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
> -       kfree(list->hid_debug_buf);
> +       kfifo_free(&list->hid_debug_fifo);
>         kfree(list);
>
>         return 0;
> @@ -1246,4 +1221,3 @@ void hid_debug_exit(void)
>  {
>         debugfs_remove_recursive(hid_debug_root);
>  }
> -
> diff --git a/include/linux/hid-debug.h b/include/linux/hid-debug.h
> index 8663f216c563..e7a7c92aaf09 100644
> --- a/include/linux/hid-debug.h
> +++ b/include/linux/hid-debug.h
> @@ -24,7 +24,10 @@
>
>  #ifdef CONFIG_DEBUG_FS
>
> +#include <linux/kfifo.h>
> +
>  #define HID_DEBUG_BUFSIZE 512
> +#define HID_DEBUG_FIFOSIZE 512
>
>  void hid_dump_input(struct hid_device *, struct hid_usage *, __s32);
>  void hid_dump_report(struct hid_device *, int , u8 *, int);
> @@ -38,10 +41,7 @@ void hid_debug_event(struct hid_device *, char *);
>  void hid_debug_event(struct hid_device *, char *);
>
> -
>  struct hid_debug_list {
> -       char *hid_debug_buf;
> -       int head;
> -       int tail;
> +       DECLARE_KFIFO_PTR(hid_debug_fifo, char);
>         struct fasync_struct *fasync;
>         struct hid_device *hdev;
>         struct list_head node;
> @@ -64,4 +64,3 @@ struct hid_debug_list {
>  #endif
>
>  #endif
> -

^ permalink raw reply

* AW: [PATCH 2/3] Input: st1232 - add support for st1633
From: Kepplinger Martin @ 2019-01-28  8:49 UTC (permalink / raw)
  To: Martin Kepplinger, devicetree@vger.kernel.org,
	linux-input@vger.kernel.org
  Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
	mark.rutland@arm.com, linux-kernel@vger.kernel.org
In-Reply-To: <20190128084449.16070-2-martink@posteo.de>

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]


________________________________________
Von: Martin Kepplinger [martink@posteo.de]
Gesendet: Montag, 28. Jänner 2019 09:44
An: devicetree@vger.kernel.org; linux-input@vger.kernel.org
Cc: dmitry.torokhov@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; Kepplinger Martin
Betreff: [PATCH 2/3] Input: st1232 - add support for st1633

From: Martin Kepplinger <martin.kepplinger@ginzinger.com>

Add support for the Sitronix ST1633 touchscreen controller to the st1232
driver. A protocol spec can be found here:
www.ampdisplay.com/documents/pdf/AM-320480B6TZQW-TC0H.pdf

Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
---

sorry, this is v3 of this series.

anyways, the of_device.h header isn't directly need now neither. thanks Dmitry!

                                 martin


revision history
----------------
v3: implement Dmitry's suggestion for i2c probe from v2 review
v2: use device_get_match_data(), invent an internal "have_z" bool property
v1: initial idea for the change



 drivers/input/touchscreen/Kconfig  |   6 +-
 drivers/input/touchscreen/st1232.c | 122 +++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 34 deletions(-)


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3616 bytes --]

^ permalink raw reply

* [PATCH 3/3] Input: st1232 - add Martin as module author
From: Martin Kepplinger @ 2019-01-28  8:44 UTC (permalink / raw)
  To: devicetree, linux-input
  Cc: dmitry.torokhov, robh+dt, mark.rutland, linux-kernel,
	Martin Kepplinger
In-Reply-To: <20190128084449.16070-1-martink@posteo.de>

From: Martin Kepplinger <martin.kepplinger@ginzinger.com>

This adds myself as an author of the st1232 driver module as Tony's
email address doesn't seem to work anymore.

Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
---
 drivers/input/touchscreen/st1232.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 19a665d48dad..906b233970aa 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -346,5 +346,6 @@ static struct i2c_driver st1232_ts_driver = {
 module_i2c_driver(st1232_ts_driver);
 
 MODULE_AUTHOR("Tony SIM <chinyeow.sim.xt@renesas.com>");
+MODULE_AUTHOR("Martin Kepplinger <martin.kepplinger@ginzinger.com>");
 MODULE_DESCRIPTION("SITRONIX ST1232 Touchscreen Controller Driver");
 MODULE_LICENSE("GPL v2");
-- 
2.20.1

^ permalink raw reply related

* [PATCH 2/3] Input: st1232 - add support for st1633
From: Martin Kepplinger @ 2019-01-28  8:44 UTC (permalink / raw)
  To: devicetree, linux-input
  Cc: dmitry.torokhov, robh+dt, mark.rutland, linux-kernel,
	Martin Kepplinger
In-Reply-To: <20190128084449.16070-1-martink@posteo.de>

From: Martin Kepplinger <martin.kepplinger@ginzinger.com>

Add support for the Sitronix ST1633 touchscreen controller to the st1232
driver. A protocol spec can be found here:
www.ampdisplay.com/documents/pdf/AM-320480B6TZQW-TC0H.pdf

Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
---
 drivers/input/touchscreen/Kconfig  |   6 +-
 drivers/input/touchscreen/st1232.c | 122 +++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 34 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 068dbbc610fc..7c597a49c265 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1168,11 +1168,11 @@ config TOUCHSCREEN_SIS_I2C
 	  module will be called sis_i2c.
 
 config TOUCHSCREEN_ST1232
-	tristate "Sitronix ST1232 touchscreen controllers"
+	tristate "Sitronix ST1232 or ST1633 touchscreen controllers"
 	depends on I2C
 	help
-	  Say Y here if you want to support Sitronix ST1232
-	  touchscreen controller.
+	  Say Y here if you want to support the Sitronix ST1232
+	  or ST1633 touchscreen controller.
 
 	  If unsure, say N.
 
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 11ff32c68025..19a665d48dad 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -23,13 +23,15 @@
 #include <linux/types.h>
 
 #define ST1232_TS_NAME	"st1232-ts"
+#define ST1633_TS_NAME	"st1633-ts"
+
+enum {
+	st1232,
+	st1633,
+};
 
 #define MIN_X		0x00
 #define MIN_Y		0x00
-#define MAX_X		0x31f	/* (800 - 1) */
-#define MAX_Y		0x1df	/* (480 - 1) */
-#define MAX_AREA	0xff
-#define MAX_FINGERS	2
 
 struct st1232_ts_finger {
 	u16 x;
@@ -38,12 +40,24 @@ struct st1232_ts_finger {
 	bool is_valid;
 };
 
+struct st_chip_info {
+	bool	have_z;
+	u16	max_x;
+	u16	max_y;
+	u16	max_area;
+	u16	max_fingers;
+	u8	start_reg;
+};
+
 struct st1232_ts_data {
 	struct i2c_client *client;
 	struct input_dev *input_dev;
-	struct st1232_ts_finger finger[MAX_FINGERS];
 	struct dev_pm_qos_request low_latency_req;
 	int reset_gpio;
+	const struct st_chip_info *chip_info;
+	int read_buf_len;
+	u8 *read_buf;
+	struct st1232_ts_finger *finger;
 };
 
 static int st1232_ts_read_data(struct st1232_ts_data *ts)
@@ -52,40 +66,35 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
 	struct i2c_client *client = ts->client;
 	struct i2c_msg msg[2];
 	int error;
-	u8 start_reg;
-	u8 buf[10];
+	int i, y;
+	u8 start_reg = ts->chip_info->start_reg;
+	u8 *buf = ts->read_buf;
 
-	/* read touchscreen data from ST1232 */
+	/* read touchscreen data */
 	msg[0].addr = client->addr;
 	msg[0].flags = 0;
 	msg[0].len = 1;
 	msg[0].buf = &start_reg;
-	start_reg = 0x10;
 
 	msg[1].addr = ts->client->addr;
 	msg[1].flags = I2C_M_RD;
-	msg[1].len = sizeof(buf);
+	msg[1].len = ts->read_buf_len;
 	msg[1].buf = buf;
 
 	error = i2c_transfer(client->adapter, msg, 2);
 	if (error < 0)
 		return error;
 
-	/* get "valid" bits */
-	finger[0].is_valid = buf[2] >> 7;
-	finger[1].is_valid = buf[5] >> 7;
+	for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
+		finger[i].is_valid = buf[i + y] >> 7;
+		if (finger[i].is_valid) {
+			finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
+			finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
 
-	/* get xy coordinate */
-	if (finger[0].is_valid) {
-		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
-		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
-		finger[0].t = buf[8];
-	}
-
-	if (finger[1].is_valid) {
-		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
-		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
-		finger[1].t = buf[9];
+			/* st1232 includes a z-axis / touch strength */
+			if (ts->chip_info->have_z)
+				finger[i].t = buf[i + 6];
+		}
 	}
 
 	return 0;
@@ -104,11 +113,14 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
 		goto end;
 
 	/* multi touch protocol */
-	for (i = 0; i < MAX_FINGERS; i++) {
+	for (i = 0; i < ts->chip_info->max_fingers; i++) {
 		if (!finger[i].is_valid)
 			continue;
 
-		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
+		if (ts->chip_info->have_z)
+			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
+					 finger[i].t);
+
 		input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
 		input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
 		input_mt_sync(input_dev);
@@ -142,12 +154,40 @@ static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
 		gpio_direction_output(ts->reset_gpio, poweron);
 }
 
+static const struct st_chip_info st1232_chip_info = {
+		.have_z = true,
+		.max_x = 0x31f, /* 800 - 1 */
+		.max_y = 0x1df, /* 480 -1 */
+		.max_area = 0xff,
+		.max_fingers = 2,
+		.start_reg = 0x12,
+};
+
+static const struct st_chip_info st1633_chip_info = {
+		.have_z = false,
+		.max_x = 0x13f, /* 320 - 1 */
+		.max_y = 0x1df, /* 480 -1 */
+		.max_area = 0x00,
+		.max_fingers = 5,
+		.start_reg = 0x12,
+};
+
 static int st1232_ts_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
 	struct st1232_ts_data *ts;
+	struct st1232_ts_finger *finger;
 	struct input_dev *input_dev;
 	int error;
+	const struct st_chip_info *match = NULL;
+
+	match = device_get_match_data(&client->dev);
+	if (!match && id)
+		match = (const void *)id->driver_data;
+	if (!match) {
+		dev_err(&client->dev, "unknown device model\n");
+		return -ENODEV;
+	}
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
@@ -163,6 +203,19 @@ static int st1232_ts_probe(struct i2c_client *client,
 	if (!ts)
 		return -ENOMEM;
 
+	ts->chip_info = match;
+	ts->finger = devm_kzalloc(&client->dev,
+				  sizeof(*finger) * ts->chip_info->max_fingers,
+				  GFP_KERNEL);
+	if (!ts->finger)
+		return -ENOMEM;
+
+	/* allocate a buffer according to the number of registers to read */
+	ts->read_buf_len = ts->chip_info->max_fingers * 4;
+	ts->read_buf = devm_kzalloc(&client->dev, ts->read_buf_len, GFP_KERNEL);
+	if (!ts->read_buf)
+		return -ENOMEM;
+
 	input_dev = devm_input_allocate_device(&client->dev);
 	if (!input_dev)
 		return -ENOMEM;
@@ -192,9 +245,14 @@ static int st1232_ts_probe(struct i2c_client *client,
 	__set_bit(EV_KEY, input_dev->evbit);
 	__set_bit(EV_ABS, input_dev->evbit);
 
-	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
-	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 0);
-	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 0);
+	if (ts->chip_info->have_z)
+		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
+				     ts->chip_info->max_area, 0, 0);
+
+	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
+			     MIN_X, ts->chip_info->max_x, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
+			     MIN_Y, ts->chip_info->max_y, 0, 0);
 
 	error = devm_request_threaded_irq(&client->dev, client->irq,
 					  NULL, st1232_ts_irq_handler,
@@ -261,13 +319,15 @@ static SIMPLE_DEV_PM_OPS(st1232_ts_pm_ops,
 			 st1232_ts_suspend, st1232_ts_resume);
 
 static const struct i2c_device_id st1232_ts_id[] = {
-	{ ST1232_TS_NAME, 0 },
+	{ ST1232_TS_NAME, (unsigned long)&st1232_chip_info },
+	{ ST1633_TS_NAME, (unsigned long)&st1633_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
 
 static const struct of_device_id st1232_ts_dt_ids[] = {
-	{ .compatible = "sitronix,st1232", },
+	{ .compatible = "sitronix,st1232", .data = &st1232_chip_info },
+	{ .compatible = "sitronix,st1633", .data = &st1633_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, st1232_ts_dt_ids);
-- 
2.20.1

^ permalink raw reply related

* [PATCH 1/3] dt-bindings: input: sitronix-st1232: add compatible string for ST1633
From: Martin Kepplinger @ 2019-01-28  8:44 UTC (permalink / raw)
  To: devicetree, linux-input
  Cc: dmitry.torokhov, robh+dt, mark.rutland, linux-kernel,
	Martin Kepplinger

From: Martin Kepplinger <martin.kepplinger@ginzinger.com>

The st1232 driver gains support for the ST1633 controller too; update
the bindings doc accordingly.

Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
---
 .../bindings/input/touchscreen/sitronix-st1232.txt          | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
index 64ad48b824a2..e73e826e0f2a 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
@@ -1,7 +1,9 @@
-* Sitronix st1232 touchscreen controller
+* Sitronix st1232 or st1633 touchscreen controller
 
 Required properties:
-- compatible: must be "sitronix,st1232"
+- compatible: must contain one of
+  * "sitronix,st1232"
+  * "sitronix,st1633"
 - reg: I2C address of the chip
 - interrupts: interrupt to which the chip is connected
 
-- 
2.20.1

^ permalink raw reply related

* Re: [git pull] Input updates for v5.0-rc3
From: pr-tracker-bot @ 2019-01-27 17:25 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Linus Torvalds, linux-kernel, linux-input
In-Reply-To: <20190126011113.GA23858@dtor-ws>

The pull request you sent on Fri, 25 Jan 2019 17:11:13 -0800:

> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/78e372e6509bc2412e86afb11be65185f4c9c568

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

^ permalink raw reply

* Re: [bug report] Input: add st-keyscan driver
From: Ken Sloat @ 2019-01-27  1:20 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Dan Carpenter, linux-input, Gabriel FERNANDEZ, lkml, Nate Drude,
	giuseppe.condorelli
In-Reply-To: <CAKdAkRT6hKywKW1NnD-hiuFn9YTZF74GZVr36ZNJqyoxQghT9Q@mail.gmail.com>

On Sat, Jan 26, 2019 at 5:15 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sat, Jan 26, 2019 at 1:25 PM Ken Sloat
> <ken.sloat@ohmlinxelectronics.com> wrote:
> >
> > On Tue, Jan 22, 2019 at 1:53 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > Hello Gabriel FERNANDEZ,
> >
> > Hello Dan,
> >
> > I have added CCs for the maintainers as well as Gabriel Fernandez as
> > currently you just have the linux-input mailing list
> >
> > > The patch 062589b13991: "Input: add st-keyscan driver" from Apr 12,
> > > 2014, leads to the following static checker warning:
> > >
> > >         drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
> > >         error: potential zalloc NULL dereference: 'keypad_data->input_dev'
> > >
> > > drivers/input/keyboard/st-keyscan.c
> > >     125 static int keyscan_probe(struct platform_device *pdev)
> > >     126 {
> > >     127         struct st_keyscan *keypad_data;
> > >     128         struct input_dev *input_dev;
> > >     129         struct resource *res;
> > >     130         int error;
> > >     131
> > >     132         if (!pdev->dev.of_node) {
> > >     133                 dev_err(&pdev->dev, "no DT data present\n");
> > >     134                 return -EINVAL;
> > >     135         }
> > >     136
> > >     137         keypad_data = devm_kzalloc(&pdev->dev, sizeof(*keypad_data),
> > >     138                                    GFP_KERNEL);
> > >     139         if (!keypad_data)
> > >     140                 return -ENOMEM;
> > >     141
> > >     142         input_dev = devm_input_allocate_device(&pdev->dev);
> > >     143         if (!input_dev) {
> > >     144                 dev_err(&pdev->dev, "failed to allocate the input device\n");
> > >     145                 return -ENOMEM;
> > >     146         }
> > >     147
> > >     148         input_dev->name = pdev->name;
> > >     149         input_dev->phys = "keyscan-keys/input0";
> > >     150         input_dev->dev.parent = &pdev->dev;
> > >     151         input_dev->open = keyscan_open;
> > >     152         input_dev->close = keyscan_close;
> > >     153
> > >     154         input_dev->id.bustype = BUS_HOST;
> > >     155
> > > --> 156         error = keypad_matrix_key_parse_dt(keypad_data);
> > >                                                    ^^^^^^^^^^^
> > I agree with you this would be a problem
> > to clarify the NULL derefence would occur here within keypad_matrix_key_parse_dt
> >
> > struct device *dev = keypad_data->input_dev->dev.parent;
> >
> > > This assumes we have set "keypad_data->input_dev = input_dev;" but we
> > > don't do that until...
> > >
> > >     157         if (error)
> > >     158                 return error;
> > >     159
> > >     160         error = matrix_keypad_build_keymap(NULL, NULL,
> > >     161                                            keypad_data->n_rows,
> > >     162                                            keypad_data->n_cols,
> > >     163                                            NULL, input_dev);
> > >     164         if (error) {
> > >     165                 dev_err(&pdev->dev, "failed to build keymap\n");
> > >     166                 return error;
> > >     167         }
> > >     168
> > >     169         input_set_drvdata(input_dev, keypad_data);
> > >     170
> > >     171         keypad_data->input_dev = input_dev;
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > this line here.  This driver has never worked and it was included almost
> > > five years ago.  Is it worth fixing?
> > >
> > >     172
> > >     173         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >     174         keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
> > >     175         if (IS_ERR(keypad_data->base))
> > >     176                 return PTR_ERR(keypad_data->base);
> > >     177
> > >
> > > regards,
> > > dan carpenter
> > >
> >
> > Here is the interesting thing, I was looking on patchwork, and several
> > of the patches including what appears to be the latest actually set
> > "keypad_data->input_dev = input_dev" before calling
> > "keypad_matrix_key_parse_dt"
> >
> > From v4 on patchwork
> > + if (IS_ERR(keypad_data->clk)) {
> > + dev_err(&pdev->dev, "cannot get clock");
> > + return PTR_ERR(keypad_data->clk);
> > + }
> > +
> > + keypad_data->input_dev = input_dev;
> > +
> > + input_dev->name = pdev->name;
> > + input_dev->phys = "keyscan-keys/input0";
> > + input_dev->dev.parent = &pdev->dev;
> > + input_dev->open = keyscan_open;
> > + input_dev->close = keyscan_close;
> > +
> > + input_dev->id.bustype = BUS_HOST;
> > +
> > + error = keypad_matrix_key_parse_dt(keypad_data);
> >
> > According to patchwork, these aren't listed as accepted, so I'm not
> > sure where the exact accepted patch came from. Looking at the commit
> > log, it looks like the issue you showed above was made in the original
> > commit 062589b1399176a9c14bc68e16169f40439d658c so I'm not quite sure
> > what is going on here. Maybe the maintainer can chime in with the
> > original patch/mailing list discussion on this. For reference, I've
> > added the patchwork links below
> > https://patchwork.kernel.org/patch/3854341/
> > https://patchwork.kernel.org/patch/3968891/
> > https://patchwork.kernel.org/patch/3969991/
>
> It may very well be that I messed up when applying the patch. I guess
> whatever platform that is using the driver has not attempted to update
> their kernel since then.
>
> Thanks.
>
> --
> Dmitry

Hi Dmitry,

Thanks for the quick response. Yes I was just looking at the other
mailing lists patchwork and while comments were missing on the
linux-input list for the v4 patch, there was a discussion on the
regular kernel mailing list:
https://lore.kernel.org/patchwork/patch/455450/#630445

It looks like you told him he didn't need to submit a v5 but generated
one based on the changes suggested in the discussion:

[begin quote]
On Wed, Apr 16, 2014 at 10:49:29AM +0200, Gabriel Fernandez wrote:
> On 13 April 2014 07:10, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > Does the version of the patch below still work for you?
> >
> Yes it's was tested on b2000 and b2089 sti boards.
>
> > Thanks.
> >
> > --
> > Dmitry
> >
> Thanks for yours remarks, i will prepare a v5 versions.


If the version I sent to you works then you do not need to prepare v5,
I'll just apply what I have.

Thanks!
[end quote]

So I guess Dan's original question remains, should this be fixed
considering it's so old and apparently nobody could possibly be using
it in the kernel since it doesn't work (in which case the driver
should probably be dropped) or should we fix it?

Thanks,
Ken Sloat

^ permalink raw reply

* Re: [bug report] Input: add st-keyscan driver
From: Dmitry Torokhov @ 2019-01-26 22:15 UTC (permalink / raw)
  To: Ken Sloat; +Cc: Dan Carpenter, linux-input, Gabriel FERNANDEZ, lkml, Nate Drude
In-Reply-To: <CAPo_4QB+ozVESD85d2qxuitgvgPgLvrjx9SyEMfFTGvkz4MjWg@mail.gmail.com>

On Sat, Jan 26, 2019 at 1:25 PM Ken Sloat
<ken.sloat@ohmlinxelectronics.com> wrote:
>
> On Tue, Jan 22, 2019 at 1:53 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hello Gabriel FERNANDEZ,
>
> Hello Dan,
>
> I have added CCs for the maintainers as well as Gabriel Fernandez as
> currently you just have the linux-input mailing list
>
> > The patch 062589b13991: "Input: add st-keyscan driver" from Apr 12,
> > 2014, leads to the following static checker warning:
> >
> >         drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
> >         error: potential zalloc NULL dereference: 'keypad_data->input_dev'
> >
> > drivers/input/keyboard/st-keyscan.c
> >     125 static int keyscan_probe(struct platform_device *pdev)
> >     126 {
> >     127         struct st_keyscan *keypad_data;
> >     128         struct input_dev *input_dev;
> >     129         struct resource *res;
> >     130         int error;
> >     131
> >     132         if (!pdev->dev.of_node) {
> >     133                 dev_err(&pdev->dev, "no DT data present\n");
> >     134                 return -EINVAL;
> >     135         }
> >     136
> >     137         keypad_data = devm_kzalloc(&pdev->dev, sizeof(*keypad_data),
> >     138                                    GFP_KERNEL);
> >     139         if (!keypad_data)
> >     140                 return -ENOMEM;
> >     141
> >     142         input_dev = devm_input_allocate_device(&pdev->dev);
> >     143         if (!input_dev) {
> >     144                 dev_err(&pdev->dev, "failed to allocate the input device\n");
> >     145                 return -ENOMEM;
> >     146         }
> >     147
> >     148         input_dev->name = pdev->name;
> >     149         input_dev->phys = "keyscan-keys/input0";
> >     150         input_dev->dev.parent = &pdev->dev;
> >     151         input_dev->open = keyscan_open;
> >     152         input_dev->close = keyscan_close;
> >     153
> >     154         input_dev->id.bustype = BUS_HOST;
> >     155
> > --> 156         error = keypad_matrix_key_parse_dt(keypad_data);
> >                                                    ^^^^^^^^^^^
> I agree with you this would be a problem
> to clarify the NULL derefence would occur here within keypad_matrix_key_parse_dt
>
> struct device *dev = keypad_data->input_dev->dev.parent;
>
> > This assumes we have set "keypad_data->input_dev = input_dev;" but we
> > don't do that until...
> >
> >     157         if (error)
> >     158                 return error;
> >     159
> >     160         error = matrix_keypad_build_keymap(NULL, NULL,
> >     161                                            keypad_data->n_rows,
> >     162                                            keypad_data->n_cols,
> >     163                                            NULL, input_dev);
> >     164         if (error) {
> >     165                 dev_err(&pdev->dev, "failed to build keymap\n");
> >     166                 return error;
> >     167         }
> >     168
> >     169         input_set_drvdata(input_dev, keypad_data);
> >     170
> >     171         keypad_data->input_dev = input_dev;
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > this line here.  This driver has never worked and it was included almost
> > five years ago.  Is it worth fixing?
> >
> >     172
> >     173         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >     174         keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
> >     175         if (IS_ERR(keypad_data->base))
> >     176                 return PTR_ERR(keypad_data->base);
> >     177
> >
> > regards,
> > dan carpenter
> >
>
> Here is the interesting thing, I was looking on patchwork, and several
> of the patches including what appears to be the latest actually set
> "keypad_data->input_dev = input_dev" before calling
> "keypad_matrix_key_parse_dt"
>
> From v4 on patchwork
> + if (IS_ERR(keypad_data->clk)) {
> + dev_err(&pdev->dev, "cannot get clock");
> + return PTR_ERR(keypad_data->clk);
> + }
> +
> + keypad_data->input_dev = input_dev;
> +
> + input_dev->name = pdev->name;
> + input_dev->phys = "keyscan-keys/input0";
> + input_dev->dev.parent = &pdev->dev;
> + input_dev->open = keyscan_open;
> + input_dev->close = keyscan_close;
> +
> + input_dev->id.bustype = BUS_HOST;
> +
> + error = keypad_matrix_key_parse_dt(keypad_data);
>
> According to patchwork, these aren't listed as accepted, so I'm not
> sure where the exact accepted patch came from. Looking at the commit
> log, it looks like the issue you showed above was made in the original
> commit 062589b1399176a9c14bc68e16169f40439d658c so I'm not quite sure
> what is going on here. Maybe the maintainer can chime in with the
> original patch/mailing list discussion on this. For reference, I've
> added the patchwork links below
> https://patchwork.kernel.org/patch/3854341/
> https://patchwork.kernel.org/patch/3968891/
> https://patchwork.kernel.org/patch/3969991/

It may very well be that I messed up when applying the patch. I guess
whatever platform that is using the driver has not attempted to update
their kernel since then.

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [bug report] Input: add st-keyscan driver
From: Ken Sloat @ 2019-01-26 21:25 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: linux-input, Ken Sloat, gabriel.fernandez, dmitry.torokhov,
	linux-kernel, Nate Drude
In-Reply-To: <20190122185351.GA28627@kadam>

On Tue, Jan 22, 2019 at 1:53 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Gabriel FERNANDEZ,

Hello Dan,

I have added CCs for the maintainers as well as Gabriel Fernandez as
currently you just have the linux-input mailing list

> The patch 062589b13991: "Input: add st-keyscan driver" from Apr 12,
> 2014, leads to the following static checker warning:
>
>         drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
>         error: potential zalloc NULL dereference: 'keypad_data->input_dev'
>
> drivers/input/keyboard/st-keyscan.c
>     125 static int keyscan_probe(struct platform_device *pdev)
>     126 {
>     127         struct st_keyscan *keypad_data;
>     128         struct input_dev *input_dev;
>     129         struct resource *res;
>     130         int error;
>     131
>     132         if (!pdev->dev.of_node) {
>     133                 dev_err(&pdev->dev, "no DT data present\n");
>     134                 return -EINVAL;
>     135         }
>     136
>     137         keypad_data = devm_kzalloc(&pdev->dev, sizeof(*keypad_data),
>     138                                    GFP_KERNEL);
>     139         if (!keypad_data)
>     140                 return -ENOMEM;
>     141
>     142         input_dev = devm_input_allocate_device(&pdev->dev);
>     143         if (!input_dev) {
>     144                 dev_err(&pdev->dev, "failed to allocate the input device\n");
>     145                 return -ENOMEM;
>     146         }
>     147
>     148         input_dev->name = pdev->name;
>     149         input_dev->phys = "keyscan-keys/input0";
>     150         input_dev->dev.parent = &pdev->dev;
>     151         input_dev->open = keyscan_open;
>     152         input_dev->close = keyscan_close;
>     153
>     154         input_dev->id.bustype = BUS_HOST;
>     155
> --> 156         error = keypad_matrix_key_parse_dt(keypad_data);
>                                                    ^^^^^^^^^^^
I agree with you this would be a problem
to clarify the NULL derefence would occur here within keypad_matrix_key_parse_dt

struct device *dev = keypad_data->input_dev->dev.parent;

> This assumes we have set "keypad_data->input_dev = input_dev;" but we
> don't do that until...
>
>     157         if (error)
>     158                 return error;
>     159
>     160         error = matrix_keypad_build_keymap(NULL, NULL,
>     161                                            keypad_data->n_rows,
>     162                                            keypad_data->n_cols,
>     163                                            NULL, input_dev);
>     164         if (error) {
>     165                 dev_err(&pdev->dev, "failed to build keymap\n");
>     166                 return error;
>     167         }
>     168
>     169         input_set_drvdata(input_dev, keypad_data);
>     170
>     171         keypad_data->input_dev = input_dev;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> this line here.  This driver has never worked and it was included almost
> five years ago.  Is it worth fixing?
>
>     172
>     173         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>     174         keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
>     175         if (IS_ERR(keypad_data->base))
>     176                 return PTR_ERR(keypad_data->base);
>     177
>
> regards,
> dan carpenter
>

Here is the interesting thing, I was looking on patchwork, and several
of the patches including what appears to be the latest actually set
"keypad_data->input_dev = input_dev" before calling
"keypad_matrix_key_parse_dt"

>From v4 on patchwork
+ if (IS_ERR(keypad_data->clk)) {
+ dev_err(&pdev->dev, "cannot get clock");
+ return PTR_ERR(keypad_data->clk);
+ }
+
+ keypad_data->input_dev = input_dev;
+
+ input_dev->name = pdev->name;
+ input_dev->phys = "keyscan-keys/input0";
+ input_dev->dev.parent = &pdev->dev;
+ input_dev->open = keyscan_open;
+ input_dev->close = keyscan_close;
+
+ input_dev->id.bustype = BUS_HOST;
+
+ error = keypad_matrix_key_parse_dt(keypad_data);

According to patchwork, these aren't listed as accepted, so I'm not
sure where the exact accepted patch came from. Looking at the commit
log, it looks like the issue you showed above was made in the original
commit 062589b1399176a9c14bc68e16169f40439d658c so I'm not quite sure
what is going on here. Maybe the maintainer can chime in with the
original patch/mailing list discussion on this. For reference, I've
added the patchwork links below
https://patchwork.kernel.org/patch/3854341/
https://patchwork.kernel.org/patch/3968891/
https://patchwork.kernel.org/patch/3969991/

Thanks,
Ken Sloat

^ permalink raw reply

* Re: [PATCH] input: elan_i2c: Add ACPI ID for touchpad in Lenovo V130-15IGM
From: Tobias Vögeli @ 2019-01-26 21:14 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Tobias Vögeli, linux-input, linux-kernel
In-Reply-To: <20190126154856.14060-1-tobias@voegeli.com>

Not sure if the last mail went trough, so here again:

Forget this patch, I did some further testing and even though the touchpad works, it generates some dmesg messages. I have to take a closer look into this.

Sorry for the spam.

Tobias


> From: Tobias Vögeli <tobias@voegeli.com>
>
> Add ELAN061A to the ACPI table to support the touchpad of the Lenovo V130-15IGM.
>
> Signed-off-by: Tobias Vögeli <tobias@voegeli.com>
> ---
>   drivers/input/mouse/elan_i2c_core.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index f322a1768fbb..4998b7007201 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -1347,6 +1347,7 @@ static const struct acpi_device_id elan_acpi_id[] = {
>   	{ "ELAN0611", 0 },
>   	{ "ELAN0612", 0 },
>   	{ "ELAN0618", 0 },
> +	{ "ELAN061A", 0 },
>   	{ "ELAN061C", 0 },
>   	{ "ELAN061D", 0 },
>   	{ "ELAN061E", 0 },

^ permalink raw reply

* Re: [PATCH] input: elan_i2c: Add ACPI ID for touchpad in Lenovo V130-15IGM
From: Tobias Vögeli @ 2019-01-26 20:46 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Tobias Vögeli, linux-input, linux-kernel
In-Reply-To: <20190126154856.14060-1-tobias@voegeli.com>

Forget this patch, I did some further testing and even though the touchpad works, it generates some dmesg messages. I have to take a closer look into this.

Sorry for the spam.

Tobias

  

> From: Tobias Vögeli <tobias@voegeli.com>
>
> Add ELAN061A to the ACPI table to support the touchpad of the Lenovo V130-15IGM.
>
> Signed-off-by: Tobias Vögeli <tobias@voegeli.com>
> ---
>   drivers/input/mouse/elan_i2c_core.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index f322a1768fbb..4998b7007201 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -1347,6 +1347,7 @@ static const struct acpi_device_id elan_acpi_id[] = {
>   	{ "ELAN0611", 0 },
>   	{ "ELAN0612", 0 },
>   	{ "ELAN0618", 0 },
> +	{ "ELAN061A", 0 },
>   	{ "ELAN061C", 0 },
>   	{ "ELAN061D", 0 },
>   	{ "ELAN061E", 0 },

^ permalink raw reply

* [PATCH v2] HID: debug: fix the ring buffer implementation
From: Vladis Dronov @ 2019-01-26 17:07 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Oleg Nesterov, linux-input,
	linux-kernel
  Cc: Vladis Dronov, stable

Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
is strange allowing lost or corrupted data. After commit 717adfdaf147
("HID: debug: check length before copy_to_user()") it is possible to enter
an infinite loop in hid_debug_events_read() by providing 0 as count, this
locks up a system. Fix this by rewriting the ring buffer implementation
with kfifo and simplify the code.

This fixes CVE-2019-3819.

v2: fix an execution logic and add a comment

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1669187
Cc: stable@vger.kernel.org # v4.18+
Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
Fixes: 717adfdaf147 ("HID: debug: check length before copy_to_user()")
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
 drivers/hid/hid-debug.c   | 116 ++++++++++++++------------------------
 include/linux/hid-debug.h |   9 ++-
 2 files changed, 47 insertions(+), 78 deletions(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index c530476edba6..08870c909268 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -30,6 +30,7 @@
 
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/kfifo.h>
 #include <linux/sched/signal.h>
 #include <linux/export.h>
 #include <linux/slab.h>
@@ -661,17 +662,12 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
 /* enqueue string to 'events' ring buffer */
 void hid_debug_event(struct hid_device *hdev, char *buf)
 {
-	unsigned i;
 	struct hid_debug_list *list;
 	unsigned long flags;
 
 	spin_lock_irqsave(&hdev->debug_list_lock, flags);
-	list_for_each_entry(list, &hdev->debug_list, node) {
-		for (i = 0; buf[i]; i++)
-			list->hid_debug_buf[(list->tail + i) % HID_DEBUG_BUFSIZE] =
-				buf[i];
-		list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
-        }
+	list_for_each_entry(list, &hdev->debug_list, node)
+		kfifo_in(&list->hid_debug_fifo, buf, strlen(buf));
 	spin_unlock_irqrestore(&hdev->debug_list_lock, flags);
 
 	wake_up_interruptible(&hdev->debug_wait);
@@ -722,8 +718,7 @@ void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage, __s32 valu
 	hid_debug_event(hdev, buf);
 
 	kfree(buf);
-        wake_up_interruptible(&hdev->debug_wait);
-
+	wake_up_interruptible(&hdev->debug_wait);
 }
 EXPORT_SYMBOL_GPL(hid_dump_input);
 
@@ -1083,8 +1078,8 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
 		goto out;
 	}
 
-	if (!(list->hid_debug_buf = kzalloc(HID_DEBUG_BUFSIZE, GFP_KERNEL))) {
-		err = -ENOMEM;
+	err = kfifo_alloc(&list->hid_debug_fifo, HID_DEBUG_FIFOSIZE, GFP_KERNEL);
+	if (err) {
 		kfree(list);
 		goto out;
 	}
@@ -1104,77 +1099,57 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
 		size_t count, loff_t *ppos)
 {
 	struct hid_debug_list *list = file->private_data;
-	int ret = 0, len;
+	int ret = 0, copied;
 	DECLARE_WAITQUEUE(wait, current);
 
 	mutex_lock(&list->read_mutex);
-	while (ret == 0) {
-		if (list->head == list->tail) {
-			add_wait_queue(&list->hdev->debug_wait, &wait);
-			set_current_state(TASK_INTERRUPTIBLE);
-
-			while (list->head == list->tail) {
-				if (file->f_flags & O_NONBLOCK) {
-					ret = -EAGAIN;
-					break;
-				}
-				if (signal_pending(current)) {
-					ret = -ERESTARTSYS;
-					break;
-				}
-
-				if (!list->hdev || !list->hdev->debug) {
-					ret = -EIO;
-					set_current_state(TASK_RUNNING);
-					goto out;
-				}
-
-				/* allow O_NONBLOCK from other threads */
-				mutex_unlock(&list->read_mutex);
-				schedule();
-				mutex_lock(&list->read_mutex);
-				set_current_state(TASK_INTERRUPTIBLE);
-			}
-
-			set_current_state(TASK_RUNNING);
-			remove_wait_queue(&list->hdev->debug_wait, &wait);
-		}
-
-		if (ret)
-			goto out;
+	if (kfifo_is_empty(&list->hid_debug_fifo)) {
+		add_wait_queue(&list->hdev->debug_wait, &wait);
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		while (kfifo_is_empty(&list->hid_debug_fifo)) {
+			if (file->f_flags & O_NONBLOCK) {
+				ret = -EAGAIN;
+				break;
+			}
+
+			if (signal_pending(current)) {
+				ret = -ERESTARTSYS;
+				break;
+			}
+
+			/* if list->hdev is NULL we cannot remove_wait_queue().
+			 * if list->hdev->debug is 0 then hid_debug_unregister()
+			 * was already called and list->hdev is being destroyed.
+			 * if we add remove_wait_queue() here we can hit a race.
+			 */
+			if (!list->hdev || !list->hdev->debug) {
+				ret = -EIO;
+				set_current_state(TASK_RUNNING);
+				goto out;
+			}
+
+			/* allow O_NONBLOCK from other threads */
+			mutex_unlock(&list->read_mutex);
+			schedule();
+			mutex_lock(&list->read_mutex);
+			set_current_state(TASK_INTERRUPTIBLE);
+		}
+
+		set_current_state(TASK_RUNNING);
+		remove_wait_queue(&list->hdev->debug_wait, &wait);
+
+		if (ret)
+			goto out;
+	}
 
-		/* pass the ringbuffer contents to userspace */
-copy_rest:
-		if (list->tail == list->head)
-			goto out;
-		if (list->tail > list->head) {
-			len = list->tail - list->head;
-			if (len > count)
-				len = count;
-
-			if (copy_to_user(buffer + ret, &list->hid_debug_buf[list->head], len)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			ret += len;
-			list->head += len;
-		} else {
-			len = HID_DEBUG_BUFSIZE - list->head;
-			if (len > count)
-				len = count;
-
-			if (copy_to_user(buffer, &list->hid_debug_buf[list->head], len)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			list->head = 0;
-			ret += len;
-			count -= len;
-			if (count > 0)
-				goto copy_rest;
-		}
-
-	}
+	/* pass the fifo content to userspace, locking is not needed with only
+	 * one concurrent reader and one concurrent writer
+	 */
+	ret = kfifo_to_user(&list->hid_debug_fifo, buffer, count, &copied);
+	if (ret)
+		goto out;
+	ret = copied;
 out:
 	mutex_unlock(&list->read_mutex);
 	return ret;
@@ -1185,7 +1160,7 @@ static __poll_t hid_debug_events_poll(struct file *file, poll_table *wait)
 	struct hid_debug_list *list = file->private_data;
 
 	poll_wait(file, &list->hdev->debug_wait, wait);
-	if (list->head != list->tail)
+	if (!kfifo_is_empty(&list->hid_debug_fifo))
 		return EPOLLIN | EPOLLRDNORM;
 	if (!list->hdev->debug)
 		return EPOLLERR | EPOLLHUP;
@@ -1200,7 +1175,7 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
 	spin_lock_irqsave(&list->hdev->debug_list_lock, flags);
 	list_del(&list->node);
 	spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
-	kfree(list->hid_debug_buf);
+	kfifo_free(&list->hid_debug_fifo);
 	kfree(list);
 
 	return 0;
@@ -1246,4 +1221,3 @@ void hid_debug_exit(void)
 {
 	debugfs_remove_recursive(hid_debug_root);
 }
-
diff --git a/include/linux/hid-debug.h b/include/linux/hid-debug.h
index 8663f216c563..e7a7c92aaf09 100644
--- a/include/linux/hid-debug.h
+++ b/include/linux/hid-debug.h
@@ -24,7 +24,10 @@
 
 #ifdef CONFIG_DEBUG_FS
 
+#include <linux/kfifo.h>
+
 #define HID_DEBUG_BUFSIZE 512
+#define HID_DEBUG_FIFOSIZE 512
 
 void hid_dump_input(struct hid_device *, struct hid_usage *, __s32);
 void hid_dump_report(struct hid_device *, int , u8 *, int);
@@ -38,10 +41,7 @@ void hid_debug_event(struct hid_device *, char *);
 void hid_debug_event(struct hid_device *, char *);
 
-
 struct hid_debug_list {
-	char *hid_debug_buf;
-	int head;
-	int tail;
+	DECLARE_KFIFO_PTR(hid_debug_fifo, char);
 	struct fasync_struct *fasync;
 	struct hid_device *hdev;
 	struct list_head node;
@@ -64,4 +64,3 @@ struct hid_debug_list {
 #endif
 
 #endif
-

^ permalink raw reply related

* [PATCH] input: elan_i2c: Add ACPI ID for touchpad in Lenovo V130-15IGM
From: Tobias Vögeli @ 2019-01-26 15:48 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Tobias Vögeli, linux-input, linux-kernel

From: Tobias Vögeli <tobias@voegeli.com>

Add ELAN061A to the ACPI table to support the touchpad of the Lenovo V130-15IGM.

Signed-off-by: Tobias Vögeli <tobias@voegeli.com>
---
 drivers/input/mouse/elan_i2c_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index f322a1768fbb..4998b7007201 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1347,6 +1347,7 @@ static const struct acpi_device_id elan_acpi_id[] = {
 	{ "ELAN0611", 0 },
 	{ "ELAN0612", 0 },
 	{ "ELAN0618", 0 },
+	{ "ELAN061A", 0 },
 	{ "ELAN061C", 0 },
 	{ "ELAN061D", 0 },
 	{ "ELAN061E", 0 },
-- 
2.20.1

^ permalink raw reply related

* Re: [PATCH] HID: debug: fix the ring buffer implementation
From: Vladis Dronov @ 2019-01-26 15:44 UTC (permalink / raw)
  To: Oleg Nesterov, Jiri Kosina, Benjamin Tissoires
  Cc: linux-input, linux-kernel, stable
In-Reply-To: <20190125130113.GA18589@redhat.com>

Hello, Oleg, all,

Thanks much, Oleg, these are really valuable additions:

> suspicious ;) if you add a comment the patch will be even better.

Great, will do.

> perhaps it make sense to move this check into the "if (kfifo_is_empty())"
> block.

Indeed, otherwise it kinda breaks the execution logic.

> is kfifo_is_empty() == T really possible here?

It looks like it is not. Reads are guarded by read_mutex and the only other
code which touches hid_debug_fifo is writer.

I will post v2 here soon. Lets see if it is ready for inclusion.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security | Senior Software Engineer

^ permalink raw reply

* [PATCH 3/3] ARM: dts: lpc32xx: reparent keypad controller to SIC1
From: Vladimir Zapolskiy @ 2019-01-26 14:29 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring
  Cc: devicetree, linux-input, linux-arm-kernel, Sylvain Lemieux
In-Reply-To: <20190126142921.16041-1-vz@mleia.com>

After switching to a new interrupt controller scheme by separating SIC1
and SIC2 from MIC interrupt controller just one SoC keypad controller
was not taken into account, fix it now:

  WARNING: CPU: 0 PID: 1 at kernel/irq/irqdomain.c:524 irq_domain_associate+0x50/0x1b0
  error: hwirq 0x36 is too large for interrupt-controller@40008000
  ...
  lpc32xx_keys 40050000.key: failed to get platform irq
  lpc32xx_keys: probe of 40050000.key failed with error -22

Fixes: 9b8ad3fb81ae ("ARM: dts: lpc32xx: reparent SIC1 and SIC2 interrupts from MIC")
Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/boot/dts/lpc32xx.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
index d4368eeff1b9..4f8f671c3343 100644
--- a/arch/arm/boot/dts/lpc32xx.dtsi
+++ b/arch/arm/boot/dts/lpc32xx.dtsi
@@ -463,7 +463,8 @@
 				compatible = "nxp,lpc3220-key";
 				reg = <0x40050000 0x1000>;
 				clocks = <&clk LPC32XX_CLK_KEY>;
-				interrupts = <54 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&sic1>;
+				interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;
 				status = "disabled";
 			};
 
-- 
2.20.1

^ permalink raw reply related

* [PATCH 2/3] ARM: dts: lpc32xx: add required clocks property to keypad device node
From: Vladimir Zapolskiy @ 2019-01-26 14:29 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring
  Cc: devicetree, linux-input, linux-arm-kernel, Sylvain Lemieux
In-Reply-To: <20190126142921.16041-1-vz@mleia.com>

NXP LPC32xx keypad controller requires a clock property to be defined.

The change fixes the driver initialization problem:

  lpc32xx_keys 40050000.key: failed to get clock
  lpc32xx_keys: probe of 40050000.key failed with error -2

Fixes: 93898eb775e5 ("arm: dts: lpc32xx: add clock properties to device nodes")
Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/boot/dts/lpc32xx.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
index ed0d6fb20122..d4368eeff1b9 100644
--- a/arch/arm/boot/dts/lpc32xx.dtsi
+++ b/arch/arm/boot/dts/lpc32xx.dtsi
@@ -462,6 +462,7 @@
 			key: key@40050000 {
 				compatible = "nxp,lpc3220-key";
 				reg = <0x40050000 0x1000>;
+				clocks = <&clk LPC32XX_CLK_KEY>;
 				interrupts = <54 IRQ_TYPE_LEVEL_HIGH>;
 				status = "disabled";
 			};
-- 
2.20.1

^ permalink raw reply related

* [PATCH 1/3] Input: lpc32xx-key - add clocks property and fix DT binding example
From: Vladimir Zapolskiy @ 2019-01-26 14:29 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring
  Cc: devicetree, linux-input, linux-arm-kernel, Sylvain Lemieux
In-Reply-To: <20190126142921.16041-1-vz@mleia.com>

The keypad controller on NXP LPC32xx requires its clock gate to be open,
therefore add description of the requires 'clocks' property.

In addition adjust the example by adding description of required 'clocks'
property and by fixing 'interrupts' property.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 Documentation/devicetree/bindings/input/lpc32xx-key.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/lpc32xx-key.txt b/Documentation/devicetree/bindings/input/lpc32xx-key.txt
index bcf62f856358..2b075a080d30 100644
--- a/Documentation/devicetree/bindings/input/lpc32xx-key.txt
+++ b/Documentation/devicetree/bindings/input/lpc32xx-key.txt
@@ -8,6 +8,7 @@ Required Properties:
 - reg: Physical base address of the controller and length of memory mapped
   region.
 - interrupts: The interrupt number to the cpu.
+- clocks: phandle to clock controller plus clock-specifier pair
 - nxp,debounce-delay-ms: Debounce delay in ms
 - nxp,scan-delay-ms: Repeated scan period in ms
 - linux,keymap: the key-code to be reported when the key is pressed
@@ -22,7 +23,9 @@ Example:
 	key@40050000 {
 		compatible = "nxp,lpc3220-key";
 		reg = <0x40050000 0x1000>;
-		interrupts = <54 0>;
+		clocks = <&clk LPC32XX_CLK_KEY>;
+		interrupt-parent = <&sic1>;
+		interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;
 		keypad,num-rows = <1>;
 		keypad,num-columns = <1>;
 		nxp,debounce-delay-ms = <3>;
-- 
2.20.1

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox