diff mbox series

Input: cyapa - do not call input_device_enabled from power mode handler

Message ID X9MbMja+TEfbKkmJ@google.com (mailing list archive)
State Not Applicable
Headers show
Series Input: cyapa - do not call input_device_enabled from power mode handler | expand

Commit Message

Dmitry Torokhov Dec. 11, 2020, 7:09 a.m. UTC
Input device's user counter is supposed to be accessed only while holding
input->mutex.  Commit d69f0a43c677 ("Input: use input_device_enabled()")
recently switched cyapa to using the dedicated API and it uncovered the
fact that cyapa driver violated this constraint.

This patch removes checks whether the input device is open when clearing
device queues when changing device's power mode as there is no harm in
sending input events through closed input device - the events will simply
be dropped by the input core.

Note that there are more places in cyapa driver that call
input_device_enabled() without holding input->mutex, those are left
unfixed for now.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Marek, could you please try this one?

 drivers/input/mouse/cyapa_gen3.c |    5 +----
 drivers/input/mouse/cyapa_gen5.c |    3 +--
 2 files changed, 2 insertions(+), 6 deletions(-)

Comments

Marek Szyprowski Dec. 11, 2020, 8:22 a.m. UTC | #1
On 11.12.2020 08:09, Dmitry Torokhov wrote:
> Input device's user counter is supposed to be accessed only while holding
> input->mutex.  Commit d69f0a43c677 ("Input: use input_device_enabled()")
> recently switched cyapa to using the dedicated API and it uncovered the
> fact that cyapa driver violated this constraint.
>
> This patch removes checks whether the input device is open when clearing
> device queues when changing device's power mode as there is no harm in
> sending input events through closed input device - the events will simply
> be dropped by the input core.
>
> Note that there are more places in cyapa driver that call
> input_device_enabled() without holding input->mutex, those are left
> unfixed for now.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> Marek, could you please try this one?

The warning is still there:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 1787 at drivers/input/input.c:2230 
input_device_enabled+0x68/0x6c
Modules linked in: cmac bnep mwifiex_sdio mwifiex sha256_generic 
libsha256 sha256_arm btmrvl_sdio btmrvl cfg80211 bluetooth s5p_mfc 
exynos_gsc v4l2_mem2mem videob
CPU: 1 PID: 1787 Comm: rtcwake Not tainted 
5.10.0-rc7-next-20201210-00001-g70a81f43fddf #2204
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c0111a58>] (unwind_backtrace) from [<c010d390>] (show_stack+0x10/0x14)
[<c010d390>] (show_stack) from [<c0b3503c>] (dump_stack+0xb4/0xd4)
[<c0b3503c>] (dump_stack) from [<c0127428>] (__warn+0xd8/0x11c)
[<c0127428>] (__warn) from [<c012751c>] (warn_slowpath_fmt+0xb0/0xb8)
[<c012751c>] (warn_slowpath_fmt) from [<c07fbccc>] 
(input_device_enabled+0x68/0x6c)
[<c07fbccc>] (input_device_enabled) from [<c080a204>] 
(cyapa_reinitialize+0x4c/0x154)
[<c080a204>] (cyapa_reinitialize) from [<c080a354>] (cyapa_resume+0x48/0x98)
[<c080a354>] (cyapa_resume) from [<c06b0230>] (dpm_run_callback+0xb0/0x3c8)
[<c06b0230>] (dpm_run_callback) from [<c06b0604>] (device_resume+0xbc/0x260)
[<c06b0604>] (device_resume) from [<c06b29e4>] (dpm_resume+0x14c/0x51c)
[<c06b29e4>] (dpm_resume) from [<c06b35b8>] (dpm_resume_end+0xc/0x18)
[<c06b35b8>] (dpm_resume_end) from [<c01a1270>] 
(suspend_devices_and_enter+0x1b4/0xbd4)
[<c01a1270>] (suspend_devices_and_enter) from [<c01a1fa4>] 
(pm_suspend+0x314/0x42c)
[<c01a1fa4>] (pm_suspend) from [<c019fe90>] (state_store+0x6c/0xc8)
[<c019fe90>] (state_store) from [<c0388438>] (kernfs_fop_write+0x10c/0x228)
[<c0388438>] (kernfs_fop_write) from [<c02dc3e8>] (vfs_write+0xc8/0x530)
[<c02dc3e8>] (vfs_write) from [<c02dc98c>] (ksys_write+0x60/0xd8)
[<c02dc98c>] (ksys_write) from [<c0100060>] (ret_fast_syscall+0x0/0x2c)
Exception stack(0xc3923fa8 to 0xc3923ff0)
irq event stamp: 54139
hardirqs last  enabled at (54147): [<c01a5f20>] vprintk_emit+0x2b8/0x308
hardirqs last disabled at (54154): [<c01a5ee4>] vprintk_emit+0x27c/0x308
softirqs last  enabled at (50722): [<c01017a8>] __do_softirq+0x528/0x684
softirqs last disabled at (50671): [<c0130ac8>] irq_exit+0x1ec/0x1f8
---[ end trace 1fbefe3f239ae597 ]---

