On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote:
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Thursday, 15 October, 2020 4:12 PM
> On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@xxxxxxxxx wrote:
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@xxxxxxxxx>
...
> > + priv->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(priv->clk))
> > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get
> > +clock\n");
> > +
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret)
> > + return ret;
> > +
> > + priv->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(priv->base)) {
>
> > + clk_disable_unprepare(priv->clk);
>
> See below.
>
> > + return PTR_ERR(priv->base);
> > + }
> > +
> > + priv->chip.base = -1;
> > + priv->chip.dev = dev;
> > + priv->chip.ops = &keembay_pwm_ops;
> > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> > +
> > + ret = pwmchip_add(&priv->chip);
> > + if (ret) {
> > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
>
> > + clk_disable_unprepare(priv->clk);
>
> This messes up with ordering of things.
>
> That's why devm golden rule is either all or none. You may fix this by switching to devm_add_action_or_reset().
>
> One of possible way is like in below drivers:
>
> % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/
>
> But it may be fixed in follow up change. Depends on maintainers' wishes.
>
> Sure. I shall incorporate and check based on maintainers wish in the next version.
>
> > + return ret;
> > + }
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int keembay_pwm_remove(struct platform_device *pdev) {
> > + struct keembay_pwm *priv = platform_get_drvdata(pdev);
> > + int ret;
> > +
> > + ret = pwmchip_remove(&priv->chip);
> > + clk_disable_unprepare(priv->clk);
> > +
> > + return ret;
>
> ...and this will be simplified to
>
> return pwmchip_remove(&priv->chip);
>
> Until v10, It is as per your suggestion. But I have changed it in v11 to overcome the issue mentioned by Uwe. I have kept the snip of v10 FYR below.
>
> //Start snip from v10 review mailing list
> //> +static int keembay_pwm_remove(struct platform_device *pdev) {
> //> + struct keembay_pwm *priv = platform_get_drvdata(pdev);
> //> +
> //> + clk_disable_unprepare(priv->clk);
> //> +
> //> + return pwmchip_remove(&priv->chip);
> //
> //You have to call pwmchip_remove first. Otherwise you're stopping the PWM while the framework still believes everything to be fine.
> //
> //> +}
> //End snip from v10 review mailing review
>
> > +}
What I said does not contradict with what Uwe said. So, please, fix ordering
either by dropping devm_ in the middle or adding devm_ action.
Now you moved serious ordering issue in ->remove() (which Uwe noted) to less
serious in ->probe(). But issue is still present.
--
With Best Regards,
Andy Shevchenko
