Received-SPF: pass (google.com: domain of linux-kernel+bounces-131370-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <5a49cadd-21b7-4384-9e7d-9105ccc288b3@samsung.com>
Date: Thu, 4 Apr 2024 14:09:32 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 7/8] pwm: Add more locking
To: =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>,
	linux-pwm@vger.kernel.org
Cc: kernel@pengutronix.de, linux-amlogic@lists.infradead.org, LKML
	<linux-kernel@vger.kernel.org>
Content-Language: en-US
From: Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <e36c1a44096cbd7e9cb693f2300ac12ed1b2f79d.1710670958.git.u.kleine-koenig@pengutronix.de>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <cover.1710670958.git.u.kleine-koenig@pengutronix.de>
	<e36c1a44096cbd7e9cb693f2300ac12ed1b2f79d.1710670958.git.u.kleine-koenig@pengutronix.de>
	<CGME20240404120932eucas1p1b3c1e07bf6f41f6330725148b0268b13@eucas1p1.samsung.com>

Hi Uwe,

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@pengutronix.de>


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

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

other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(prepare_lock);
                                lock(&chip->nonatomic_lock);
                                lock(prepare_lock);
   lock(&chip->nonatomic_lock);

  *** DEADLOCK ***

4 locks held by kworker/u24:4/59:
  #0: ffff000000220d48 ((wq_completion)async){+.+.}-{0:0}, at: 
process_one_work+0x1a0/0x634
  #1: ffff80008461bde0 ((work_completion)(&entry->work)){+.+.}-{0:0}, 
at: process_one_work+0x1c8/0x634
  #2: ffff0000015bf0f8 (&dev->mutex){....}-{3:3}, at: 
__device_attach_async_helper+0x3c/0xd4
  #3: ffff80008310fa40 (prepare_lock){+.+.}-{3:3}, at: 
clk_prepare_lock+0x4c/0xa8

stack backtrace:
CPU: 1 PID: 59 Comm: kworker/u24:4 Tainted: G         C 6.9.0-rc1+ #14790
Hardware name: Khadas VIM3 (DT)
Workqueue: async async_run_entry_fn
Call trace:
  dump_backtrace+0x98/0xf0
  show_stack+0x18/0x24
  dump_stack_lvl+0x90/0xd0
  dump_stack+0x18/0x24
  print_circular_bug+0x290/0x370
  check_noncircular+0x15c/0x170
  __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


I'm not really sure if this warning shows a real problem or just some 
functions are missing lock dep annotations. Quite a lots of subsystems 
are involved in it (clocks, regulators and pwms) and this issue is fully 
reproducible here during system boot on Khadas VIM3 ARM64-based board.


