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;
MIME-Version: 1.0
In-Reply-To: <YJnllh7GfuVlL3ze@google.com>
References: <20210510220012.2003285-1-swboyd@chromium.org> <YJnllh7GfuVlL3ze@google.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.9.1
Date:   Mon, 10 May 2021 22:11:21 -0700
Message-ID: <CAE-0n539o_DWqHbPuarWozk4Rev_d++2Da=AvOYALwvB1j3dVA@mail.gmail.com>
Subject: Re: [PATCH] Input: elan_i2c: Disable irq on shutdown
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        Jingle Wu <jingle.wu@emc.com.tw>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Dmitry Torokhov (2021-05-10 19:01:58)
> 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.

It sounds nice if we don't have to play whack-a-mole, except for the
part where the irq is requested in this driver via
devm_request_threaded_irq(). The i2c bus code doesn't request the irq,
so it doesn't enable it, hence the responsibility of enabling and
disabling the irq is on the driver. Maybe another option would be to
disable all device irqs, similar to how that is done during suspend, but
then we would need a split shutdown flow where there's irq enabled
shutdown and irq disabled shutdown callbacks. That would be a large
change. Similarly, disabling it in the i2c bus code would be a large
change that would mean auditing each i2c driver shutdown function to
make sure we don't disable the irq more than the number of times it has
been enabled. Please don't make me shave this yak.

>
> >
> > 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.

Alright sure. I was worried about some process that may still be running
given that shutdown/restart doesn't actually stop/freeze all processes
like suspend does. If you're not worried about something like a firmware
update happening in parallel with the restart syscall then OK. Cutting
those things off may mean they don't work properly, but we're already
shutting down so all bets are off.