Re: [PATCH 7/8] pwm: Add more locking

[Uwe Kleine-König]

Hello Marek,

[Cc += linux-clk@xxxxxxxxxxxxxxx, Alexander Stein, linux-arm-kernel@xxxxxxxxxxxxxxxxxxx]

On Thu, Apr 04, 2024 at 02:09:32PM +0200, Marek Szyprowski wrote:
> On 17.03.2024 11:40, Uwe Kleine-König wrote:
> > This ensures that a pwm_chip that has no corresponding driver isn't used
> > and that a driver doesn't go away while a callback is still running.
> >
> > In the presence of device links this isn't necessary yet (so this is no
> > fix) but for pwm character device support this is needed.
> >
> > To not serialize all pwm_apply_state() calls, this introduces a per chip
> > lock. An additional complication is that for atomic chips a mutex cannot
> > be used (as pwm_apply_atomic() must not sleem) and a spinlock cannot be
> > held while calling an operation for a sleeping chip. So depending on the
> > chip being atomic or not a spinlock or a mutex is used.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> 
> 
> This patch landed some time ago in linux-next as commit a740f7879609 
> ("pwm: Add more locking"). Recently I've finally found that $subject 
> patch is responsible for the following lock dep splat observed for some 
> time during day-to-day linux-next testing:
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.9.0-rc1+ #14790 Tainted: G         C
> ------------------------------------------------------
> kworker/u24:4/59 is trying to acquire lock:
> ffff00003c65b510 (&chip->nonatomic_lock){+.+.}-{3:3}, at: 
> pwm_apply_might_sleep+0x90/0xd8
> 
> but task is already holding lock:
> ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x4c/0xa8
> 
> which lock already depends on the new lock.
> 
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (prepare_lock){+.+.}-{3:3}:
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         clk_prepare_lock+0x4c/0xa8
>         clk_round_rate+0x38/0xd0
>         meson_pwm_apply+0x128/0x220 [pwm_meson]
>         __pwm_apply+0x64/0x1f8
>         pwm_apply_might_sleep+0x58/0xd8
>         pwm_regulator_set_voltage+0xbc/0x12c
>         _regulator_do_set_voltage+0x110/0x688
>         regulator_set_voltage_rdev+0x64/0x25c
>         regulator_do_balance_voltage+0x20c/0x47c
>         regulator_balance_voltage+0x50/0x9c
>         regulator_set_voltage_unlocked+0xa4/0x128
>         regulator_set_voltage+0x50/0xec
>         _opp_config_regulator_single+0x4c/0x130
>         _set_opp+0xfc/0x544
>         dev_pm_opp_set_rate+0x190/0x284
>         set_target+0x34/0x40
>         __cpufreq_driver_target+0x170/0x290
>         cpufreq_online+0x814/0xa3c
>         cpufreq_add_dev+0x80/0x98
>         subsys_interface_register+0xfc/0x118
>         cpufreq_register_driver+0x150/0x234
>         dt_cpufreq_probe+0x150/0x480
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach+0xa8/0x1b0
>         device_initial_probe+0x14/0x20
>         bus_probe_device+0xb0/0xb4
>         deferred_probe_work_func+0x8c/0xc8
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the meson pwm driver calling clk_set_rate() in the .apply()
callback.

> -> #0 (&chip->nonatomic_lock){+.+.}-{3:3}:
>         __lock_acquire+0x1318/0x20c4
>         lock_acquire.part.0+0xc8/0x20c
>         lock_acquire+0x68/0x84
>         __mutex_lock+0xa0/0x840
>         mutex_lock_nested+0x24/0x30
>         pwm_apply_might_sleep+0x90/0xd8
>         clk_pwm_prepare+0x54/0x84
>         clk_core_prepare+0xc8/0x2f8
>         clk_prepare+0x28/0x44
>         mmc_pwrseq_simple_pre_power_on+0x4c/0x8c
>         mmc_pwrseq_pre_power_on+0x24/0x34
>         mmc_power_up.part.0+0x20/0x16c
>         mmc_start_host+0xa0/0xac
>         mmc_add_host+0x84/0xf0
>         meson_mmc_probe+0x318/0x3e8
>         platform_probe+0x68/0xd8
>         really_probe+0xbc/0x2a0
>         __driver_probe_device+0x78/0x12c
>         driver_probe_device+0xdc/0x164
>         __device_attach_driver+0xb8/0x138
>         bus_for_each_drv+0x84/0xe0
>         __device_attach_async_helper+0xb0/0xd4
>         async_run_entry_fn+0x34/0xe0
>         process_one_work+0x220/0x634
>         worker_thread+0x268/0x3a8
>         kthread+0x124/0x128
>         ret_from_fork+0x10/0x20

This is the clk_pwm driver that calls pwm_enable() in its .prepare()
callback. Looking at
arch/arm64/boot/dts/amlogic/meson-g12b-s922x-khadas-vim3.dts the
pwm-clock uses &pwm_ef which is a meson pwm.

This alone only works because clk_prepare_lock() is reentrant for a
single thread (see commit 533ddeb1e86f ("clk: allow reentrant calls into
the clk framework")). (Is this really safe? What does the prepare_lock
actually protect?)

And because of this the lockdep splat is a false positive (as is every
ABBA splat involving the clk prepare_lock).

I think to properly address this, the global prepare_lock should be
replaced by a per-clk lock. This would at least make

	https://lore.kernel.org/all/20231017135436.1241650-1-alexander.stein@xxxxxxxxxxxxxxx/

(which I think is racy and needs a call to clk_rate_exclusive_get())
unnecessary.

Having said that, I don't know how to prevent that lockdep splat with
neither dropping the blamed commit (which is needed for race free
operation in the pwm framework) nor spending hours to rework the locking
of the clk framework.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature