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

  Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/mmc/host/Kconfig      |  8 ++++
 drivers/mmc/host/Makefile     |  1 +
 drivers/mmc/host/sdhci-npcm.c | 84 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-npcm.c
  

On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

Thank you for an update, my comments below.

...

> +config MMC_SDHCI_NPCM

>  config MMC_SDHCI_IPROC

Perhaps after IPROC?

...

> @@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)       += sdhci-pic32.o
>  obj-$(CONFIG_MMC_SDHCI_BRCMSTB)                += sdhci-brcmstb.o
>  obj-$(CONFIG_MMC_SDHCI_OMAP)           += sdhci-omap.o
>  obj-$(CONFIG_MMC_SDHCI_SPRD)           += sdhci-sprd.o
> +obj-$(CONFIG_MMC_SDHCI_NPCM)           += sdhci-npcm.o

Perhaps after IPROC? (There is a group of platform drivers slightly
below than here)

>  obj-$(CONFIG_MMC_CQHCI)                        += cqhci.o

...

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

...

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

> +       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()?

> +       return 0;
> +
> +err_sdhci_add:
> +       clk_disable_unprepare(pltfm_host->clk);
> +       sdhci_pltfm_free(pdev);
> +       return ret;
> +}

Missing ->remove() due to above.

Have you tried to compile as a module and then remove and insert it
several times?
  

Hi Andy,

Thanks for your comments.

On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
>
> Thank you for an update, my comments below.
>
> ...
>
> > +config MMC_SDHCI_NPCM
>
> >  config MMC_SDHCI_IPROC
>
> Perhaps after IPROC?
Will be done in the next version.
>
> ...
>
> > @@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)       += sdhci-pic32.o
> >  obj-$(CONFIG_MMC_SDHCI_BRCMSTB)                += sdhci-brcmstb.o
> >  obj-$(CONFIG_MMC_SDHCI_OMAP)           += sdhci-omap.o
> >  obj-$(CONFIG_MMC_SDHCI_SPRD)           += sdhci-sprd.o
> > +obj-$(CONFIG_MMC_SDHCI_NPCM)           += sdhci-npcm.o
>
> Perhaps after IPROC? (There is a group of platform drivers slightly
> below than here)
Will be done in the next version.
>
> >  obj-$(CONFIG_MMC_CQHCI)                        += cqhci.o
>
> ...
>
> > +#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?
>
> ...
>
> > +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?
>
> > +       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.
2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.
>
> > +       return 0;
> > +
> > +err_sdhci_add:
> > +       clk_disable_unprepare(pltfm_host->clk);
> > +       sdhci_pltfm_free(pdev);
> > +       return ret;
> > +}
>
> Missing ->remove() due to above.
Will check
>
> Have you tried to compile as a module and then remove and insert it
> several times?
will try
>
> --
> With Best Regards,
> Andy Shevchenko

Best regards,

Tomer
  

On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> 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;
> > > +}
  

On 5/12/22 15:25, Andy Shevchenko wrote:
> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>> On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> 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().

devm_ calls exactly those, so what is the issue?

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

On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 5/12/22 15:25, Andy Shevchenko wrote:
> > On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >> On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>> On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> 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;
> >>>> +}
  

On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > On 5/12/22 15:25, Andy Shevchenko wrote:
> > > On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> > >> On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >>> On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> 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.

To elaborate more, the

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

is the correct order.

Hence in this case the driver can be worked around easily (by
shuffling the order in ->probe() to call devm_ first), but as I said
looking into implementation of the _unregister() I'm pretty sure that
clock management should be in sdhci-pltfm, rather than in all callers
who won't need the full customization.

Hope this helps to understand my point.

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

On 5/12/22 16:17, Andy Shevchenko wrote:
> On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>
>> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 5/12/22 15:25, Andy Shevchenko wrote:
>>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>>>>> On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>>>> On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> 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.
> 
> To elaborate more, the
> 
> 1. call all devm_func()
> 2. call only non-devm_func()
> 
> is the correct order.

1. WRT pltfm_host->clk, that is what is happening
2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_
e.g. mmc_alloc_host() / mmc_free_host()

> 
> Hence in this case the driver can be worked around easily (by
> shuffling the order in ->probe() to call devm_ first), but as I said
> looking into implementation of the _unregister() I'm pretty sure that
> clock management should be in sdhci-pltfm, rather than in all callers
> who won't need the full customization.
> 
> Hope this helps to understand my point.
> 
>>>> 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
> 
> 
>
  

