lkml.org 
[lkml]   [2022]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v2] pwm: sifive: Always let the first pwm_apply_state succeed
    On Wed, 9 Nov 2022 at 16:33, Uwe Kleine-König
    <u.kleine-koenig@pengutronix.de> wrote:
    >
    > On Wed, Nov 09, 2022 at 01:45:43PM +0100, Emil Renner Berthing wrote:
    > > On Wed, 9 Nov 2022 at 13:01, Uwe Kleine-König
    > > <u.kleine-koenig@pengutronix.de> wrote:
    > > >
    > > > Hello Emil,
    > > >
    > > > On Wed, Nov 09, 2022 at 12:37:24PM +0100, Emil Renner Berthing wrote:
    > > > > Commit 2cfe9bbec56ea579135cdd92409fff371841904f added support for the
    > > > > RGB and green PWM controlled LEDs on the HiFive Unmatched board
    > > > > managed by the leds-pwm-multicolor and leds-pwm drivers respectively.
    > > > > All three colours of the RGB LED and the green LED run from different
    > > > > lines of the same PWM, but with the same period so this works fine when
    > > > > the LED drivers are loaded one after the other.
    > > > >
    > > > > Unfortunately it does expose a race in the PWM driver when both LED
    > > > > drivers are loaded at roughly the same time. Here is an example:
    > > > >
    > > > > | Thread A | Thread B |
    > > > > | led_pwm_mc_probe | led_pwm_probe |
    > > > > | devm_fwnode_pwm_get | |
    > > > > | pwm_sifive_request | |
    > > > > | ddata->user_count++ | |
    > > > > | | devm_fwnode_pwm_get |
    > > > > | | pwm_sifive_request |
    > > > > | | ddata->user_count++ |
    > > > > | ... | ... |
    > > > > | pwm_state_apply | pwm_state_apply |
    > > > > | pwm_sifive_apply | pwm_sifive_apply |
    > > > >
    > > > > Now both calls to pwm_sifive_apply will see that ddata->approx_period,
    > > > > initially 0, is different from the requested period and the clock needs
    > > > > to be updated. But since ddata->user_count >= 2 both calls will fail
    > > > > with -EBUSY, which will then cause both LED drivers to fail to probe.
    > > > >
    > > > > Fix it by letting the first call to pwm_sifive_apply update the clock
    > > > > even when ddata->user_count != 1.
    > > > >
    > > > > Fixes: 9e37a53eb051 ("pwm: sifive: Add a driver for SiFive SoC PWM")
    > > > > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
    > > > > ---
    > > > > drivers/pwm/pwm-sifive.c | 8 +++++++-
    > > > > 1 file changed, 7 insertions(+), 1 deletion(-)
    > > > >
    > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
    > > > > index 2d4fa5e5fdd4..b3c60ec72a6e 100644
    > > > > --- a/drivers/pwm/pwm-sifive.c
    > > > > +++ b/drivers/pwm/pwm-sifive.c
    > > > > @@ -159,7 +159,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
    > > > >
    > > > > mutex_lock(&ddata->lock);
    > > > > if (state->period != ddata->approx_period) {
    > > > > - if (ddata->user_count != 1) {
    > > > > + /*
    > > > > + * Don't let a 2nd user change the period underneath the 1st user.
    > > > > + * However if ddate->approx_period == 0 this is the first time we set
    > > > > + * any period, so let whoever gets here first set the period so other
    > > > > + * users who agree on the period won't fail.
    > > > > + */
    > > > > + if (ddata->user_count != 1 && ddata->approx_period) {
    > > >
    > > > While I'm convinced this works, we'd get some more uniform behaviour
    > > > compared to other hardwares with similar restrictions if you lock the
    > > > period on enabling the PWM instead of at request time. See for example
    > > > drivers/pwm/pwm-pca9685.c.
    > >
    > > Hmm.. that driver uses a pwms_enabled bitmap rather than a user count,
    > > but it still sets the bit in the request method and refuses to change
    > > period in the apply method if more than 1 bit is set.
    >
    > Note there are two different bitmaps. The one modified in .request is
    > for gpio stuff and the other in .apply() for locking the common period
    > length.

    Yeah, there is the pwms_enabled and pwms_inuse bitmaps, but
    pwms_enabled is used both in .request and .apply.

    > > So as far as I
    > > can tell it still suffers from the same race. However using a bitmap
    > > instead of a user count would let us handle everything in the apply
    > > method if we don't set the bit in the request method, but then the
    > > behaviour would still be different. In any case it would still be a
    > > large change to this driver.
    > >
    > > How about we merge this bug fix that can easily be backported first
    > > and then look at how it should be handled properly?
    >
    > I thought it wouldn't be that hard to do it right from the start,
    > but I admit it's harder than I expected to get right. My prototype looks
    > as follows:

    This works for me (modulo the two extra {'s). I'd still prefer merging
    the simpler version and then this on top for ease of backporting, but
    as long as the race is fixed I'm fine. Will you send a cleaned up
    version of this?

    /Emil

    > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
    > index 2d4fa5e5fdd4..89846d95bfc0 100644
    > --- a/drivers/pwm/pwm-sifive.c
    > +++ b/drivers/pwm/pwm-sifive.c
    > @@ -41,13 +41,13 @@
    >
    > struct pwm_sifive_ddata {
    > struct pwm_chip chip;
    > - struct mutex lock; /* lock to protect user_count and approx_period */
    > + struct mutex lock; /* lock to protect approx_period */
    > struct notifier_block notifier;
    > struct clk *clk;
    > void __iomem *regs;
    > unsigned int real_period;
    > unsigned int approx_period;
    > - int user_count;
    > + DECLARE_BITMAP(pwms_enabled, 4);
    > };
    >
    > static inline
    > @@ -59,10 +59,16 @@ struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
    > static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *pwm)
    > {
    > struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
    > + u32 val = readl(ddata->regs + PWM_SIFIVE_PWMCFG);
    >
    > - mutex_lock(&ddata->lock);
    > - ddata->user_count++;
    > - mutex_unlock(&ddata->lock);
    > + if (val & PWM_SIFIVE_PWMCFG_EN_ALWAYS) {
    > + val = readl(ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
    > + if (val > 0) {
    > + mutex_lock(&ddata->lock);
    > + __set_bit(pwm->hwpwm, ddata->pwms_enabled);
    > + mutex_unlock(&ddata->lock);
    > + }
    > + }
    >
    > return 0;
    > }
    > @@ -72,7 +78,7 @@ static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *pwm)
    > struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
    >
    > mutex_lock(&ddata->lock);
    > - ddata->user_count--;
    > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled);
    > mutex_unlock(&ddata->lock);
    > }
    >
    > @@ -158,11 +164,18 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
    > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
    >
    > mutex_lock(&ddata->lock);
    > +
    > + if (state->enabled) {
    > + __set_bit(pwm->hwpwm, ddata->pwms_enabled);
    > +
    > if (state->period != ddata->approx_period) {
    > - if (ddata->user_count != 1) {
    > + if (bitmap_weight(ddata->pwms_enabled, 4) > 1) {
    > + if (!enabled) {
    > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled);
    > mutex_unlock(&ddata->lock);
    > return -EBUSY;
    > }
    > +
    > ddata->approx_period = state->period;
    > pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
    > }
    > @@ -177,14 +190,23 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
    > ret = clk_enable(ddata->clk);
    > if (ret) {
    > dev_err(ddata->chip.dev, "Enable clk failed\n");
    > + if (state->enabled) {
    > + mutex_lock(&ddata->lock);
    > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled);
    > + mutex_unlock(&ddata->lock);
    > + }
    > return ret;
    > }
    > }
    >
    > writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
    >
    > - if (!state->enabled)
    > + if (!state->enabled) {
    > + mutex_lock(&ddata->lock);
    > + __clear_bit(pwm->hwpwm, ddata->pwms_enabled);
    > + mutex_unlock(&ddata->lock);
    > clk_disable(ddata->clk);
    > + }
    >
    > return 0;
    > }
    >
    > Best regards
    > Uwe
    >
    > --
    > Pengutronix e.K. | Uwe Kleine-König |
    > Industrial Linux Solutions | https://www.pengutronix.de/ |

    \
     
     \ /
      Last update: 2022-11-16 18:42    [W:2.393 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site