> ---
>   drivers/pwm/core.c  | 98 +++++++++++++++++++++++++++++++++++++++++----
>   include/linux/pwm.h | 13 ++++++
>   2 files changed, 103 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 09ff6ef0b634..2e08fcfbe138 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -29,6 +29,22 @@ static DEFINE_MUTEX(pwm_lock);
>   
>   static DEFINE_IDR(pwm_chips);
>   
> +static void pwmchip_lock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_lock(&chip->atomic_lock);
> +	else
> +		mutex_lock(&chip->nonatomic_lock);
> +}
> +
> +static void pwmchip_unlock(struct pwm_chip *chip)
> +{
> +	if (chip->atomic)
> +		spin_unlock(&chip->atomic_lock);
> +	else
> +		mutex_unlock(&chip->nonatomic_lock);
> +}
> +
>   static void pwm_apply_debug(struct pwm_device *pwm,
>   			    const struct pwm_state *state)
>   {
> @@ -183,6 +199,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
>   int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   {
>   	int err;
> +	struct pwm_chip *chip = pwm->chip;
>   
>   	/*
>   	 * Some lowlevel driver's implementations of .apply() make use of
> @@ -193,7 +210,13 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   	 */
>   	might_sleep();
>   
> -	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
> +	pwmchip_lock(chip);
> +	if (!chip->operational) {
> +		pwmchip_unlock(chip);
> +		return -ENODEV;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
>   		/*
>   		 * Catch any drivers that have been marked as atomic but
>   		 * that will sleep anyway.
> @@ -205,6 +228,8 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>   		err = __pwm_apply(pwm, state);
>   	}
>   
> +	pwmchip_unlock(chip);
> +
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
> @@ -291,16 +316,26 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
>   int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>   		unsigned long timeout)
>   {
> +	struct pwm_chip *chip;
>   	int err;
>   
> -	if (!pwm || !pwm->chip->ops)
> +	if (!pwm || !(chip = pwm->chip)->ops)
>   		return -EINVAL;
>   
> -	if (!pwm->chip->ops->capture)
> +	if (!chip->ops->capture)
>   		return -ENOSYS;
>   
>   	mutex_lock(&pwm_lock);
> -	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
> +
> +	pwmchip_lock(chip);
> +
> +	if (chip->operational)
> +		err = chip->ops->capture(pwm->chip, pwm, result, timeout);
> +	else
> +		err = -ENODEV;
> +
> +	pwmchip_unlock(chip);
> +
>   	mutex_unlock(&pwm_lock);
>   
>   	return err;
> @@ -340,6 +375,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   	if (test_bit(PWMF_REQUESTED, &pwm->flags))
>   		return -EBUSY;
>   
> +	/*
> +	 * This function is called while holding pwm_lock. As .operational only
> +	 * changes while holding this lock, checking it here without holding the
> +	 * chip lock is fine.
> +	 */
> +	if (!chip->operational)
> +		return -ENODEV;
> +
>   	if (!try_module_get(chip->owner))
>   		return -ENODEV;
>   
> @@ -368,7 +411,12 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   		 */
>   		struct pwm_state state = { 0, };
>   
> +		pwmchip_lock(chip);
> +
>   		err = ops->get_state(chip, pwm, &state);
> +
> +		pwmchip_unlock(chip);
> +
>   		trace_pwm_get(pwm, &state, err);
>   
>   		if (!err)
> @@ -997,6 +1045,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
>   
>   	chip->npwm = npwm;
>   	chip->uses_pwmchip_alloc = true;
> +	chip->operational = false;
>   
>   	pwmchip_dev = &chip->dev;
>   	device_initialize(pwmchip_dev);
> @@ -1102,6 +1151,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   
>   	chip->owner = owner;
>   
> +	if (chip->atomic)
> +		spin_lock_init(&chip->atomic_lock);
> +	else
> +		mutex_init(&chip->nonatomic_lock);
> +
>   	mutex_lock(&pwm_lock);
>   
>   	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
> @@ -1115,6 +1169,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_add(chip);
>   
> +	pwmchip_lock(chip);
> +	chip->operational = true;
> +	pwmchip_unlock(chip);
> +
>   	ret = device_add(&chip->dev);
>   	if (ret)
>   		goto err_device_add;
> @@ -1124,6 +1182,10 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>   	return 0;
>   
>   err_device_add:
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_remove(chip);
>   
> @@ -1145,12 +1207,27 @@ EXPORT_SYMBOL_GPL(__pwmchip_add);
>   void pwmchip_remove(struct pwm_chip *chip)
>   {
>   	pwmchip_sysfs_unexport(chip);
> +	unsigned int i;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	pwmchip_lock(chip);
> +	chip->operational = false;
> +	pwmchip_unlock(chip);
> +
> +	for (i = 0; i < chip->npwm; ++i) {
> +		struct pwm_device *pwm = &chip->pwms[i];
> +
> +		if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +			dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);
> +			if (pwm->chip->ops->free)
> +				pwm->chip->ops->free(pwm->chip, pwm);
> +		}
> +	}
>   
>   	if (IS_ENABLED(CONFIG_OF))
>   		of_pwmchip_remove(chip);
>   
> -	mutex_lock(&pwm_lock);
> -
>   	idr_remove(&pwm_chips, chip->id);
>   
>   	mutex_unlock(&pwm_lock);
> @@ -1532,12 +1609,17 @@ void pwm_put(struct pwm_device *pwm)
>   
>   	mutex_lock(&pwm_lock);
>   
> -	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +	/*
> +	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
> +	 * don't warn in this case. This can only happen if a consumer called
> +	 * pwm_put() twice.
> +	 */
> +	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
>   		pr_warn("PWM device already freed\n");
>   		goto out;
>   	}
>   
> -	if (chip->ops->free)
> +	if (chip->operational && chip->ops->free)
>   		pwm->chip->ops->free(pwm->chip, pwm);
>   
>   	pwm->label = NULL;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 60b92c2c75ef..9c84e0ba81a4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -274,6 +274,9 @@ struct pwm_ops {
>    * @of_xlate: request a PWM device given a device tree PWM specifier
>    * @atomic: can the driver's ->apply() be called in atomic context
>    * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
> + * @operational: signals if the chip can be used (or is already deregistered)
> + * @nonatomic_lock: mutex for nonatomic chips
> + * @atomic_lock: mutex for atomic chips
>    * @pwms: array of PWM devices allocated by the framework
>    */
>   struct pwm_chip {
> @@ -289,6 +292,16 @@ struct pwm_chip {
>   
>   	/* only used internally by the PWM framework */
>   	bool uses_pwmchip_alloc;
> +	bool operational;
> +	union {
> +		/*
> +		 * depending on the chip being atomic or not either the mutex or
> +		 * the spinlock is used. It protects .operational and
> +		 * synchronizes calls to the .ops->apply and .ops->get_state()
> +		 */
> +		struct mutex nonatomic_lock;
> +		struct spinlock atomic_lock;
> +	};
>   	struct pwm_device pwms[] __counted_by(npwm);
>   };
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland