| Message ID | 20210510220012.2003285-1-swboyd@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | Input: elan_i2c: Disable irq on shutdown | expand |
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
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.
Hi Stephen, Sorry for the long delay with the response. On Mon, May 10, 2021 at 10:11:21PM -0700, Stephen Boyd wrote: > 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. There is purity, and there is practicality. Drivers normally prepare device for suspend and waking up from suspend, however I2C core does handle enabling interrupt as wakeup source because this saves on boilerplate. I2C core already does a lot of preparing for interrupt being requested by drivers (parsing ACPI and DT tables, etc). > 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. Yes, that would be quite large patch, and probably not needed for devices/buses that do not use out of bound signalling of attention. > 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. I do not think keeping counter balanced would be important here, as we are shutting down, and upon reboot everything will be reinitialized from scratch. Also, we are lucky in that there is just a handful of I2C drivers defining shutdown() methods. > Please don't make me shave this yak. I'm afraid someone has to... I'm adding Wolfram to CC to get his take on this. Thanks. -- Dmitry
Quoting Dmitry Torokhov (2021-06-02 18:18:17) > Hi Stephen, > > Sorry for the long delay with the response. > > On Mon, May 10, 2021 at 10:11:21PM -0700, Stephen Boyd wrote: > > 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. > > There is purity, and there is practicality. Drivers normally prepare > device for suspend and waking up from suspend, however I2C core does > handle enabling interrupt as wakeup source because this saves on > boilerplate. I2C core already does a lot of preparing for interrupt > being requested by drivers (parsing ACPI and DT tables, etc). > > > 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. > > Yes, that would be quite large patch, and probably not needed for > devices/buses that do not use out of bound signalling of attention. > > > 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. > > I do not think keeping counter balanced would be important here, as we > are shutting down, and upon reboot everything will be reinitialized from > scratch. Also, we are lucky in that there is just a handful of I2C > drivers defining shutdown() methods. > > > Please don't make me shave this yak. > > I'm afraid someone has to... I'm adding Wolfram to CC to get his take on > this. > I suppose another option would be to introduce some common function that i2c drivers can use for their shutdown op, like i2c_generic_shutdown() that would disable the irq? I would guess that it isn't a great idea to blanket disable the irq in case some i2c driver wants to do something that may require that irq to come in once more during shutdown to signal that things are off or something like that. Would having this common function that this driver opts into work for you?
On Wed, Jun 02, 2021 at 06:33:49PM -0700, Stephen Boyd wrote: > Quoting Dmitry Torokhov (2021-06-02 18:18:17) > > Hi Stephen, > > > > Sorry for the long delay with the response. > > > > On Mon, May 10, 2021 at 10:11:21PM -0700, Stephen Boyd wrote: > > > 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. > > > > There is purity, and there is practicality. Drivers normally prepare > > device for suspend and waking up from suspend, however I2C core does > > handle enabling interrupt as wakeup source because this saves on > > boilerplate. I2C core already does a lot of preparing for interrupt > > being requested by drivers (parsing ACPI and DT tables, etc). > > > > > 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. > > > > Yes, that would be quite large patch, and probably not needed for > > devices/buses that do not use out of bound signalling of attention. > > > > > 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. > > > > I do not think keeping counter balanced would be important here, as we > > are shutting down, and upon reboot everything will be reinitialized from > > scratch. Also, we are lucky in that there is just a handful of I2C > > drivers defining shutdown() methods. > > > > > Please don't make me shave this yak. > > > > I'm afraid someone has to... I'm adding Wolfram to CC to get his take on > > this. > > > > I suppose another option would be to introduce some common function that > i2c drivers can use for their shutdown op, like i2c_generic_shutdown() > that would disable the irq? I would guess that it isn't a great idea to > blanket disable the irq in case some i2c driver wants to do something > that may require that irq to come in once more during shutdown to signal > that things are off or something like that. > > Would having this common function that this driver opts into work for > you? Opting in in this fashion will still require changes in the majority of drivers (any I2C touchscreen or touchpad may be touched while system is being shut down, so all of them will need to have interrupt freed or disabled, or they may initiate I2C transfer). How about something like this; diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 38107c0c318c..c835e7bb71de 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -603,9 +603,12 @@ static void i2c_device_shutdown(struct device *dev) if (!client || !dev->driver) return; + driver = to_i2c_driver(dev->driver); if (driver->shutdown) driver->shutdown(client); + else if (client->irq > 0) + disable_irq(client->irq); } static void i2c_client_dev_release(struct device *dev) Thanks. -- Dmitry
Quoting Dmitry Torokhov (2021-06-02 21:33:41) > On Wed, Jun 02, 2021 at 06:33:49PM -0700, Stephen Boyd wrote: > > Quoting Dmitry Torokhov (2021-06-02 18:18:17) > > > > > > I do not think keeping counter balanced would be important here, as we > > > are shutting down, and upon reboot everything will be reinitialized from > > > scratch. Also, we are lucky in that there is just a handful of I2C > > > drivers defining shutdown() methods. > > > > > > > Please don't make me shave this yak. > > > > > > I'm afraid someone has to... I'm adding Wolfram to CC to get his take on > > > this. > > > > > > > I suppose another option would be to introduce some common function that > > i2c drivers can use for their shutdown op, like i2c_generic_shutdown() > > that would disable the irq? I would guess that it isn't a great idea to > > blanket disable the irq in case some i2c driver wants to do something > > that may require that irq to come in once more during shutdown to signal > > that things are off or something like that. > > > > Would having this common function that this driver opts into work for > > you? > > Opting in in this fashion will still require changes in the majority > of drivers (any I2C touchscreen or touchpad may be touched while system > is being shut down, so all of them will need to have interrupt freed or > disabled, or they may initiate I2C transfer). How about something like > this; Yes, this approach should work. I assume a device that doesn't have a shutdown function will be happy to let the i2c core disable the irq for it so it looks low risk. Will you send a proper patch to Wolfram or would you like me to wrap it up and resend? > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 38107c0c318c..c835e7bb71de 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -603,9 +603,12 @@ static void i2c_device_shutdown(struct device *dev) > > if (!client || !dev->driver) > return; > + > driver = to_i2c_driver(dev->driver); > if (driver->shutdown) > driver->shutdown(client); > + else if (client->irq > 0) > + disable_irq(client->irq); > } > > static void i2c_client_dev_release(struct device *dev) >
On Wed, Jun 02, 2021 at 11:13:40PM -0700, Stephen Boyd wrote: > Quoting Dmitry Torokhov (2021-06-02 21:33:41) > > On Wed, Jun 02, 2021 at 06:33:49PM -0700, Stephen Boyd wrote: > > > Quoting Dmitry Torokhov (2021-06-02 18:18:17) > > > > > > > > I do not think keeping counter balanced would be important here, as we > > > > are shutting down, and upon reboot everything will be reinitialized from > > > > scratch. Also, we are lucky in that there is just a handful of I2C > > > > drivers defining shutdown() methods. > > > > > > > > > Please don't make me shave this yak. > > > > > > > > I'm afraid someone has to... I'm adding Wolfram to CC to get his take on > > > > this. > > > > > > > > > > I suppose another option would be to introduce some common function that > > > i2c drivers can use for their shutdown op, like i2c_generic_shutdown() > > > that would disable the irq? I would guess that it isn't a great idea to > > > blanket disable the irq in case some i2c driver wants to do something > > > that may require that irq to come in once more during shutdown to signal > > > that things are off or something like that. > > > > > > Would having this common function that this driver opts into work for > > > you? > > > > Opting in in this fashion will still require changes in the majority > > of drivers (any I2C touchscreen or touchpad may be touched while system > > is being shut down, so all of them will need to have interrupt freed or > > disabled, or they may initiate I2C transfer). How about something like > > this; > > Yes, this approach should work. I assume a device that doesn't have a > shutdown function will be happy to let the i2c core disable the irq for > it so it looks low risk. > > Will you send a proper patch to Wolfram or would you like me to wrap it > up and resend? If you could do that I'd be more than happy. Thanks. -- Dmitry
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. + */ + mutex_lock(&data->sysfs_mutex); + disable_irq(client->irq); + mutex_unlock(&data->sysfs_mutex); +} + static int __maybe_unused elan_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -1423,6 +1439,7 @@ static struct i2c_driver elan_driver = { .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, .probe = elan_probe, + .shutdown = elan_shutdown, .id_table = elan_id, };
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 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(+) base-commit: 9f4ad9e425a1d3b6a34617b8ea226d56a119a717