| Message ID | 20240730180903.81688-1-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] mfd: rk8xx: Fix shutdown handler | expand |
Hi, On Tuesday, 30 July 2024 at 9:05 PM, Sebastian Reichel <sebastian.reichel@collabora.com> wrote: > To avoid this we keep the SYS_OFF_MODE_POWER_OFF_PREPARE handler for the > SPI backend. This is not great, but at least avoids regressions and the > fix should be small enough to allow backporting. > > As a side-effect this also works around a shutdown problem on the Asus > C201. For reasons unknown that skips calling the prepare handler and > directly calls the final shutdown handler. I can clarify that bit a little - basically, when the handler is registered as a poweroff prepare handler, the kernel thinks that the C201 cannot poweroff and converts the poweroff into a halt, so I assume the power_off_handler_list is empty - which makes sense, the C201 has no firmware to call or other poweroff methods that I'd be aware of. (See kernel/reboot.c line 746 and 683.) Anyways, this patch works on the C201 (tested on top of 6.11-rc1). Tested-by: Urja <urja@urja.dev> Thank you -- Urja Rannikko
Am Dienstag, 30. Juli 2024, 20:05:05 CEST schrieb Sebastian Reichel: > When I converted rk808 to device managed resources I converted the rk808 > specific pm_power_off handler to devm_register_sys_off_handler() using > SYS_OFF_MODE_POWER_OFF_PREPARE, which is allowed to sleep. I did this > because the driver's poweroff function makes use of regmap and the backend > of that might sleep. > > But the PMIC poweroff function will kill off the board power and the > kernel does some extra steps after the prepare handler. Thus the prepare > handler should not be used for the PMIC's poweroff routine. Instead the > normal SYS_OFF_MODE_POWER_OFF phase should be used. The old pm_power_off > method is also being called from there, so this would have been a > cleaner conversion anyways. > > But it still makes sense to investigate the sleep handling and check > if there are any issues. Apparently the Rockchip and Meson I2C drivers > (the only platforms using the PMICs handled by this driver) both have > support for atomic transfers and thus may be called from the proper > poweroff context. > > Things are different on the SPI side. That is so far only used by rk806 > and that one is only used by Rockchip RK3588. Unfortunately the Rockchip > SPI driver does not support atomic transfers. That means using the > normal POWER_OFF handler would introduce the following error splash > during shutdown on all RK3588 boards currently supported upstream: > > [ 13.761353] ------------[ cut here ]------------ > [ 13.761764] Voluntary context switch within RCU read-side critical section! > [ 13.761776] WARNING: CPU: 0 PID: 1 at kernel/rcu/tree_plugin.h:330 rcu_note_context_switch+0x3ac/0x404 > [ 13.763219] Modules linked in: > [ 13.763498] CPU: 0 UID: 0 PID: 1 Comm: systemd-shutdow Not tainted 6.10.0-12284-g2818a9a19514 #1499 > [ 13.764297] Hardware name: Rockchip RK3588 EVB1 V10 Board (DT) > [ 13.764812] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 13.765427] pc : rcu_note_context_switch+0x3ac/0x404 > [ 13.765871] lr : rcu_note_context_switch+0x3ac/0x404 > [ 13.766314] sp : ffff800084f4b5b0 > [ 13.766609] x29: ffff800084f4b5b0 x28: ffff00040139b800 x27: 00007dfb4439ae80 > [ 13.767245] x26: ffff00040139bc80 x25: 0000000000000000 x24: ffff800082118470 > [ 13.767880] x23: 0000000000000000 x22: ffff000400300000 x21: ffff000400300000 > [ 13.768515] x20: ffff800083a9d600 x19: ffff0004fee48600 x18: fffffffffffed448 > [ 13.769151] x17: 000000040044ffff x16: 005000f2b5503510 x15: 0000000000000048 > [ 13.769787] x14: fffffffffffed490 x13: ffff80008473b3c0 x12: 0000000000000900 > [ 13.770421] x11: 0000000000000300 x10: ffff800084797bc0 x9 : ffff80008473b3c0 > [ 13.771057] x8 : 00000000ffffefff x7 : ffff8000847933c0 x6 : 0000000000000300 > [ 13.771692] x5 : 0000000000000301 x4 : 40000000fffff300 x3 : 0000000000000000 > [ 13.772328] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000400300000 > [ 13.772964] Call trace: > [ 13.773184] rcu_note_context_switch+0x3ac/0x404 > [ 13.773598] __schedule+0x94/0xb0c > [ 13.773907] schedule+0x34/0x104 > [ 13.774198] schedule_timeout+0x84/0xfc > [ 13.774544] wait_for_completion_timeout+0x78/0x14c > [ 13.774980] spi_transfer_one_message+0x588/0x690 > [ 13.775403] __spi_pump_transfer_message+0x19c/0x4ec > [ 13.775846] __spi_sync+0x2a8/0x3c4 > [ 13.776161] spi_write_then_read+0x120/0x208 > [ 13.776543] rk806_spi_bus_read+0x54/0x88 > [ 13.776905] _regmap_raw_read+0xec/0x16c > [ 13.777257] _regmap_bus_read+0x44/0x7c > [ 13.777601] _regmap_read+0x60/0xd8 > [ 13.777915] _regmap_update_bits+0xf4/0x13c > [ 13.778289] regmap_update_bits_base+0x64/0x98 > [ 13.778686] rk808_power_off+0x70/0xfc > [ 13.779024] sys_off_notify+0x40/0x6c > [ 13.779356] atomic_notifier_call_chain+0x60/0x90 > [ 13.779776] do_kernel_power_off+0x54/0x6c > [ 13.780146] machine_power_off+0x18/0x24 > [ 13.780499] kernel_power_off+0x70/0x7c > [ 13.780845] __do_sys_reboot+0x210/0x270 > [ 13.781198] __arm64_sys_reboot+0x24/0x30 > [ 13.781558] invoke_syscall+0x48/0x10c > [ 13.781897] el0_svc_common+0x3c/0xe8 > [ 13.782228] do_el0_svc+0x20/0x2c > [ 13.782528] el0_svc+0x34/0xd8 > [ 13.782806] el0t_64_sync_handler+0x120/0x12c > [ 13.783197] el0t_64_sync+0x190/0x194 > [ 13.783527] ---[ end trace 0000000000000000 ]--- > > To avoid this we keep the SYS_OFF_MODE_POWER_OFF_PREPARE handler for the > SPI backend. This is not great, but at least avoids regressions and the > fix should be small enough to allow backporting. > > As a side-effect this also works around a shutdown problem on the Asus > C201. For reasons unknown that skips calling the prepare handler and > directly calls the final shutdown handler. > > Fixes: 4fec8a5a85c49 ("mfd: rk808: Convert to device managed resources") > Cc: stable@vger.kernel.org > Reported-by: Urja <urja@urja.dev> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> On a QNAP-TS433: Tested-by: Heiko Stuebner <heiko@sntech.de> Change itself also looks nice and it definitly helps my qnap-mcu patches. Because the mcu needs to turn off its parts before and till now I worked around the issue by occupying another priority, while it should simply be part of the prepare + default-prio stage, so Reviewed-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/mfd/rk8xx-core.c | 15 +++++++++++++-- > drivers/mfd/rk8xx-i2c.c | 2 +- > drivers/mfd/rk8xx-spi.c | 2 +- > include/linux/mfd/rk808.h | 2 +- > 4 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c > index 5eda3c0dbbdf..757ef8181328 100644 > --- a/drivers/mfd/rk8xx-core.c > +++ b/drivers/mfd/rk8xx-core.c > @@ -692,10 +692,11 @@ void rk8xx_shutdown(struct device *dev) > } > EXPORT_SYMBOL_GPL(rk8xx_shutdown); > > -int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap) > +int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap, bool is_spi) > { > struct rk808 *rk808; > const struct rk808_reg_data *pre_init_reg; > + enum sys_off_mode pwr_off_mode = SYS_OFF_MODE_POWER_OFF; > const struct mfd_cell *cells; > int dual_support = 0; > int nr_pre_init_regs; > @@ -785,10 +786,20 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap > if (ret) > return dev_err_probe(dev, ret, "failed to add MFD devices\n"); > > + /* > + * Currently the Rockchip SPI driver always sleeps when doing SPI > + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF > + * handler, so we are using the prepare handler as a workaround. > + * This should be removed once the Rockchip SPI driver has been > + * adapted. > + */ > + if (is_spi) > + pwr_off_mode = SYS_OFF_MODE_POWER_OFF_PREPARE; > + > if (device_property_read_bool(dev, "rockchip,system-power-controller") || > device_property_read_bool(dev, "system-power-controller")) { > ret = devm_register_sys_off_handler(dev, > - SYS_OFF_MODE_POWER_OFF_PREPARE, SYS_OFF_PRIO_HIGH, > + pwr_off_mode, SYS_OFF_PRIO_HIGH, > &rk808_power_off, rk808); > if (ret) > return dev_err_probe(dev, ret, > diff --git a/drivers/mfd/rk8xx-i2c.c b/drivers/mfd/rk8xx-i2c.c > index 69a6b297d723..a2029decd654 100644 > --- a/drivers/mfd/rk8xx-i2c.c > +++ b/drivers/mfd/rk8xx-i2c.c > @@ -189,7 +189,7 @@ static int rk8xx_i2c_probe(struct i2c_client *client) > return dev_err_probe(&client->dev, PTR_ERR(regmap), > "regmap initialization failed\n"); > > - return rk8xx_probe(&client->dev, data->variant, client->irq, regmap); > + return rk8xx_probe(&client->dev, data->variant, client->irq, regmap, false); > } > > static void rk8xx_i2c_shutdown(struct i2c_client *client) > diff --git a/drivers/mfd/rk8xx-spi.c b/drivers/mfd/rk8xx-spi.c > index 3405fb82ff9f..20f9428f94bb 100644 > --- a/drivers/mfd/rk8xx-spi.c > +++ b/drivers/mfd/rk8xx-spi.c > @@ -94,7 +94,7 @@ static int rk8xx_spi_probe(struct spi_device *spi) > return dev_err_probe(&spi->dev, PTR_ERR(regmap), > "Failed to init regmap\n"); > > - return rk8xx_probe(&spi->dev, RK806_ID, spi->irq, regmap); > + return rk8xx_probe(&spi->dev, RK806_ID, spi->irq, regmap, true); > } > > static const struct of_device_id rk8xx_spi_of_match[] = { > diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h > index 69cbea78b430..be15b84cff9e 100644 > --- a/include/linux/mfd/rk808.h > +++ b/include/linux/mfd/rk808.h > @@ -1349,7 +1349,7 @@ struct rk808 { > }; > > void rk8xx_shutdown(struct device *dev); > -int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap); > +int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap, bool is_spi); > int rk8xx_suspend(struct device *dev); > int rk8xx_resume(struct device *dev); > >
On Tue, 30 Jul 2024, Sebastian Reichel wrote: > When I converted rk808 to device managed resources I converted the rk808 > specific pm_power_off handler to devm_register_sys_off_handler() using > SYS_OFF_MODE_POWER_OFF_PREPARE, which is allowed to sleep. I did this > because the driver's poweroff function makes use of regmap and the backend > of that might sleep. > > But the PMIC poweroff function will kill off the board power and the > kernel does some extra steps after the prepare handler. Thus the prepare > handler should not be used for the PMIC's poweroff routine. Instead the > normal SYS_OFF_MODE_POWER_OFF phase should be used. The old pm_power_off > method is also being called from there, so this would have been a > cleaner conversion anyways. > > But it still makes sense to investigate the sleep handling and check > if there are any issues. Apparently the Rockchip and Meson I2C drivers > (the only platforms using the PMICs handled by this driver) both have > support for atomic transfers and thus may be called from the proper > poweroff context. > > Things are different on the SPI side. That is so far only used by rk806 > and that one is only used by Rockchip RK3588. Unfortunately the Rockchip > SPI driver does not support atomic transfers. That means using the > normal POWER_OFF handler would introduce the following error splash > during shutdown on all RK3588 boards currently supported upstream: > > [ 13.761353] ------------[ cut here ]------------ > [ 13.761764] Voluntary context switch within RCU read-side critical section! > [ 13.761776] WARNING: CPU: 0 PID: 1 at kernel/rcu/tree_plugin.h:330 rcu_note_context_switch+0x3ac/0x404 > [ 13.763219] Modules linked in: > [ 13.763498] CPU: 0 UID: 0 PID: 1 Comm: systemd-shutdow Not tainted 6.10.0-12284-g2818a9a19514 #1499 > [ 13.764297] Hardware name: Rockchip RK3588 EVB1 V10 Board (DT) > [ 13.764812] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 13.765427] pc : rcu_note_context_switch+0x3ac/0x404 > [ 13.765871] lr : rcu_note_context_switch+0x3ac/0x404 > [ 13.766314] sp : ffff800084f4b5b0 > [ 13.766609] x29: ffff800084f4b5b0 x28: ffff00040139b800 x27: 00007dfb4439ae80 > [ 13.767245] x26: ffff00040139bc80 x25: 0000000000000000 x24: ffff800082118470 > [ 13.767880] x23: 0000000000000000 x22: ffff000400300000 x21: ffff000400300000 > [ 13.768515] x20: ffff800083a9d600 x19: ffff0004fee48600 x18: fffffffffffed448 > [ 13.769151] x17: 000000040044ffff x16: 005000f2b5503510 x15: 0000000000000048 > [ 13.769787] x14: fffffffffffed490 x13: ffff80008473b3c0 x12: 0000000000000900 > [ 13.770421] x11: 0000000000000300 x10: ffff800084797bc0 x9 : ffff80008473b3c0 > [ 13.771057] x8 : 00000000ffffefff x7 : ffff8000847933c0 x6 : 0000000000000300 > [ 13.771692] x5 : 0000000000000301 x4 : 40000000fffff300 x3 : 0000000000000000 > [ 13.772328] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000400300000 > [ 13.772964] Call trace: > [ 13.773184] rcu_note_context_switch+0x3ac/0x404 > [ 13.773598] __schedule+0x94/0xb0c > [ 13.773907] schedule+0x34/0x104 > [ 13.774198] schedule_timeout+0x84/0xfc > [ 13.774544] wait_for_completion_timeout+0x78/0x14c > [ 13.774980] spi_transfer_one_message+0x588/0x690 > [ 13.775403] __spi_pump_transfer_message+0x19c/0x4ec > [ 13.775846] __spi_sync+0x2a8/0x3c4 > [ 13.776161] spi_write_then_read+0x120/0x208 > [ 13.776543] rk806_spi_bus_read+0x54/0x88 > [ 13.776905] _regmap_raw_read+0xec/0x16c > [ 13.777257] _regmap_bus_read+0x44/0x7c > [ 13.777601] _regmap_read+0x60/0xd8 > [ 13.777915] _regmap_update_bits+0xf4/0x13c > [ 13.778289] regmap_update_bits_base+0x64/0x98 > [ 13.778686] rk808_power_off+0x70/0xfc > [ 13.779024] sys_off_notify+0x40/0x6c > [ 13.779356] atomic_notifier_call_chain+0x60/0x90 > [ 13.779776] do_kernel_power_off+0x54/0x6c > [ 13.780146] machine_power_off+0x18/0x24 > [ 13.780499] kernel_power_off+0x70/0x7c > [ 13.780845] __do_sys_reboot+0x210/0x270 > [ 13.781198] __arm64_sys_reboot+0x24/0x30 > [ 13.781558] invoke_syscall+0x48/0x10c > [ 13.781897] el0_svc_common+0x3c/0xe8 > [ 13.782228] do_el0_svc+0x20/0x2c > [ 13.782528] el0_svc+0x34/0xd8 > [ 13.782806] el0t_64_sync_handler+0x120/0x12c > [ 13.783197] el0t_64_sync+0x190/0x194 > [ 13.783527] ---[ end trace 0000000000000000 ]--- > > To avoid this we keep the SYS_OFF_MODE_POWER_OFF_PREPARE handler for the > SPI backend. This is not great, but at least avoids regressions and the > fix should be small enough to allow backporting. > > As a side-effect this also works around a shutdown problem on the Asus > C201. For reasons unknown that skips calling the prepare handler and > directly calls the final shutdown handler. > > Fixes: 4fec8a5a85c49 ("mfd: rk808: Convert to device managed resources") > Cc: stable@vger.kernel.org > Reported-by: Urja <urja@urja.dev> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/mfd/rk8xx-core.c | 15 +++++++++++++-- > drivers/mfd/rk8xx-i2c.c | 2 +- > drivers/mfd/rk8xx-spi.c | 2 +- > include/linux/mfd/rk808.h | 2 +- > 4 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c > index 5eda3c0dbbdf..757ef8181328 100644 > --- a/drivers/mfd/rk8xx-core.c > +++ b/drivers/mfd/rk8xx-core.c > @@ -692,10 +692,11 @@ void rk8xx_shutdown(struct device *dev) > } > EXPORT_SYMBOL_GPL(rk8xx_shutdown); > > -int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap) > +int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap, bool is_spi) > { > struct rk808 *rk808; > const struct rk808_reg_data *pre_init_reg; > + enum sys_off_mode pwr_off_mode = SYS_OFF_MODE_POWER_OFF; > const struct mfd_cell *cells; > int dual_support = 0; > int nr_pre_init_regs; > @@ -785,10 +786,20 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap > if (ret) > return dev_err_probe(dev, ret, "failed to add MFD devices\n"); > > + /* > + * Currently the Rockchip SPI driver always sleeps when doing SPI > + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF > + * handler, so we are using the prepare handler as a workaround. > + * This should be removed once the Rockchip SPI driver has been > + * adapted. > + */ So why not just adapt the SPI driver now? What's the bet that if accepted, this hack is still here in 5 years time? > + if (is_spi) > + pwr_off_mode = SYS_OFF_MODE_POWER_OFF_PREPARE; > + > if (device_property_read_bool(dev, "rockchip,system-power-controller") || > device_property_read_bool(dev, "system-power-controller")) { > ret = devm_register_sys_off_handler(dev, > - SYS_OFF_MODE_POWER_OFF_PREPARE, SYS_OFF_PRIO_HIGH, > + pwr_off_mode, SYS_OFF_PRIO_HIGH, > &rk808_power_off, rk808); > if (ret) > return dev_err_probe(dev, ret, > diff --git a/drivers/mfd/rk8xx-i2c.c b/drivers/mfd/rk8xx-i2c.c > index 69a6b297d723..a2029decd654 100644 > --- a/drivers/mfd/rk8xx-i2c.c > +++ b/drivers/mfd/rk8xx-i2c.c > @@ -189,7 +189,7 @@ static int rk8xx_i2c_probe(struct i2c_client *client) > return dev_err_probe(&client->dev, PTR_ERR(regmap), > "regmap initialization failed\n"); > > - return rk8xx_probe(&client->dev, data->variant, client->irq, regmap); > + return rk8xx_probe(&client->dev, data->variant, client->irq, regmap, false); > } > > static void rk8xx_i2c_shutdown(struct i2c_client *client) > diff --git a/drivers/mfd/rk8xx-spi.c b/drivers/mfd/rk8xx-spi.c > index 3405fb82ff9f..20f9428f94bb 100644 > --- a/drivers/mfd/rk8xx-spi.c > +++ b/drivers/mfd/rk8xx-spi.c > @@ -94,7 +94,7 @@ static int rk8xx_spi_probe(struct spi_device *spi) > return dev_err_probe(&spi->dev, PTR_ERR(regmap), > "Failed to init regmap\n"); > > - return rk8xx_probe(&spi->dev, RK806_ID, spi->irq, regmap); > + return rk8xx_probe(&spi->dev, RK806_ID, spi->irq, regmap, true); > } > > static const struct of_device_id rk8xx_spi_of_match[] = { > diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h > index 69cbea78b430..be15b84cff9e 100644 > --- a/include/linux/mfd/rk808.h > +++ b/include/linux/mfd/rk808.h > @@ -1349,7 +1349,7 @@ struct rk808 { > }; > > void rk8xx_shutdown(struct device *dev); > -int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap); > +int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap, bool is_spi); > int rk8xx_suspend(struct device *dev); > int rk8xx_resume(struct device *dev); > > -- > 2.43.0 >
Hello Lee, On Thu, Aug 01, 2024 at 02:18:23PM GMT, Lee Jones wrote: > > + /* > > + * Currently the Rockchip SPI driver always sleeps when doing SPI > > + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF > > + * handler, so we are using the prepare handler as a workaround. > > + * This should be removed once the Rockchip SPI driver has been > > + * adapted. > > + */ > > So why not just adapt the SPI driver now? This patch is simple and thus can easily be backported, so that the Acer Chromebook shutdown is fixed in the stable kernels. SPI based rkxx has been using SYS_OFF_MODE_POWER_OFF_PREPARE from the start, so it's not a regression. As far as I could see the SPI framework does not have something comparable to the I2C .xfer_atomic handler. So fixing up the Rockchip SPI driver probably involves creating some SPI core helpers. I'm not yet sure about the best way to deal with this. But I guess it will be better not having to backport all of the requires changes to stable. In any case I think the next step in this direction is discussing how to handle this in general for SPI. > What's the bet that if accepted, this hack is still here in 5 years time? Even if I don't work on this now, I would expect somebody to have issues with broken shutdown on RK3588 boards before 5 years are over :) Greetings, -- Sebastian
On 7/30/24 21:05, Sebastian Reichel wrote: > + /* > + * Currently the Rockchip SPI driver always sleeps when doing SPI > + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF > + * handler, so we are using the prepare handler as a workaround. > + * This should be removed once the Rockchip SPI driver has been > + * adapted. > + */ > + if (is_spi) > + pwr_off_mode = SYS_OFF_MODE_POWER_OFF_PREPARE; This prevents the syscore_shutdown() step from execution. Is it better than not powering off? I'd rather skip registration of the power-off handlers in a case of SPI :)
Am Donnerstag, 1. August 2024, 17:31:33 CEST schrieb Dmitry Osipenko: > On 7/30/24 21:05, Sebastian Reichel wrote: > > + /* > > + * Currently the Rockchip SPI driver always sleeps when doing SPI > > + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF > > + * handler, so we are using the prepare handler as a workaround. > > + * This should be removed once the Rockchip SPI driver has been > > + * adapted. > > + */ > > + if (is_spi) > > + pwr_off_mode = SYS_OFF_MODE_POWER_OFF_PREPARE; > > This prevents the syscore_shutdown() step from execution. Is it better > than not powering off? > > I'd rather skip registration of the power-off handlers in a case of SPI :) Or blasphemous thought, we could live with the warning-splash for a bit. From Sebastian's log I assume the WARNING comes from the wait_for_completion() in spi_transfer_wait(), and I guess the transfer with the poweroff command itself will already have happened then? So the device is most likely still powered off in that case? Not sure how much of "bad taste" that thought is though ;-) Heiko [0] https://elixir.bootlin.com/linux/v6.10.2/source/drivers/spi/spi.c#L1671
Hi, On Thu, Aug 01, 2024 at 07:41:44PM GMT, Heiko Stübner wrote: > Am Donnerstag, 1. August 2024, 17:31:33 CEST schrieb Dmitry Osipenko: > > On 7/30/24 21:05, Sebastian Reichel wrote: > > > + /* > > > + * Currently the Rockchip SPI driver always sleeps when doing SPI > > > + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF > > > + * handler, so we are using the prepare handler as a workaround. > > > + * This should be removed once the Rockchip SPI driver has been > > > + * adapted. > > > + */ > > > + if (is_spi) > > > + pwr_off_mode = SYS_OFF_MODE_POWER_OFF_PREPARE; > > > > This prevents the syscore_shutdown() step from execution. Is it better > > than not powering off? > > > > I'd rather skip registration of the power-off handlers in a case of SPI :) > > Or blasphemous thought, we could live with the warning-splash for a bit. > > From Sebastian's log I assume the WARNING comes from the > wait_for_completion() in spi_transfer_wait(), and I guess the transfer > with the poweroff command itself will already have happened then? > > So the device is most likely still powered off in that case? > Not sure how much of "bad taste" that thought is though ;-) Yes, as far as I could see it works fine (the splash from the commit message is from exactly this solution running on RK3588 EVB1 and the board was powered off properly as far as I can tell). But it felt a bit strange to knowingly introduce an error splash in a fix intended for being backported to the stable trees, so I switched to the current version before sending. -- Sebastian
On 8/1/24 20:52, Sebastian Reichel wrote: > Hi, > > On Thu, Aug 01, 2024 at 07:41:44PM GMT, Heiko Stübner wrote: >> Am Donnerstag, 1. August 2024, 17:31:33 CEST schrieb Dmitry Osipenko: >>> On 7/30/24 21:05, Sebastian Reichel wrote: >>>> + /* >>>> + * Currently the Rockchip SPI driver always sleeps when doing SPI >>>> + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF >>>> + * handler, so we are using the prepare handler as a workaround. >>>> + * This should be removed once the Rockchip SPI driver has been >>>> + * adapted. >>>> + */ >>>> + if (is_spi) >>>> + pwr_off_mode = SYS_OFF_MODE_POWER_OFF_PREPARE; >>> >>> This prevents the syscore_shutdown() step from execution. Is it better >>> than not powering off? >>> >>> I'd rather skip registration of the power-off handlers in a case of SPI :) >> >> Or blasphemous thought, we could live with the warning-splash for a bit. >> >> From Sebastian's log I assume the WARNING comes from the >> wait_for_completion() in spi_transfer_wait(), and I guess the transfer >> with the poweroff command itself will already have happened then? >> >> So the device is most likely still powered off in that case? >> Not sure how much of "bad taste" that thought is though ;-) > > Yes, as far as I could see it works fine (the splash from the commit > message is from exactly this solution running on RK3588 EVB1 and the > board was powered off properly as far as I can tell). But it felt a > bit strange to knowingly introduce an error splash in a fix intended > for being backported to the stable trees, so I switched to the current > version before sending. Can you add a busy-wait to the SPI driver TX func for the case where it's invoked with a disabled interrupts? That could be a better workaround, silencing the warning and keeping power-off working properly.
Hello all, On 2024-08-01 17:22, Sebastian Reichel wrote: > On Thu, Aug 01, 2024 at 02:18:23PM GMT, Lee Jones wrote: >> > + /* >> > + * Currently the Rockchip SPI driver always sleeps when doing SPI >> > + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF >> > + * handler, so we are using the prepare handler as a workaround. >> > + * This should be removed once the Rockchip SPI driver has been >> > + * adapted. >> > + */ >> >> So why not just adapt the SPI driver now? > > This patch is simple and thus can easily be backported, so that the > Acer Chromebook shutdown is fixed in the stable kernels. SPI based > rkxx has been using SYS_OFF_MODE_POWER_OFF_PREPARE from the start, > so it's not a regression. > > As far as I could see the SPI framework does not have something > comparable to the I2C .xfer_atomic handler. So fixing up the > Rockchip SPI driver probably involves creating some SPI core > helpers. I'm not yet sure about the best way to deal with this. > But I guess it will be better not having to backport all of the > requires changes to stable. > > In any case I think the next step in this direction is discussing > how to handle this in general for SPI. I agree about accepting this (hot)fix first, and improving the SPI drivers later. Having this fixed now provides clear benefits. Also, please feel free to include: Reviewed-by: Dragan Simic <dsimic@manjaro.org> >> What's the bet that if accepted, this hack is still here in 5 years >> time? > > Even if I don't work on this now, I would expect somebody to have > issues with broken shutdown on RK3588 boards before 5 years are > over :)
diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c index 5eda3c0dbbdf..757ef8181328 100644 --- a/drivers/mfd/rk8xx-core.c +++ b/drivers/mfd/rk8xx-core.c @@ -692,10 +692,11 @@ void rk8xx_shutdown(struct device *dev) } EXPORT_SYMBOL_GPL(rk8xx_shutdown); -int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap) +int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap, bool is_spi) { struct rk808 *rk808; const struct rk808_reg_data *pre_init_reg; + enum sys_off_mode pwr_off_mode = SYS_OFF_MODE_POWER_OFF; const struct mfd_cell *cells; int dual_support = 0; int nr_pre_init_regs; @@ -785,10 +786,20 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap if (ret) return dev_err_probe(dev, ret, "failed to add MFD devices\n"); + /* + * Currently the Rockchip SPI driver always sleeps when doing SPI + * transfers. This is not allowed in the SYS_OFF_MODE_POWER_OFF + * handler, so we are using the prepare handler as a workaround. + * This should be removed once the Rockchip SPI driver has been + * adapted. + */ + if (is_spi) + pwr_off_mode = SYS_OFF_MODE_POWER_OFF_PREPARE; + if (device_property_read_bool(dev, "rockchip,system-power-controller") || device_property_read_bool(dev, "system-power-controller")) { ret = devm_register_sys_off_handler(dev, - SYS_OFF_MODE_POWER_OFF_PREPARE, SYS_OFF_PRIO_HIGH, + pwr_off_mode, SYS_OFF_PRIO_HIGH, &rk808_power_off, rk808); if (ret) return dev_err_probe(dev, ret, diff --git a/drivers/mfd/rk8xx-i2c.c b/drivers/mfd/rk8xx-i2c.c index 69a6b297d723..a2029decd654 100644 --- a/drivers/mfd/rk8xx-i2c.c +++ b/drivers/mfd/rk8xx-i2c.c @@ -189,7 +189,7 @@ static int rk8xx_i2c_probe(struct i2c_client *client) return dev_err_probe(&client->dev, PTR_ERR(regmap), "regmap initialization failed\n"); - return rk8xx_probe(&client->dev, data->variant, client->irq, regmap); + return rk8xx_probe(&client->dev, data->variant, client->irq, regmap, false); } static void rk8xx_i2c_shutdown(struct i2c_client *client) diff --git a/drivers/mfd/rk8xx-spi.c b/drivers/mfd/rk8xx-spi.c index 3405fb82ff9f..20f9428f94bb 100644 --- a/drivers/mfd/rk8xx-spi.c +++ b/drivers/mfd/rk8xx-spi.c @@ -94,7 +94,7 @@ static int rk8xx_spi_probe(struct spi_device *spi) return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Failed to init regmap\n"); - return rk8xx_probe(&spi->dev, RK806_ID, spi->irq, regmap); + return rk8xx_probe(&spi->dev, RK806_ID, spi->irq, regmap, true); } static const struct of_device_id rk8xx_spi_of_match[] = { diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h index 69cbea78b430..be15b84cff9e 100644 --- a/include/linux/mfd/rk808.h +++ b/include/linux/mfd/rk808.h @@ -1349,7 +1349,7 @@ struct rk808 { }; void rk8xx_shutdown(struct device *dev); -int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap); +int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap *regmap, bool is_spi); int rk8xx_suspend(struct device *dev); int rk8xx_resume(struct device *dev);
When I converted rk808 to device managed resources I converted the rk808 specific pm_power_off handler to devm_register_sys_off_handler() using SYS_OFF_MODE_POWER_OFF_PREPARE, which is allowed to sleep. I did this because the driver's poweroff function makes use of regmap and the backend of that might sleep. But the PMIC poweroff function will kill off the board power and the kernel does some extra steps after the prepare handler. Thus the prepare handler should not be used for the PMIC's poweroff routine. Instead the normal SYS_OFF_MODE_POWER_OFF phase should be used. The old pm_power_off method is also being called from there, so this would have been a cleaner conversion anyways. But it still makes sense to investigate the sleep handling and check if there are any issues. Apparently the Rockchip and Meson I2C drivers (the only platforms using the PMICs handled by this driver) both have support for atomic transfers and thus may be called from the proper poweroff context. Things are different on the SPI side. That is so far only used by rk806 and that one is only used by Rockchip RK3588. Unfortunately the Rockchip SPI driver does not support atomic transfers. That means using the normal POWER_OFF handler would introduce the following error splash during shutdown on all RK3588 boards currently supported upstream: [ 13.761353] ------------[ cut here ]------------ [ 13.761764] Voluntary context switch within RCU read-side critical section! [ 13.761776] WARNING: CPU: 0 PID: 1 at kernel/rcu/tree_plugin.h:330 rcu_note_context_switch+0x3ac/0x404 [ 13.763219] Modules linked in: [ 13.763498] CPU: 0 UID: 0 PID: 1 Comm: systemd-shutdow Not tainted 6.10.0-12284-g2818a9a19514 #1499 [ 13.764297] Hardware name: Rockchip RK3588 EVB1 V10 Board (DT) [ 13.764812] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 13.765427] pc : rcu_note_context_switch+0x3ac/0x404 [ 13.765871] lr : rcu_note_context_switch+0x3ac/0x404 [ 13.766314] sp : ffff800084f4b5b0 [ 13.766609] x29: ffff800084f4b5b0 x28: ffff00040139b800 x27: 00007dfb4439ae80 [ 13.767245] x26: ffff00040139bc80 x25: 0000000000000000 x24: ffff800082118470 [ 13.767880] x23: 0000000000000000 x22: ffff000400300000 x21: ffff000400300000 [ 13.768515] x20: ffff800083a9d600 x19: ffff0004fee48600 x18: fffffffffffed448 [ 13.769151] x17: 000000040044ffff x16: 005000f2b5503510 x15: 0000000000000048 [ 13.769787] x14: fffffffffffed490 x13: ffff80008473b3c0 x12: 0000000000000900 [ 13.770421] x11: 0000000000000300 x10: ffff800084797bc0 x9 : ffff80008473b3c0 [ 13.771057] x8 : 00000000ffffefff x7 : ffff8000847933c0 x6 : 0000000000000300 [ 13.771692] x5 : 0000000000000301 x4 : 40000000fffff300 x3 : 0000000000000000 [ 13.772328] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000400300000 [ 13.772964] Call trace: [ 13.773184] rcu_note_context_switch+0x3ac/0x404 [ 13.773598] __schedule+0x94/0xb0c [ 13.773907] schedule+0x34/0x104 [ 13.774198] schedule_timeout+0x84/0xfc [ 13.774544] wait_for_completion_timeout+0x78/0x14c [ 13.774980] spi_transfer_one_message+0x588/0x690 [ 13.775403] __spi_pump_transfer_message+0x19c/0x4ec [ 13.775846] __spi_sync+0x2a8/0x3c4 [ 13.776161] spi_write_then_read+0x120/0x208 [ 13.776543] rk806_spi_bus_read+0x54/0x88 [ 13.776905] _regmap_raw_read+0xec/0x16c [ 13.777257] _regmap_bus_read+0x44/0x7c [ 13.777601] _regmap_read+0x60/0xd8 [ 13.777915] _regmap_update_bits+0xf4/0x13c [ 13.778289] regmap_update_bits_base+0x64/0x98 [ 13.778686] rk808_power_off+0x70/0xfc [ 13.779024] sys_off_notify+0x40/0x6c [ 13.779356] atomic_notifier_call_chain+0x60/0x90 [ 13.779776] do_kernel_power_off+0x54/0x6c [ 13.780146] machine_power_off+0x18/0x24 [ 13.780499] kernel_power_off+0x70/0x7c [ 13.780845] __do_sys_reboot+0x210/0x270 [ 13.781198] __arm64_sys_reboot+0x24/0x30 [ 13.781558] invoke_syscall+0x48/0x10c [ 13.781897] el0_svc_common+0x3c/0xe8 [ 13.782228] do_el0_svc+0x20/0x2c [ 13.782528] el0_svc+0x34/0xd8 [ 13.782806] el0t_64_sync_handler+0x120/0x12c [ 13.783197] el0t_64_sync+0x190/0x194 [ 13.783527] ---[ end trace 0000000000000000 ]--- To avoid this we keep the SYS_OFF_MODE_POWER_OFF_PREPARE handler for the SPI backend. This is not great, but at least avoids regressions and the fix should be small enough to allow backporting. As a side-effect this also works around a shutdown problem on the Asus C201. For reasons unknown that skips calling the prepare handler and directly calls the final shutdown handler. Fixes: 4fec8a5a85c49 ("mfd: rk808: Convert to device managed resources") Cc: stable@vger.kernel.org Reported-by: Urja <urja@urja.dev> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- drivers/mfd/rk8xx-core.c | 15 +++++++++++++-- drivers/mfd/rk8xx-i2c.c | 2 +- drivers/mfd/rk8xx-spi.c | 2 +- include/linux/mfd/rk808.h | 2 +- 4 files changed, 16 insertions(+), 5 deletions(-)