Re: [PATCH v11 3/5] pwm: sun8i: export the bypass outputs as clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] pwm: sun8i: missed rate protection in sun8i_pwm_pair_rate_constrained()
- [Medium] pwm: sun8i: sun8i_pwm_pair_rate_notifier() blocks bypass clk_set_rate()
- [Medium] pwm: sun8i: sun8i_pwm_read_waveform() clamps concurrent clk_set_rate()
- [Medium] pwm: sun8i: torn rate and constraint snapshot in sun8i_pwm_round_waveform_tohw()
--

commit 77adf52feb5fc5043dc604aa0ff2272bf2185bb6
Author: Richard Genoud <richard.genoud@bootlin.com>

pwm: sun8i: export the bypass outputs as clocks

This commit exports each Allwinner H616 PWM channel's hardware bypass path
as a Common Clock Framework clock and arbitrates channel ownership and pair
rate protection between PWM consumers and clock consumers.

> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> index bcc2c57443780..56854e6218c9a 100644
> --- a/drivers/pwm/pwm-sun8i.c
> +++ b/drivers/pwm/pwm-sun8i.c

[ ... ]

> @@ -399,12 +519,20 @@ static int sun8i_pwm_read_waveform(struct pwm_chip *chip,
>  	struct sun8i_pwm_waveform *wfhw = _wfhw;
>  	struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
>  	unsigned int pair = SUN8I_PWM_PAIR_IDX(pwm->hwpwm);
> +	struct clk *pair_clk = sun8i_chip->pairs[pair].hw->clk;
>  	unsigned long pair_rate;
>  	u32 pccr, pcr, pdzcr, per, ppr;
> +	int ret;
>  
> -	pair_rate = sun8i_pwm_get_pair_rate(sun8i_chip, pwm->hwpwm);
> -	if (pair_rate > U32_MAX)
> -		return -ERANGE;
> +	/*
> +	 * An unrequested channel can be retuned and enabled by a clock
> +	 * consumer. Keep its rate stable until the register snapshot is taken.
> +	 * Use the provider handle, separate from the PWM rate-setting handles.
> +	 */
> +	ret = clk_rate_exclusive_get(pair_clk);
> +	if (ret)
> +		return ret;
> +	pair_rate = clk_get_rate(pair_clk);
>  
>  	scoped_guard(spinlock_irqsave, &sun8i_chip->lock) {
>  		pccr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair));
> @@ -413,6 +541,10 @@ static int sun8i_pwm_read_waveform(struct pwm_chip *chip,
>  		ppr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PPR(pwm->hwpwm));
>  		pdzcr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PDZCR(pair));
>  	}
> +	clk_rate_exclusive_put(pair_clk);

[Severity: Medium]
Could holding clk_rate_exclusive_get(pair_clk) across sun8i_pwm_read_waveform()
cause concurrent clk_set_rate(), clk_set_rate_exclusive(), or clk_round_rate()
calls on a child bypass clock to silently clamp to the existing rate?

In drivers/clk/clk.c, clk_rate_exclusive_get(pair_clk) increments
pair->hw->core->protect_count and releases prepare_lock before returning.
While sun8i_pwm_read_waveform() is between clk_rate_exclusive_get(pair_clk)
and clk_rate_exclusive_put(pair_clk), a clock consumer calling
clk_set_rate(bypass_clk, new_rate) or clk_set_rate_exclusive(bypass_clk,
new_rate) on chan->bypass_hw runs clk_core_set_rate_nolock() ->
clk_core_req_round_rate_nolock().

Because the temporary rate protection was taken directly on the parent
pair_clk handle rather than through bypass_clk, bypass_clk->core->protect_count
is 0, so clk_core_rate_nuke_protect() does not discount the hold on
pair->hw->core. When clk_core_round_rate_nolock() forwards the request to
pair->hw->core via CLK_SET_RATE_PARENT, clk_core_determine_round_nolock()
sees clk_core_rate_is_protected(pair->hw->core) is true and clamps the
request to the current pair rate, causing clk_core_set_rate_nolock() to
see no rate change and return 0 without updating the hardware clock rate.

[ ... ]

> @@ -510,7 +642,8 @@ sun8i_pwm_pair_rate_constrained(struct sun8i_pwm_chip *sun8i_chip,
>  	unsigned int sibling = idx ^ 1;
>  
>  	guard(spinlock_irqsave)(&sun8i_chip->lock);
> -	return sun8i_chip->channels[sibling].rate_exclusive ||
> +	return sun8i_chip->channels[sibling].mode == SUN8I_PWM_MODE_CLK ||
> +	       sun8i_chip->channels[sibling].rate_exclusive ||
>  	       sun8i_pwm_channel_is_enabled_locked(sun8i_chip, sibling);
>  }

[Severity: Medium]
What happens if a consumer of the sibling bypass clock holds rate exclusivity
via clk_rate_exclusive_get() while the bypass clock is unprepared?

In drivers/net/phy/xpowers/ac300.c, ac300_ephy_ctl_create() locks the bypass
clock rate at probe time with devm_clk_rate_exclusive_get(dev, priv->clk),
while only calling clk_prepare_enable(priv->clk) in ac300_ephy_ctl_power_on()
and calling clk_disable_unprepare(priv->clk) in ac300_ephy_ctl_disable().

When the bypass clock is unprepared, sun8i_pwm_bypass_unprepare() resets
chan->mode to SUN8I_PWM_MODE_NONE:

drivers/pwm/pwm-sun8i.c:sun8i_pwm_bypass_unprepare() {
	...
	guard(spinlock_irqsave)(&sun8i_chip->lock);
	if (chan->mode != SUN8I_PWM_MODE_CLK)
		return;

	chan->mode = SUN8I_PWM_MODE_NONE;
}

Even though pair->hw->core->protect_count remains non-zero in CCF from the
consumer's clk_rate_exclusive_get(), sun8i_pwm_pair_rate_constrained() sees
mode != SUN8I_PWM_MODE_CLK, rate_exclusive == false, and the sibling channel
disabled in hardware, so it returns false.

As a result, sun8i_pwm_round_waveform_tohw() treats the pair as unconstrained
and can pick a candidate waveform with a different pair_rate. Then in
sun8i_pwm_write_waveform() -> sun8i_pwm_prepare_update(),
sun8i_pwm_pair_rate_constrained() also returns false, so
sun8i_pwm_prepare_update() disables the active PWM channel in hardware when
stop is true and calls clk_set_rate(chan->pair_clk, wfhw->pair_rate), which
fails with -EBUSY in CCF because the pair clock is still rate-protected.
In addition, because chan->mode is SUN8I_PWM_MODE_NONE while unprepared,
sun8i_pwm_request() can claim the channel for PWM, causing the clock
consumer's next clk_prepare_enable() to fail with -EBUSY in
sun8i_pwm_bypass_prepare().

[Severity: Medium]
Can sun8i_pwm_round_waveform_tohw() also observe a mismatched pair_rate and
constraint state when racing with a sibling bypass clock consumer?

In sun8i_pwm_round_waveform_tohw(), current_rate is sampled before checking
sun8i_pwm_pair_rate_constrained():

drivers/pwm/pwm-sun8i.c:sun8i_pwm_round_waveform_tohw() {
	...
	current_rate = sun8i_pwm_get_pair_rate(sun8i_chip, pwm->hwpwm);
	if (!current_rate || current_rate > U32_MAX)
		return -ERANGE;
	rounding.current_pair_rate = current_rate;

	if (sun8i_pwm_pair_rate_constrained(sun8i_chip, pwm->hwpwm)) {
		sun8i_pwm_consider_pair_rate(&rounding, current_rate);
	...
}

When the PWM channel is currently disabled (so chan->rate_exclusive is false),
sun8i_pwm_round_waveform_tohw() does not hold rate exclusivity on the pair
clock. Because CCF operations on the sibling's bypass clock do not acquire
pwmchip_lock(chip), a concurrent sibling clock consumer can call
clk_set_rate() and clk_prepare_enable() after sun8i_pwm_get_pair_rate()
returns the old rate but before sun8i_pwm_pair_rate_constrained() checks
channels[sibling].mode == SUN8I_PWM_MODE_CLK.

Wouldn't sun8i_pwm_round_waveform_tohw() then round exclusively against the
stale pre-retune current_rate instead of the rate at which the sibling just
locked the pair clock?

> @@ -1033,6 +1166,42 @@ static int sun8i_pwm_register_pair_clocks(struct device *dev,
>  	return 0;
>  }
>  
> +/* Register the bypass clock for each channel. */
> +static int sun8i_pwm_register_bypass_clocks(struct device *dev,
> +					    struct sun8i_pwm_chip *sun8i_chip)
> +{
> +	for (unsigned int i = 0; i < SUN8I_PWM_NPWM; i++) {
> +		struct sun8i_pwm_channel *chan = &sun8i_chip->channels[i];
> +		struct clk_hw *parent = sun8i_chip->pairs[SUN8I_PWM_PAIR_IDX(i)].hw;
> +		const char *name;
> +		int ret;
> +
> +		name = devm_kasprintf(dev, GFP_KERNEL, "%s#pwm-bypass%u",
> +				      dev_name(dev), i);
> +		if (!name)
> +			return -ENOMEM;
> +
> +		chan->chip = sun8i_chip;
> +		chan->index = i;
> +		/*
> +		 * Protect the shared rate while prepared. Preserve unclaimed
> +		 * firmware outputs through CCF's unused-clock cleanup, as for
> +		 * PWM waveforms; only a consumer may turn them off.
> +		 */
> +		chan->bypass_hw.init =
> +			CLK_HW_INIT_HW(name, parent, &sun8i_pwm_bypass_ops,
> +				       CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE |
> +				       CLK_IGNORE_UNUSED);

[Severity: Medium]
If firmware leaves a channel enabled at boot, will clk_set_rate() on that
channel's bypass clock always be rejected with -EBUSY by
sun8i_pwm_pair_rate_notifier()?

When firmware leaves a channel enabled (with SUN8I_PWM_PCCR_GATE and
SUN8I_PWM_ENABLE(i) set), sun8i_pwm_sanitize_disabled_pairs() and
CLK_IGNORE_UNUSED preserve the enabled state in hardware. Because
chan->bypass_hw is registered with CLK_SET_RATE_GATE, clk_prepare() protects
the clock rate in CCF, so any clk_set_rate() call (including from DT
assigned-clock-rates or a consumer before clk_prepare_enable()) must happen
while chan->bypass_hw is still unprepared and its CCF enable_count is 0.

However, the hardware enable bit SUN8I_PWM_ENABLE(i) in SUN8I_PWM_PER is
only cleared later in sun8i_pwm_bypass_enable() or sun8i_pwm_bypass_disable().
When clk_set_rate() is called on the unprepared bypass clock, CCF propagates
PRE_RATE_CHANGE to pair->hw while SUN8I_PWM_ENABLE(i) is still set in
SUN8I_PWM_PER:

drivers/pwm/pwm-sun8i.c:sun8i_pwm_pair_rate_notifier() {
	...
	guard(spinlock_irqsave)(&sun8i_chip->lock);
	active = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER) &
		 sun8i_pwm_pair_enable_mask(pair->index);

	return active ? NOTIFY_BAD : NOTIFY_OK;
}

Unlike sun8i_pwm_prepare_update() on the PWM path, which clears
SUN8I_PWM_ENABLE(idx) before calling clk_set_rate(), nothing clears
SUN8I_PWM_ENABLE(i) before a bypass clock rate change, so
sun8i_pwm_pair_rate_notifier() sees active is true and aborts the rate change
with NOTIFY_BAD (-EBUSY).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-h616-pwm-v8-v11-0-07abb59575c3@gmail.com?part=3