| Message ID | 20221205085351.27566-1-tmaimon77@gmail.com |
|---|---|
| Headers | show |
| Series | MMC: add NPCM SDHCI driver support | expand |
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 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 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 > > >
On Mon, 05 Dec 2022 10:53:50 +0200, Tomer Maimon wrote: > Add binding for Nuvoton NPCM SDHCI controller. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > .../devicetree/bindings/mmc/npcm,sdhci.yaml | 45 +++++++++++++++++++ > 1 file changed, 45 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
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 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