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@xxxxxxxxxxxxxxx
> Reported-by: Urja <urja@xxxxxxxx>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
> 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
>
--
Lee Jones [李琼斯]
