Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 10 May 2021 19:01:58 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        Jingle Wu <jingle.wu@emc.com.tw>
Subject: Re: [PATCH] Input: elan_i2c: Disable irq on shutdown
Message-ID: <YJnllh7GfuVlL3ze@google.com>
References: <20210510220012.2003285-1-swboyd@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210510220012.2003285-1-swboyd@chromium.org>
Precedence: bulk

Hi Stephen,

On Mon, May 10, 2021 at 03:00:12PM -0700, Stephen Boyd wrote:
> Touching an elan trackpad while shutting down the system sometimes leads
> to the following warning from i2c core. This is because the irq is still
> active and working, but the i2c bus for the device has been shutdown
> already. If the bus has been taken down then we shouldn't expect
> transfers to work. Disable the irq on shutdown so that this driver
> doesn't try to get the report in the irq handler after the i2c bus is
> shutdown.
> 
>  i2c i2c-7: Transfer while suspended
>  WARNING: CPU: 0 PID: 196 at drivers/i2c/i2c-core.h:54 __i2c_transfer+0xb8/0x38c
>  Modules linked in: rfcomm algif_hash algif_skcipher af_alg uinput xt_cgroup
>  CPU: 0 PID: 196 Comm: irq/166-ekth300 Not tainted 5.4.115 #96
>  Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
>  pstate: 60c00009 (nZCv daif +PAN +UAO)
>  pc : __i2c_transfer+0xb8/0x38c
>  lr : __i2c_transfer+0xb8/0x38c
>  sp : ffffffc011793c20
>  x29: ffffffc011793c20 x28: 0000000000000000
>  x27: ffffff85efd60348 x26: ffffff85efdb8040
>  x25: ffffffec39d579cc x24: ffffffec39d57bac
>  x23: ffffffec3aab17b9 x22: ffffff85f02d6400
>  x21: 0000000000000001 x20: ffffff85f02aa190
>  x19: ffffff85f02aa100 x18: 00000000ffff0a10
>  x17: 0000000000000044 x16: 00000000000000ec
>  x15: ffffffec3a0b9174 x14: 0000000000000006
>  x13: 00000000003fe680 x12: 0000000000000000
>  x11: 0000000000000000 x10: 00000000ffffffff
>  x9 : 806da3cb9f8c1d00 x8 : 806da3cb9f8c1d00
>  x7 : 0000000000000000 x6 : ffffffec3afd3bef
>  x5 : 0000000000000000 x4 : 0000000000000000
>  x3 : 0000000000000000 x2 : fffffffffffffcc7
>  x1 : 0000000000000000 x0 : 0000000000000023
>  Call trace:
>   __i2c_transfer+0xb8/0x38c
>   i2c_transfer+0xa0/0xf4
>   i2c_transfer_buffer_flags+0x64/0x98
>   elan_i2c_get_report+0x2c/0x88
>   elan_isr+0x68/0x3e4
>   irq_thread_fn+0x2c/0x70
>   irq_thread+0xf8/0x148
>   kthread+0x140/0x17c
>   ret_from_fork+0x10/0x18

This does not seem to me that it is Elan-specific issue. I wonder if
this should be pushed into I2C core to shut off client->irq in shutdown
for everyone.

> 
> Cc: Jingle Wu <jingle.wu@emc.com.tw>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index bef73822315d..6f64992e70d1 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -1338,6 +1338,22 @@ static int elan_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static void elan_shutdown(struct i2c_client *client)
> +{
> +	struct elan_tp_data *data = i2c_get_clientdata(client);
> +
> +	/*
> +	 * Make sure we don't access i2c bus after it is shutdown.
> +	 *
> +	 * We are taking the mutex to make sure sysfs operations are
> +	 * complete before we attempt to silence the device by disabling
> +	 * the irq.
> +	 */

I do not think important on shutdown as I expect we'll stop/kill
userspace first, and then fully reinitialize device on subsequent boot.

Thanks.

-- 
Dmitry