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

[Andy Shevchenko]

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:

...

> > > +#include <linux/clk.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/mmc/host.h>
> > > +#include <linux/mmc/mmc.h>
> > > +#include <linux/module.h>
> >
> > I guess platform_device.h is missing here.
> Build and work without platform_device.h, do I need it for module use?

The rule of thumb is to include headers we are the direct user of. I
believe you have a data type and API that are defined in that header.

...

> > > +static int npcm_sdhci_probe(struct platform_device *pdev)
> > > +{
> > > +       struct sdhci_pltfm_host *pltfm_host;
> > > +       struct sdhci_host *host;
> > > +       u32 caps;
> > > +       int ret;
> > > +
> > > +       host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0);
> > > +       if (IS_ERR(host))
> > > +               return PTR_ERR(host);
> > > +
> > > +       pltfm_host = sdhci_priv(host);
> >
> > > +       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().

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().

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