Re: [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver

[Andy Shevchenko]

On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> On 5/12/22 15:25, Andy Shevchenko wrote:
> > On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@xxxxxxxxx> wrote:
> >> On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> >>> On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@xxxxxxxxx> wrote:

...

> >>>> +       pltfm_host->clk = devm_clk_get_optional(&pdev->dev, NULL);
> >>>
> >>> You can't mix devm with non-devm in this way.
> >> Can you explain what you mean You can't mix devm with non-devm in this
> >> way, where is the mix?
> >> In version 1 used devm_clk_get, is it problematic?
> >
> > devm_ is problematic in your case.
> > TL;DR: you need to use clk_get_optional() and clk_put().
>
> devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be
not problematic. To be on the safe side, the best approach is to make
sure that allocated resources are being deallocated in the reversed
order. That said, the

1. call non-devm_func()
2. call devm_func()

is wrong strictly speaking.

> > Your ->remove() callback doesn't free resources in the reversed order
> > which may or, by luck, may not be the case of all possible crashes,
> > UAFs, races, etc during removal stage. All the same for error path in
> > ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

> >>>> +       if (IS_ERR(pltfm_host->clk))
> >>>> +               return PTR_ERR(pltfm_host->clk);
> >>>> +
> >>>> +       ret = clk_prepare_enable(pltfm_host->clk);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>> +
> >>>> +       caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> >>>> +       if (caps & SDHCI_CAN_DO_8BIT)
> >>>> +               host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> >>>> +
> >>>> +       ret = mmc_of_parse(host->mmc);
> >>>> +       if (ret)
> >>>> +               goto err_sdhci_add;
> >>>> +
> >>>> +       ret = sdhci_add_host(host);
> >>>> +       if (ret)
> >>>> +               goto err_sdhci_add;
> >>>
> >>> Why can't you use sdhci_pltfm_register()?
> >> two things are missing in sdhci_pltfm_register
> >> 1. clock.
> >
> > Taking into account the implementation of the corresponding
> > _unregister() I would add the clock handling to the _register() one.
> > Perhaps via a new member of the platform data that supplies the name
> > and index of the clock and hence all clk_get_optional() / clk_put will
> > be moved there.
> >
> >> 2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.
> >
> > All the same, why can't platform data be utilised for this?
> >
> >>>> +       return 0;
> >>>> +
> >>>> +err_sdhci_add:
> >>>> +       clk_disable_unprepare(pltfm_host->clk);
> >>>> +       sdhci_pltfm_free(pdev);
> >>>> +       return ret;
> >>>> +}


-- 
With Best Regards,
Andy Shevchenko