Re: [PATCH] mailbox: imx: Fix clk handling in imx_mu_probe()

[Oleksij Rempel]

On 17.12.18 08:24, Alexey Khoroshilov wrote:
> Hi Oleksij,
>
> By chance I took a look at another implementations:
>
> arch/arm/mach-ep93xx/clock.c#L266
>
> int clk_enable(struct clk *clk)
> {
> 	unsigned long flags;
>
> 	if (!clk)
> 		return -EINVAL;
> ...
>
> arch/c6x/platforms/pll.c#L48
>
> int clk_enable(struct clk *clk)
> {
> 	unsigned long flags;
>
> 	if (clk == NULL || IS_ERR(clk))
> 		return -EINVAL;
>
> So, I am not sure the NULL resistance is a part of the clk_enable()
> contract?

Main framework is always preferred. If some local code provide 
replacement of a functions already provided by the clk framework and not 
follow same rules, then this code should be fixed. If this code is even 
exporting this function for global use, then it is probably buggy.
And if i see it correctly:
1d81eedb8f6c1 (Lennert Buytenhek  2006-06-24 10:33:02 +0100 266) int 
clk_enable(struct clk *clk)

It is ancient code... it just can't be correct.

> --
> Alexey
>
>
> On 17.12.2018 9:01, Oleksij Rempel wrote:
> > Hi Alexey,
> >
> > On Sun, Dec 16, 2018 at 02:01:44AM +0300, Alexey Khoroshilov wrote:
> > > Handling of devm_clk_get() suggests that the driver should support
> > > lack of priv->clk. But imx_mu_probe() fails on clk_prepare_enable(NULL)
> > > in that case.
> > >
> > > The patch removes the try to enable absent clk and adds error handling
> > > for mbox_controller_register().
> > >
> > > Found by Linux Driver Verification project (linuxtesting.org).
> > >
> > > Signed-off-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
> > > ---
> > >   drivers/mailbox/imx-mailbox.c | 18 +++++++++++++-----
> > >   1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> > > index 363d35d5e49d..ddde398f576e 100644
> > > --- a/drivers/mailbox/imx-mailbox.c
> > > +++ b/drivers/mailbox/imx-mailbox.c
> > > @@ -292,10 +292,12 @@ static int imx_mu_probe(struct platform_device *pdev)
> > >   		priv->clk = NULL;
> > >   	}
> > >   
> > > -	ret = clk_prepare_enable(priv->clk);
> > > -	if (ret) {
> > > -		dev_err(dev, "Failed to enable clock\n");
> > > -		return ret;
> > > +	if (priv->clk) {
> > > +		ret = clk_prepare_enable(priv->clk);
> > > +		if (ret) {
> > > +			dev_err(dev, "Failed to enable clock\n");
> > > +			return ret;
> > > +		}
> > >   	}
> > >   
> > >   	for (i = 0; i < IMX_MU_CHANS; i++) {
> > > @@ -324,7 +326,13 @@ static int imx_mu_probe(struct platform_device *pdev)
> > >   
> > >   	imx_mu_init_generic(priv);
> > >   
> > > -	return mbox_controller_register(&priv->mbox);
> > > +	ret = mbox_controller_register(&priv->mbox);
> > > +	if (ret) {
> > > +		clk_disable_unprepare(priv->clk);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > >   }
> > >   
> > >   static int imx_mu_remove(struct platform_device *pdev)
> > > --
> > > 2.7.4
> >
> > NACK for this patch.
> >
> > Here are code snippets from clk framework:
> > ============================================================================
> > /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
> > static inline int clk_prepare_enable(struct clk *clk)
> > {
> > 	int ret;
> >
> > 	ret = clk_prepare(clk);
> > 	if (ret)
> > 		return ret;
> > 	ret = clk_enable(clk);
> > 	if (ret)
> > 		clk_unprepare(clk);
> >
> > 	return ret;
> > }
> >
> > int clk_prepare(struct clk *clk)
> > {
> > 	if (!clk)
> > 		return 0;
> >
> > 	return clk_core_prepare_lock(clk->core);
> > }
> >
> > int clk_enable(struct clk *clk)
> > {
> > 	if (!clk)
> > 		return 0;
> >
> > 	return clk_core_enable_lock(clk->core);
> > }
> > ============================================================================
> >
> > As you can see, complete code path is NULL resistant. Are you trying to
> > fix some real issue, or it is a theoretical work?