>   drivers/input/mouse/cyapa_gen3.c |    5 +----
>   drivers/input/mouse/cyapa_gen5.c |    3 +--
>   2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
> index a97f4acb6452..4a9022faf945 100644
> --- a/drivers/input/mouse/cyapa_gen3.c
> +++ b/drivers/input/mouse/cyapa_gen3.c
> @@ -907,7 +907,6 @@ static u16 cyapa_get_wait_time_for_pwr_cmd(u8 pwr_mode)
>   static int cyapa_gen3_set_power_mode(struct cyapa *cyapa, u8 power_mode,
>   		u16 always_unused, enum cyapa_pm_stage pm_stage)
>   {
> -	struct input_dev *input = cyapa->input;
>   	u8 power;
>   	int tries;
>   	int sleep_time;
> @@ -953,7 +952,6 @@ static int cyapa_gen3_set_power_mode(struct cyapa *cyapa, u8 power_mode,
>   	 * depending on the command's content.
>   	 */
>   	if (cyapa->operational &&
> -	    input && input_device_enabled(input) &&
>   	    (pm_stage == CYAPA_PM_RUNTIME_SUSPEND ||
>   	     pm_stage == CYAPA_PM_RUNTIME_RESUME)) {
>   		/* Try to polling in 120Hz, read may fail, just ignore it. */
> @@ -1223,8 +1221,7 @@ static int cyapa_gen3_try_poll_handler(struct cyapa *cyapa)
>   	    (data.finger_btn & OP_DATA_VALID) != OP_DATA_VALID)
>   		return -EINVAL;
>   
> -	return cyapa_gen3_event_process(cyapa, &data);
> -
> +	return cyapa->input ? cyapa_gen3_event_process(cyapa, &data) : 0;
>   }
>   
>   static int cyapa_gen3_initialize(struct cyapa *cyapa) { return 0; }
> diff --git a/drivers/input/mouse/cyapa_gen5.c b/drivers/input/mouse/cyapa_gen5.c
> index abf42f77b4c5..afc5aa4dcf47 100644
> --- a/drivers/input/mouse/cyapa_gen5.c
> +++ b/drivers/input/mouse/cyapa_gen5.c
> @@ -518,8 +518,7 @@ int cyapa_empty_pip_output_data(struct cyapa *cyapa,
>   			*len = length;
>   			/* Response found, success. */
>   			return 0;
> -		} else if (cyapa->operational &&
> -			   input && input_device_enabled(input) &&
> +		} else if (cyapa->operational && input &&
>   			   (pm_stage == CYAPA_PM_RUNTIME_RESUME ||
>   			    pm_stage == CYAPA_PM_RUNTIME_SUSPEND)) {
>   			/* Parse the data and report it if it's valid. */
>
Best regards
Dmitry Torokhov Dec. 11, 2020, 8:31 a.m. UTC | #2
On Fri, Dec 11, 2020 at 09:22:44AM +0100, Marek Szyprowski wrote:
> 
> On 11.12.2020 08:09, Dmitry Torokhov wrote:
> > Input device's user counter is supposed to be accessed only while holding
> > input->mutex.  Commit d69f0a43c677 ("Input: use input_device_enabled()")
> > recently switched cyapa to using the dedicated API and it uncovered the
> > fact that cyapa driver violated this constraint.
> >
> > This patch removes checks whether the input device is open when clearing
> > device queues when changing device's power mode as there is no harm in
> > sending input events through closed input device - the events will simply
> > be dropped by the input core.
> >
> > Note that there are more places in cyapa driver that call
> > input_device_enabled() without holding input->mutex, those are left
> > unfixed for now.
> >
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >
> > Marek, could you please try this one?
> 
> The warning is still there:

Ah, yes, we are hitting another instance right after setting power mode.
I need to think more how to handle that one.

Thanks.
diff mbox series

Patch

diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
index a97f4acb6452..4a9022faf945 100644
--- a/drivers/input/mouse/cyapa_gen3.c
+++ b/drivers/input/mouse/cyapa_gen3.c
@@ -907,7 +907,6 @@  static u16 cyapa_get_wait_time_for_pwr_cmd(u8 pwr_mode)
 static int cyapa_gen3_set_power_mode(struct cyapa *cyapa, u8 power_mode,
 		u16 always_unused, enum cyapa_pm_stage pm_stage)
 {
-	struct input_dev *input = cyapa->input;
 	u8 power;
 	int tries;
 	int sleep_time;
@@ -953,7 +952,6 @@  static int cyapa_gen3_set_power_mode(struct cyapa *cyapa, u8 power_mode,
 	 * depending on the command's content.
 	 */
 	if (cyapa->operational &&
-	    input && input_device_enabled(input) &&
 	    (pm_stage == CYAPA_PM_RUNTIME_SUSPEND ||
 	     pm_stage == CYAPA_PM_RUNTIME_RESUME)) {
 		/* Try to polling in 120Hz, read may fail, just ignore it. */
@@ -1223,8 +1221,7 @@  static int cyapa_gen3_try_poll_handler(struct cyapa *cyapa)
 	    (data.finger_btn & OP_DATA_VALID) != OP_DATA_VALID)
 		return -EINVAL;
 
-	return cyapa_gen3_event_process(cyapa, &data);
-
+	return cyapa->input ? cyapa_gen3_event_process(cyapa, &data) : 0;
 }
 
 static int cyapa_gen3_initialize(struct cyapa *cyapa) { return 0; }
diff --git a/drivers/input/mouse/cyapa_gen5.c b/drivers/input/mouse/cyapa_gen5.c
index abf42f77b4c5..afc5aa4dcf47 100644
--- a/drivers/input/mouse/cyapa_gen5.c
+++ b/drivers/input/mouse/cyapa_gen5.c
@@ -518,8 +518,7 @@  int cyapa_empty_pip_output_data(struct cyapa *cyapa,
 			*len = length;
 			/* Response found, success. */
 			return 0;
-		} else if (cyapa->operational &&
-			   input && input_device_enabled(input) &&
+		} else if (cyapa->operational && input &&
 			   (pm_stage == CYAPA_PM_RUNTIME_RESUME ||
 			    pm_stage == CYAPA_PM_RUNTIME_SUSPEND)) {
 			/* Parse the data and report it if it's valid. */