Re: [PATCH 01/18] pwm: Provide devm_pwmchip_alloc() function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 18, 2023 at 2:21 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> On Wed, Oct 18, 2023 at 02:01:30PM -0700, Saravana Kannan wrote:
> > On Wed, Oct 18, 2023 at 4:17 AM Uwe Kleine-König
> > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Oct 17, 2023 at 06:42:40PM -0700, Saravana Kannan wrote:
> > > > On Tue, Oct 17, 2023 at 4:35 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Sat, Oct 14, 2023 at 9:17 AM Uwe Kleine-König
> > > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> > > > > > > On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> > > > > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > Hello Saravana,
> > > > > > > >
> > > > > > > > you were pointed out to me as the expert for device links. I found a
> > > > > > > > problem with these.
> > > > > > > >
> > > > > > > > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > > > > > > > Today I managed to trigger the problem I intend to address with this
> > > > > > > > > series. My machine to test this on is an stm32mp157. To be able to
> > > > > > > > > trigger the problem reliably I applied the following patches on top of
> > > > > > > > > v6.5-rc1:
> > > > > > > > >
> > > > > > > > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > > > > > > > >    This is a cleanup that I already sent out.
> > > > > > > > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@xxxxxxxxxxxxxx
> > > > > > > > >    The purpose for reproducing the problem is to not trigger further
> > > > > > > > >    calls to the apply callback.
> > > > > > > > >
> > > > > > > > >  - The following patch:
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > > > > > > > index 687967d3265f..c7fc02b0fa3c 100644
> > > > > > > > > --- a/drivers/pwm/pwm-stm32.c
> > > > > > > > > +++ b/drivers/pwm/pwm-stm32.c
> > > > > > > > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > > > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > > > > > > > >       int ret;
> > > > > > > > >
> > > > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > > +     msleep(5000);
> > > > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > > +
> > > > > > > > >       enabled = pwm->state.enabled;
> > > > > > > > >
> > > > > > > > >       if (enabled && !state->enabled) {
> > > > > > > > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > > > > > > > >  {
> > > > > > > > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > > > > > > > >
> > > > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > >       pwmchip_remove(&priv->chip);
> > > > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > > +
> > > > > > > > > +     priv->regmap = NULL;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > > > > > > > >
> > > > > > > > > The first hunk is only there to widen the race window. The second is to
> > > > > > > > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > > > > > > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > > > > > > > understand why. *shrug*)
> > > > > > > > >
> > > > > > > > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > > > > > > > >
> > > > > > > > > Now I do the following:
> > > > > > > > >
> > > > > > > > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > > > >
> > > > > > > > > Unbinding the fan device has two effects:
> > > > > > > > >
> > > > > > > > >  - The device link between fan and pwm looses its property to unbind fan
> > > > > > > > >    when pwm gets unbound.
> > > > > > > > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > > > > > > > >  - It calls pwm_fan_cleanup() which triggers a call to
> > > > > > > > >    pwm_apply_state().
> > > > > > > > >
> > > > > > > > > So when the pwm device gets unbound the first thread is sleeping in
> > > > > > > > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > > > > > > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > > > > > > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > > > > > > > >
> > > > > > > > > This looks as follows:
> > > > > > > > >
> > > > > > > > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > > > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > > > > > > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > > > > > > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > > > > > > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > > > > > > > [  192.240727] 8<--- cut here ---
> > > > > > > > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > Even without the crash you can see that stm32_pwm_apply() is still
> > > > > > > > > running after pwmchip_remove() completed.
> > > > > > > > >
> > > > > > > > > I'm unsure if the device link could be improved here to ensure that the
> > > > > > > > > fan is completely unbound even if it started unbinding already before
> > > > > > > > > the pwm device gets unbound. (And if it could, would this fit the device
> > > > > > > > > links purpose and so be a sensible improvement?)
> > > > > > > >
> > > > > > > > While I think that there is something to be done in the pwm core that
> > > > > > > > this doesn't explode (i.e. do proper lifetime tracking such that a
> > > > > > > > pwm_chip doesn't disappear while still being used---and I'm working on
> > > > > > > > that) I expected that the device links between pwm consumer and provider
> > > > > > > > would prevent the above described oops, too. But somehow the fan already
> > > > > > > > going away (but still using the PWM) when the PWM is unbound, results in
> > > > > > > > the PWM disappearing before the fan is completely gone.
> > > > > > > >
> > > > > > > > Is this expected, or a problem that can (and should?) be fixed?
> > > > > > >
> > > > > > > I didn't read your full series, but I read this email. With what's in
> > > > > > > this email, the problem seems to be in the driver or the pwm
> > > > > > > framework. The pwm driver/framework can't tell the driver core that
> > > > > > > you successfully unbound (returning from .remove()) before you have
> > > > > > > finish all your ongoing transactions with the device. If your
> > > > > > > "apply()" is still running, you need to make sure it's complete before
> > > > > > > .remove() does any resource releasing/clean up.
> > > > > > >
> > > > > > > Also, how is the consumer driver's .remove() succeeding if it has an
> > > > > > > ongoing pwm call()?
> > > > > >
> > > > > > The thing that works fine and as expected is:
> > > > > >
> > > > > >  - trigger unbind of PWM device via sysfs
> > > > > >
> > > > > > Because there is a device link PWM provider -> pwm consumer (fan), the
> > > > > > fan is removed and once its gone (and not earlier), the PWM gets unbound.
> > > > > >
> > > > > > The failing sequence is:
> > > > > >
> > > > > >  - trigger unbind of fan device in userspace thread A via sysfs. The
> > > > > >    fan's remove callback blocks for 5s in pwm_apply_state() and so
> > > > > >    .remove() doesn't complete yet.
> > > > > >
> > > > > >  - a second later: trigger unbind of PWM device via sysfs in thread B.
> > > > > >    As before I'd expect that the device link results in waiting for the
> > > > > >    fan to be removed completely, but the PWM is removed immediately.
> > > > > >
> > > > > >  - pwm_apply_state's sleep completes (in thread B) and operates on freed
> > > > > >    resources => bang!
> > > > > >
> > > > > > > This all sounds like insufficient locking and
> > > > > > > critical region protection in both the consumer and supplier.
> > > > > >
> > > > > > My (and I think also Thierry's) expectation was, that the device link
> > > > > > provides the needed synchronisation. But it doesn't as it doesn't block
> > > > > > the PWM provider going away until the fan is completely gone.
> > > > > >
> > > > > > > Device links can't do anything here because you are giving it wrong
> > > > > > > info -- that the unbind was successful before it actually is.
> > > > > >
> > > > > > The fan's unbind is ongoing, but not complete yet and I'd expect that
> > > > > > the device link blocks unbinding the PWM until the fan is completely
> > > > > > gone. So I think there is no wrong information.
> > > > > >
> > > > > > > Device links will and can make sure that the consumer is unbound
> > > > > > > successfully before the unbind is called on the supplier. And it looks
> > > > > > > like that's still true here.
> > > > > >
> > > > > > I hope you understood the situation better now and see the problem we
> > > > > > have.
> > > > > >
> > > > > > The problem is fixable in the pwm framework (and I'm working on that),
> > > > > > but I think there is also something to improve around devicelink
> > > > > > handling.
> > > > >
> > > > > Thanks for a better explanation of the issue. I agree, this seems like
> > > > > something device links should be able to take care of.
> > > > >
> > > > > I'll take a look into this.
> > > >
> > > > Took me a while to debug this because I couldn't find the .remove()
> > > > function and I was very confused about what's going on.
> > > > I'm guessing you started hitting this issue only after moving to the
> > > > devm_ variant of the pwm APIs.
> > >
> > > Ah I see. That problem wouldn't happen if the fan called a pwm API
> > > function in its remove callback but that happens in a devm cleanup call
> > > (registered by devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx) in
> > > pwm_fan_probe()). I first thought you talked about
> > > 8c89fd866ad221af037ef0ec3d60b83d0b859c65.
> >
> > Am I not talking about that commit?
>
> The relevant thing is that the fan (i.e. the consumer) uses
> pwm_apply_state() in a devm cleanup. If the pwm provider uses
> devm_pwmchip_add or plain pwmchip_add + pwmchip_remove in .remove()
> doesn't matter.

Duh! For whatever reason I had forgotten that stm32 was the supplier
and was confusing myself.

>
> > Btw, I'm still a bit confused by this thread. In your earlier emails
> > to me, you said:
> >
> > >  - trigger unbind of fan device in userspace thread A via sysfs. The
> > >  fan's remove callback blocks for 5s in pwm_apply_state() and so
> > >  .remove() doesn't complete yet.
> >
> > But the latest tree (Tot) didn't have any .remove() function at all.
> > So I just decided to see if there's any issue in Tot and just fix
> > that. I'm glad my fix helps fixed the issue with the used of devm_*()
> > APIs.
>
> I was sloppy here and called it "remove callback" when it was really a
> devm cleanup call. Sorry if that confused you. I didn't expect this to
> make a difference (and I'm not even sure I was aware this is a devm
> cleanup and not directly .remove() when I wrote that).
>
> > However, are you really seeing the issue (supplier freed before
> > consumer) if you do the clean up in the .remove() function? If so,
> > there might still be another issue that needs to be fixed.
>
> I didn't test that and now having understood the issue you fixed and
> seeing the effect, I confidently claim there is nothing to fix for
> drivers that use pwm_apply_state in .remove().

Yeah, I'm convinced now too!

-Saravana





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux