2022-04-05 01:41:56

by Biju Das

Subject: RE: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

Hi Prabhakar and Pavel,

Thanks for the patch.

> Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of
> returning directly
>
> Jump to error path "edisclk" instead of returning directly in case of
> devm_reset_control_get_optional_exclusive() failure.
>
> Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible")
> Reported-by: Pavel Machek <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/mmc/host/renesas_sdhi_core.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> b/drivers/mmc/host/renesas_sdhi_core.c
> index 2797a9c0f17d..cddb0185f5fb 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device
> *pdev,
> goto efree;
>
> priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev,
> NULL);
> - if (IS_ERR(priv->rstc))
> - return PTR_ERR(priv->rstc);
> + if (IS_ERR(priv->rstc)) {
> + ret = PTR_ERR(priv->rstc);
> + goto edisclk;
> + }

Why can't devm_reset_control_get_optional_exclusive to be moved up before devm_clk_get?

Cheers,
Biju

>
> ver = sd_ctrl_read16(host, CTL_VERSION);
> /* GEN2_SDR104 is first known SDHI to use 32bit block count */
> --
> 2.17.1

2022-04-05 02:51:47

by Lad, Prabhakar

Subject: Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of returning directly

Hi Biju,

Thank you for the review.

On Mon, Apr 4, 2022 at 7:02 PM Biju Das <[email protected]> wrote:
>
> Hi Prabhakar and Pavel,
>
> Thanks for the patch.
>
> > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of
> > returning directly
> >
> > Jump to error path "edisclk" instead of returning directly in case of
> > devm_reset_control_get_optional_exclusive() failure.
> >
> > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible")
> > Reported-by: Pavel Machek <[email protected]>
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > drivers/mmc/host/renesas_sdhi_core.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > b/drivers/mmc/host/renesas_sdhi_core.c
> > index 2797a9c0f17d..cddb0185f5fb 100644
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device
> > *pdev,
> > goto efree;
> >
> > priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev,
> > NULL);
> > - if (IS_ERR(priv->rstc))
> > - return PTR_ERR(priv->rstc);
> > + if (IS_ERR(priv->rstc)) {
> > + ret = PTR_ERR(priv->rstc);
> > + goto edisclk;
> > + }
>
> Why can't devm_reset_control_get_optional_exclusive to be moved up before devm_clk_get?
>
In that case we will have to jump to the "efree" label Or if you don't
want goto at all this can be moved to the very beginning of the probe.

Wolfram, what is your preference on the above?

Cheers,
Prabhakar