Hi Andy and Adrian,

Thanks for your clarifications

On Mon, 5 Dec 2022 at 16:33, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 5/12/22 16:17, Andy Shevchenko wrote:
> > On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >>
> >> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>> On 5/12/22 15:25, Andy Shevchenko wrote:
> >>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >>>>> On Mon, 5 Dec 2022 at 12:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>>>>> On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <tmaimon77@gmail.com> 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.
> >
> > To elaborate more, the
> >
> > 1. call all devm_func()
> > 2. call only non-devm_func()
> >
> > is the correct order.
>
> 1. WRT pltfm_host->clk, that is what is happening
> 2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_
> e.g. mmc_alloc_host() / mmc_free_host()
I little confused about what to decide, should I use only
non-devm_func because mmc_alloc_host() / mmc_free_host() is not
warrped with devm_?
>
> >
> > Hence in this case the driver can be worked around easily (by
> > shuffling the order in ->probe() to call devm_ first), but as I said
> > looking into implementation of the _unregister() I'm pretty sure that
> > clock management should be in sdhci-pltfm, rather than in all callers
> > who won't need the full customization.
> >
> > Hope this helps to understand my point.
> >
> >>>> 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.
Do you mean to add it to sdhci_pltfm_register function? if yes I
believe it will take some time to modify sdhci_pltfm_register
I prefer not to use sdhci_pltfm_register.
> >>>>
> >>>>> 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
> >
> >
> >
>

Best regards,

Tomer
  

On Wed, Dec 7, 2022 at 3:01 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> On Mon, 5 Dec 2022 at 16:33, Adrian Hunter <adrian.hunter@intel.com> wrote:
> > On 5/12/22 16:17, Andy Shevchenko wrote:
> > > On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > >> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>> On 5/12/22 15:25, Andy Shevchenko wrote:
> > >>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:

...

> > >>>> 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.
> > >
> > > To elaborate more, the
> > >
> > > 1. call all devm_func()
> > > 2. call only non-devm_func()
> > >
> > > is the correct order.
> >
> > 1. WRT pltfm_host->clk, that is what is happening
> > 2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_
> > e.g. mmc_alloc_host() / mmc_free_host()
> I little confused about what to decide, should I use only
> non-devm_func because mmc_alloc_host() / mmc_free_host() is not
> warrped with devm_?

It is up to you how to proceed. I pointed out the problem with your
code which may or may not be fatal.

If you want to solve it, there are several approaches:
1) get rid of devm_ completely;
2) properly shuffle the ordering in ->probe(), so all devm_ calls are
followed by non-devm_;
3) wrap non-devm_ cals to become managed (see
devm_add_action_or_reset() approach);
4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix
sdhci_pltfm_register() to handle the clock.

Personally, the list order is from the least, what I prefer, to the
most (i.o.w. I would like to see rather 4) than 1) to be implemented).

> > > Hence in this case the driver can be worked around easily (by
> > > shuffling the order in ->probe() to call devm_ first), but as I said
> > > looking into implementation of the _unregister() I'm pretty sure that
> > > clock management should be in sdhci-pltfm, rather than in all callers
> > > who won't need the full customization.
> > >
> > > Hope this helps to understand my point.
> > >
> > >>>> 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.

...

> > >>>>>> 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.
> Do you mean to add it to sdhci_pltfm_register function? if yes I
> believe it will take some time to modify sdhci_pltfm_register
> I prefer not to use sdhci_pltfm_register.

In the Linux kernel we are trying hard to avoid code duplication. Why
do you need it to be open coded? (Yes, I heard you, but somebody
should fix the issues with that funcion at some point, right?)

> > >>>>> 2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.
> > >>>>
> > >>>> All the same, why can't platform data be utilised for this?
  

On 5/12/22 10:53, Tomer Maimon wrote:
> Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/Kconfig      |  8 ++++
>  drivers/mmc/host/Makefile     |  1 +
>  drivers/mmc/host/sdhci-npcm.c | 84 +++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-npcm.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index fb1062a6394c..82ab6fc25dca 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -415,6 +415,14 @@ config MMC_SDHCI_MILBEAUT
>  
>  	  If unsure, say N.
>  
> +config MMC_SDHCI_NPCM
> +	tristate "Secure Digital Host Controller Interface support for NPCM"
> +	depends on ARCH_NPCM || COMPILE_TEST
> +	depends on MMC_SDHCI_PLTFM
> +	help
> +	  This provides support for the SD/eMMC controller found in
> +	  NPCM BMC family SoCs.
> +
>  config MMC_SDHCI_IPROC
>  	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
>  	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 4e4ceb32c4b4..a101f87a5f19 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)	+= sdhci-pic32.o
>  obj-$(CONFIG_MMC_SDHCI_BRCMSTB)		+= sdhci-brcmstb.o
>  obj-$(CONFIG_MMC_SDHCI_OMAP)		+= sdhci-omap.o
>  obj-$(CONFIG_MMC_SDHCI_SPRD)		+= sdhci-sprd.o
> +obj-$(CONFIG_MMC_SDHCI_NPCM)		+= sdhci-npcm.o
>  obj-$(CONFIG_MMC_CQHCI)			+= cqhci.o
>  cqhci-y					+= cqhci-core.o
>  cqhci-$(CONFIG_MMC_CRYPTO)		+= cqhci-crypto.o
> diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c
> new file mode 100644
> index 000000000000..beace15b6c00
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-npcm.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NPCM SDHC MMC host controller driver.
> + *
> + * Copyright (c) 2020 Nuvoton Technology corporation.
> + */
> +
> +#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>
> +
> +#include "sdhci-pltfm.h"
> +
> +static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
> +	.quirks  = SDHCI_QUIRK_DELAY_AFTER_POWER,
> +	.quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
> +		   SDHCI_QUIRK2_NO_1_8_V,
> +};
> +
> +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);
> +	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;
> +
> +	return 0;
> +
> +err_sdhci_add:
> +	clk_disable_unprepare(pltfm_host->clk);
> +	sdhci_pltfm_free(pdev);
> +	return ret;
> +}
> +
> +static const struct of_device_id npcm_sdhci_of_match[] = {
> +	{ .compatible = "nuvoton,npcm750-sdhci" },
> +	{ .compatible = "nuvoton,npcm845-sdhci" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match);
> +
> +static struct platform_driver npcm_sdhci_driver = {
> +	.driver = {
> +		.name	= "npcm-sdhci",
> +		.of_match_table = npcm_sdhci_of_match,
> +		.pm	= &sdhci_pltfm_pmops,
> +	},
> +	.probe		= npcm_sdhci_probe,
> +	.remove		= sdhci_pltfm_unregister,
> +};
> +module_platform_driver(npcm_sdhci_driver);
> +
> +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver");
> +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
> +MODULE_LICENSE("GPL");
  

On 7/12/22 15:25, Andy Shevchenko wrote:
> On Wed, Dec 7, 2022 at 3:01 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>> On Mon, 5 Dec 2022 at 16:33, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 5/12/22 16:17, Andy Shevchenko wrote:
>>>> On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko
>>>> <andy.shevchenko@gmail.com> wrote:
>>>>> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> On 5/12/22 15:25, Andy Shevchenko wrote:
>>>>>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> 
> ...
> 
>>>>>>> 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.
>>>>
>>>> To elaborate more, the
>>>>
>>>> 1. call all devm_func()
>>>> 2. call only non-devm_func()
>>>>
>>>> is the correct order.
>>>
>>> 1. WRT pltfm_host->clk, that is what is happening
>>> 2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_
>>> e.g. mmc_alloc_host() / mmc_free_host()
>> I little confused about what to decide, should I use only
>> non-devm_func because mmc_alloc_host() / mmc_free_host() is not
>> warrped with devm_?
> 
> It is up to you how to proceed. I pointed out the problem with your
> code which may or may not be fatal.
> 
> If you want to solve it, there are several approaches:
> 1) get rid of devm_ completely;
> 2) properly shuffle the ordering in ->probe(), so all devm_ calls are
> followed by non-devm_;
> 3) wrap non-devm_ cals to become managed (see
> devm_add_action_or_reset() approach);
> 4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix
> sdhci_pltfm_register() to handle the clock.

I can take care of sdhci_pltfm when I next have some time.
Otherwise it looks OK to me, so I am acking it.

> 
> Personally, the list order is from the least, what I prefer, to the
> most (i.o.w. I would like to see rather 4) than 1) to be implemented).
> 
>>>> Hence in this case the driver can be worked around easily (by
>>>> shuffling the order in ->probe() to call devm_ first), but as I said
>>>> looking into implementation of the _unregister() I'm pretty sure that
>>>> clock management should be in sdhci-pltfm, rather than in all callers
>>>> who won't need the full customization.
>>>>
>>>> Hope this helps to understand my point.
>>>>
>>>>>>> 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.
> 
> ...
> 
>>>>>>>>> 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.
>> Do you mean to add it to sdhci_pltfm_register function? if yes I
>> believe it will take some time to modify sdhci_pltfm_register
>> I prefer not to use sdhci_pltfm_register.
> 
> In the Linux kernel we are trying hard to avoid code duplication. Why
> do you need it to be open coded? (Yes, I heard you, but somebody
> should fix the issues with that funcion at some point, right?)
> 
>>>>>>>> 2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.
>>>>>>>
>>>>>>> All the same, why can't platform data be utilised for this?
>
  

On Wed, Dec 7, 2022 at 3:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 7/12/22 15:25, Andy Shevchenko wrote:
> > On Wed, Dec 7, 2022 at 3:01 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >> On Mon, 5 Dec 2022 at 16:33, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>> On 5/12/22 16:17, Andy Shevchenko wrote:
> >>>> On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko
> >>>> <andy.shevchenko@gmail.com> wrote:
> >>>>> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>>> On 5/12/22 15:25, Andy Shevchenko wrote:
> >>>>>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:

...

> >>>>>>> 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.
> >>>>
> >>>> To elaborate more, the
> >>>>
> >>>> 1. call all devm_func()
> >>>> 2. call only non-devm_func()
> >>>>
> >>>> is the correct order.
> >>>
> >>> 1. WRT pltfm_host->clk, that is what is happening
> >>> 2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_
> >>> e.g. mmc_alloc_host() / mmc_free_host()
> >> I little confused about what to decide, should I use only
> >> non-devm_func because mmc_alloc_host() / mmc_free_host() is not
> >> warrped with devm_?
> >
> > It is up to you how to proceed. I pointed out the problem with your
> > code which may or may not be fatal.
> >
> > If you want to solve it, there are several approaches:
> > 1) get rid of devm_ completely;
> > 2) properly shuffle the ordering in ->probe(), so all devm_ calls are
> > followed by non-devm_;
> > 3) wrap non-devm_ cals to become managed (see
> > devm_add_action_or_reset() approach);
> > 4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix
> > sdhci_pltfm_register() to handle the clock.
>
> I can take care of sdhci_pltfm when I next have some time.
> Otherwise it looks OK to me, so I am acking it.

Thank you!

> > Personally, the list order is from the least, what I prefer, to the
> > most (i.o.w. I would like to see rather 4) than 1) to be implemented).
> >
> >>>> Hence in this case the driver can be worked around easily (by
> >>>> shuffling the order in ->probe() to call devm_ first), but as I said
> >>>> looking into implementation of the _unregister() I'm pretty sure that
> >>>> clock management should be in sdhci-pltfm, rather than in all callers
> >>>> who won't need the full customization.
> >>>>
> >>>> Hope this helps to understand my point.
> >>>>
> >>>>>>> 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.

...

> >>>>>>>>> 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.
> >> Do you mean to add it to sdhci_pltfm_register function? if yes I
> >> believe it will take some time to modify sdhci_pltfm_register
> >> I prefer not to use sdhci_pltfm_register.
> >
> > In the Linux kernel we are trying hard to avoid code duplication. Why
> > do you need it to be open coded? (Yes, I heard you, but somebody
> > should fix the issues with that funcion at some point, right?)
> >
> >>>>>>>> 2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.
> >>>>>>>
> >>>>>>> All the same, why can't platform data be utilised for this?
  

Thanks a lot, Adrian and andy!

Appreciate it

On Wed, 7 Dec 2022 at 18:49, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Wed, Dec 7, 2022 at 3:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > On 7/12/22 15:25, Andy Shevchenko wrote:
> > > On Wed, Dec 7, 2022 at 3:01 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> > >> On Mon, 5 Dec 2022 at 16:33, Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>> On 5/12/22 16:17, Andy Shevchenko wrote:
> > >>>> On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko
> > >>>> <andy.shevchenko@gmail.com> wrote:
> > >>>>> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>>>>> On 5/12/22 15:25, Andy Shevchenko wrote:
> > >>>>>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> ...
>
> > >>>>>>> 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.
> > >>>>
> > >>>> To elaborate more, the
> > >>>>
> > >>>> 1. call all devm_func()
> > >>>> 2. call only non-devm_func()
> > >>>>
> > >>>> is the correct order.
> > >>>
> > >>> 1. WRT pltfm_host->clk, that is what is happening
> > >>> 2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_
> > >>> e.g. mmc_alloc_host() / mmc_free_host()
> > >> I little confused about what to decide, should I use only
> > >> non-devm_func because mmc_alloc_host() / mmc_free_host() is not
> > >> warrped with devm_?
> > >
> > > It is up to you how to proceed. I pointed out the problem with your
> > > code which may or may not be fatal.
> > >
> > > If you want to solve it, there are several approaches:
> > > 1) get rid of devm_ completely;
> > > 2) properly shuffle the ordering in ->probe(), so all devm_ calls are
> > > followed by non-devm_;
> > > 3) wrap non-devm_ cals to become managed (see
> > > devm_add_action_or_reset() approach);
> > > 4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix
> > > sdhci_pltfm_register() to handle the clock.
> >
> > I can take care of sdhci_pltfm when I next have some time.
> > Otherwise it looks OK to me, so I am acking it.
>
> Thank you!
>
> > > Personally, the list order is from the least, what I prefer, to the
> > > most (i.o.w. I would like to see rather 4) than 1) to be implemented).
> > >
> > >>>> Hence in this case the driver can be worked around easily (by
> > >>>> shuffling the order in ->probe() to call devm_ first), but as I said
> > >>>> looking into implementation of the _unregister() I'm pretty sure that
> > >>>> clock management should be in sdhci-pltfm, rather than in all callers
> > >>>> who won't need the full customization.
> > >>>>
> > >>>> Hope this helps to understand my point.
> > >>>>
> > >>>>>>> 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.
>
> ...
>
> > >>>>>>>>> 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.
> > >> Do you mean to add it to sdhci_pltfm_register function? if yes I
> > >> believe it will take some time to modify sdhci_pltfm_register
> > >> I prefer not to use sdhci_pltfm_register.
> > >
> > > In the Linux kernel we are trying hard to avoid code duplication. Why
> > > do you need it to be open coded? (Yes, I heard you, but somebody
> > > should fix the issues with that funcion at some point, right?)
> > >
> > >>>>>>>> 2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.
> > >>>>>>>
> > >>>>>>> All the same, why can't platform data be utilised for this?
>
> --
> With Best Regards,
> Andy Shevchenko
  

On Mon, Dec 05, 2022 at 10:53:51AM +0200, Tomer Maimon wrote:
> Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>

I still don't see this driver in the upstream kernel, or in linux-next.

Couple of comments:

- devm ordering does not really matter here. The devm resource
  is the clock, it does not depend on local data, and it will be
  released last, so that is ok.
- sdhci_pltfm_unregister() calls clk_disable_unprepare(),
  so there is no enabled clock floating around on driver removal.
  Unfortunately, that also means that the more convenient
  devm_clk_get_optional_enabled() can not be used.

Real problem inline below.

Guenter

> ---
>  drivers/mmc/host/Kconfig      |  8 ++++
>  drivers/mmc/host/Makefile     |  1 +
>  drivers/mmc/host/sdhci-npcm.c | 84 +++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-npcm.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index fb1062a6394c..82ab6fc25dca 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -415,6 +415,14 @@ config MMC_SDHCI_MILBEAUT
>  
>  	  If unsure, say N.
>  
> +config MMC_SDHCI_NPCM
> +	tristate "Secure Digital Host Controller Interface support for NPCM"
> +	depends on ARCH_NPCM || COMPILE_TEST
> +	depends on MMC_SDHCI_PLTFM
> +	help
> +	  This provides support for the SD/eMMC controller found in
> +	  NPCM BMC family SoCs.
> +
>  config MMC_SDHCI_IPROC
>  	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
>  	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 4e4ceb32c4b4..a101f87a5f19 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)	+= sdhci-pic32.o
>  obj-$(CONFIG_MMC_SDHCI_BRCMSTB)		+= sdhci-brcmstb.o
>  obj-$(CONFIG_MMC_SDHCI_OMAP)		+= sdhci-omap.o
>  obj-$(CONFIG_MMC_SDHCI_SPRD)		+= sdhci-sprd.o
> +obj-$(CONFIG_MMC_SDHCI_NPCM)		+= sdhci-npcm.o
>  obj-$(CONFIG_MMC_CQHCI)			+= cqhci.o
>  cqhci-y					+= cqhci-core.o
>  cqhci-$(CONFIG_MMC_CRYPTO)		+= cqhci-crypto.o
> diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c
> new file mode 100644
> index 000000000000..beace15b6c00
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-npcm.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NPCM SDHC MMC host controller driver.
> + *
> + * Copyright (c) 2020 Nuvoton Technology corporation.
> + */
> +
> +#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>
> +
> +#include "sdhci-pltfm.h"
> +
> +static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
> +	.quirks  = SDHCI_QUIRK_DELAY_AFTER_POWER,
> +	.quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
> +		   SDHCI_QUIRK2_NO_1_8_V,
> +};
> +
> +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);
> +	if (IS_ERR(pltfm_host->clk))
> +		return PTR_ERR(pltfm_host->clk);
> +
> +	ret = clk_prepare_enable(pltfm_host->clk);
> +	if (ret)
> +		return ret;
> +

The two functions above should not return but goto the call
to sdhci_pltfm_free() to avoid a memory leak on error.

> +	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;
> +
> +	return 0;
> +
> +err_sdhci_add:
> +	clk_disable_unprepare(pltfm_host->clk);
> +	sdhci_pltfm_free(pdev);
> +	return ret;
> +}
> +
> +static const struct of_device_id npcm_sdhci_of_match[] = {
> +	{ .compatible = "nuvoton,npcm750-sdhci" },
> +	{ .compatible = "nuvoton,npcm845-sdhci" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match);
> +
> +static struct platform_driver npcm_sdhci_driver = {
> +	.driver = {
> +		.name	= "npcm-sdhci",
> +		.of_match_table = npcm_sdhci_of_match,
> +		.pm	= &sdhci_pltfm_pmops,
> +	},
> +	.probe		= npcm_sdhci_probe,
> +	.remove		= sdhci_pltfm_unregister,
> +};
> +module_platform_driver(npcm_sdhci_driver);
> +
> +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver");
> +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.33.0
>
  

On Fri, Mar 17, 2023 at 4:16 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Dec 05, 2022 at 10:53:51AM +0200, Tomer Maimon wrote:
> > Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>
> I still don't see this driver in the upstream kernel, or in linux-next.
>
> Couple of comments:
>
> - devm ordering does not really matter here. The devm resource
>   is the clock, it does not depend on local data, and it will be
>   released last, so that is ok.

Not sure. Strictly speaking this is the problem. If you leave a clock
going on in a wrong period of time it (theoretically) might break your
hardware once and forever. Similar discussion about power, clock and
reset signals has been held for camera sensors.
  

On 3/17/23 10:36, Andy Shevchenko wrote:
> On Fri, Mar 17, 2023 at 4:16 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Mon, Dec 05, 2022 at 10:53:51AM +0200, Tomer Maimon wrote:
>>> Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
>>>
>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>>
>> I still don't see this driver in the upstream kernel, or in linux-next.
>>
>> Couple of comments:
>>
>> - devm ordering does not really matter here. The devm resource
>>    is the clock, it does not depend on local data, and it will be
>>    released last, so that is ok.
> 
> Not sure. Strictly speaking this is the problem. If you leave a clock
> going on in a wrong period of time it (theoretically) might break your
> hardware once and forever. Similar discussion about power, clock and
> reset signals has been held for camera sensors.
> 

In general I agree, but not here. The remove function (sdhci_pltfm_unregister)
does call clk_disable_unprepare(), so the clock isn't left running.

Also, I think it is worthwhile to point out that exactly the same sequence
(sdhci_pltfm_init followed by devm_clk_get and cleanup/removal with
sdhci_pltfm_unregister) is shared among several sdhci drivers (including
the memory leak I pointed out, but only in the aspeed driver).

On a higher level I do agree that the sdhci platform code is in need of cleanup,
but I don't think it is appropriate to tie such a cleanup to this driver
submission.

Note that I don't really care much, I just realized that this patch is stuck
when I tried to test booting from SD drive with qemu.

Guenter
  

On Fri, 17 Mar 2023 at 15:16, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Dec 05, 2022 at 10:53:51AM +0200, Tomer Maimon wrote:
> > Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>
> I still don't see this driver in the upstream kernel, or in linux-next.

Tomer, Guenter,

Looks like I may have missed picking it up, probably because I thought
the review was still ongoing.

Please re-submit and include the reviewed-by tags, etc.

Kind regards
Uffe

>
> Couple of comments:
>
> - devm ordering does not really matter here. The devm resource
>   is the clock, it does not depend on local data, and it will be
>   released last, so that is ok.
> - sdhci_pltfm_unregister() calls clk_disable_unprepare(),
>   so there is no enabled clock floating around on driver removal.
>   Unfortunately, that also means that the more convenient
>   devm_clk_get_optional_enabled() can not be used.
>
> Real problem inline below.
>
> Guenter
>
> > ---
> >  drivers/mmc/host/Kconfig      |  8 ++++
> >  drivers/mmc/host/Makefile     |  1 +
> >  drivers/mmc/host/sdhci-npcm.c | 84 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 93 insertions(+)
> >  create mode 100644 drivers/mmc/host/sdhci-npcm.c
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index fb1062a6394c..82ab6fc25dca 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -415,6 +415,14 @@ config MMC_SDHCI_MILBEAUT
> >
> >         If unsure, say N.
> >
> > +config MMC_SDHCI_NPCM
> > +     tristate "Secure Digital Host Controller Interface support for NPCM"
> > +     depends on ARCH_NPCM || COMPILE_TEST
> > +     depends on MMC_SDHCI_PLTFM
> > +     help
> > +       This provides support for the SD/eMMC controller found in
> > +       NPCM BMC family SoCs.
> > +
> >  config MMC_SDHCI_IPROC
> >       tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
> >       depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST
> > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> > index 4e4ceb32c4b4..a101f87a5f19 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)     += sdhci-pic32.o
> >  obj-$(CONFIG_MMC_SDHCI_BRCMSTB)              += sdhci-brcmstb.o
> >  obj-$(CONFIG_MMC_SDHCI_OMAP)         += sdhci-omap.o
> >  obj-$(CONFIG_MMC_SDHCI_SPRD)         += sdhci-sprd.o
> > +obj-$(CONFIG_MMC_SDHCI_NPCM)         += sdhci-npcm.o
> >  obj-$(CONFIG_MMC_CQHCI)                      += cqhci.o
> >  cqhci-y                                      += cqhci-core.o
> >  cqhci-$(CONFIG_MMC_CRYPTO)           += cqhci-crypto.o
> > diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c
> > new file mode 100644
> > index 000000000000..beace15b6c00
> > --- /dev/null
> > +++ b/drivers/mmc/host/sdhci-npcm.c
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * NPCM SDHC MMC host controller driver.
> > + *
> > + * Copyright (c) 2020 Nuvoton Technology corporation.
> > + */
> > +
> > +#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>
> > +
> > +#include "sdhci-pltfm.h"
> > +
> > +static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
> > +     .quirks  = SDHCI_QUIRK_DELAY_AFTER_POWER,
> > +     .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
> > +                SDHCI_QUIRK2_NO_1_8_V,
> > +};
> > +
> > +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);
> > +     if (IS_ERR(pltfm_host->clk))
> > +             return PTR_ERR(pltfm_host->clk);
> > +
> > +     ret = clk_prepare_enable(pltfm_host->clk);
> > +     if (ret)
> > +             return ret;
> > +
>
> The two functions above should not return but goto the call
> to sdhci_pltfm_free() to avoid a memory leak on error.
>
> > +     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;
> > +
> > +     return 0;
> > +
> > +err_sdhci_add:
> > +     clk_disable_unprepare(pltfm_host->clk);
> > +     sdhci_pltfm_free(pdev);
> > +     return ret;
> > +}
> > +
> > +static const struct of_device_id npcm_sdhci_of_match[] = {
> > +     { .compatible = "nuvoton,npcm750-sdhci" },
> > +     { .compatible = "nuvoton,npcm845-sdhci" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match);
> > +
> > +static struct platform_driver npcm_sdhci_driver = {
> > +     .driver = {
> > +             .name   = "npcm-sdhci",
> > +             .of_match_table = npcm_sdhci_of_match,
> > +             .pm     = &sdhci_pltfm_pmops,
> > +     },
> > +     .probe          = npcm_sdhci_probe,
> > +     .remove         = sdhci_pltfm_unregister,
> > +};
> > +module_platform_driver(npcm_sdhci_driver);
> > +
> > +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver");
> > +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.33.0
> >