On Fri, Oct 16, 2020 at 12:21:27PM +0300, Andy Shevchenko wrote:

On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote:

quoted

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: Thursday, 15 October, 2020 4:12 PM
On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote:

quoted

From: Vijayakannan Ayyathurai <redacted>

...

quoted

quoted

+	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)) {

quoted

+		clk_disable_unprepare(priv->clk);

See below.

quoted

+		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));

quoted

+		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.

quoted

+		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

quoted

+}

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.

The easiest way is probably to do the clk_prepare_enable() only after
all devm_ stuff in .probe. (But I'm not emphatic about that, it would be
fine for me to wait until devm_clk_get_enabled() lands.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |