From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Tomer Maimon <tmaimon77@gmail.com> Cc: Adrian Hunter <adrian.hunter@intel.com>, ulf.hansson@linaro.org, avifishman70@gmail.com, tali.perry1@gmail.com, joel@jms.id.au, venture@google.com, yuenn@google.com, benjaminfair@google.com, skhan@linuxfoundation.org, davidgow@google.com, pbrobinson@gmail.com, gsomlo@gmail.com, briannorris@chromium.org, arnd@arndb.de, krakoczy@antmicro.com, openbmc@lists.ozlabs.org, linux-mmc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver Date: Wed, 7 Dec 2022 15:25:27 +0200 [thread overview] Message-ID: <CAHp75Vd5DzkCW0Gpouv+0Or=Yhjp_KdFGP-jXkpHD=UZrG2ajA@mail.gmail.com> (raw) In-Reply-To: <CAP6Zq1h9XvH501e_nH9TkUCKPNOuH7dhOM8FrsUM=PYX4gt0qw@mail.gmail.com> 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? -- With Best Regards, Andy Shevchenko
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Tomer Maimon <tmaimon77@gmail.com> Cc: devicetree@vger.kernel.org, ulf.hansson@linaro.org, benjaminfair@google.com, arnd@arndb.de, krakoczy@antmicro.com, avifishman70@gmail.com, venture@google.com, openbmc@lists.ozlabs.org, briannorris@chromium.org, linux-mmc@vger.kernel.org, Adrian Hunter <adrian.hunter@intel.com>, tali.perry1@gmail.com, gsomlo@gmail.com, joel@jms.id.au, davidgow@google.com, skhan@linuxfoundation.org, linux-kernel@vger.kernel.org, pbrobinson@gmail.com Subject: Re: [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver Date: Wed, 7 Dec 2022 15:25:27 +0200 [thread overview] Message-ID: <CAHp75Vd5DzkCW0Gpouv+0Or=Yhjp_KdFGP-jXkpHD=UZrG2ajA@mail.gmail.com> (raw) In-Reply-To: <CAP6Zq1h9XvH501e_nH9TkUCKPNOuH7dhOM8FrsUM=PYX4gt0qw@mail.gmail.com> 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? -- With Best Regards, Andy Shevchenko
next prev parent reply other threads:[~2022-12-07 13:26 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-05 8:53 [PATCH v2 0/2] MMC: add NPCM SDHCI driver support Tomer Maimon 2022-12-05 8:53 ` Tomer Maimon 2022-12-05 8:53 ` [PATCH v2 1/2] dt-bindings: mmc: npcm,sdhci: Document NPCM SDHCI controller Tomer Maimon 2022-12-05 8:53 ` Tomer Maimon 2022-12-05 22:24 ` Rob Herring 2022-12-05 22:24 ` Rob Herring 2022-12-05 8:53 ` [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver Tomer Maimon 2022-12-05 8:53 ` Tomer Maimon 2022-12-05 10:54 ` Andy Shevchenko 2022-12-05 10:54 ` Andy Shevchenko 2022-12-05 11:20 ` Tomer Maimon 2022-12-05 11:20 ` Tomer Maimon 2022-12-05 13:25 ` Andy Shevchenko 2022-12-05 13:25 ` Andy Shevchenko 2022-12-05 13:41 ` Adrian Hunter 2022-12-05 13:41 ` Adrian Hunter 2022-12-05 14:14 ` Andy Shevchenko 2022-12-05 14:14 ` Andy Shevchenko 2022-12-05 14:17 ` Andy Shevchenko 2022-12-05 14:17 ` Andy Shevchenko 2022-12-05 14:33 ` Adrian Hunter 2022-12-05 14:33 ` Adrian Hunter 2022-12-07 13:01 ` Tomer Maimon 2022-12-07 13:01 ` Tomer Maimon 2022-12-07 13:25 ` Andy Shevchenko [this message] 2022-12-07 13:25 ` Andy Shevchenko 2022-12-07 13:49 ` Adrian Hunter 2022-12-07 13:49 ` Adrian Hunter 2022-12-07 16:48 ` Andy Shevchenko 2022-12-07 16:48 ` Andy Shevchenko 2022-12-08 12:58 ` Tomer Maimon 2022-12-08 12:58 ` Tomer Maimon 2022-12-07 13:47 ` Adrian Hunter 2022-12-07 13:47 ` Adrian Hunter 2023-03-17 14:16 ` Guenter Roeck 2023-03-17 14:16 ` Guenter Roeck 2023-03-17 17:36 ` Andy Shevchenko 2023-03-17 17:36 ` Andy Shevchenko 2023-03-17 18:37 ` Guenter Roeck 2023-03-17 18:37 ` Guenter Roeck 2023-03-23 12:19 ` Ulf Hansson 2023-03-23 12:19 ` Ulf Hansson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAHp75Vd5DzkCW0Gpouv+0Or=Yhjp_KdFGP-jXkpHD=UZrG2ajA@mail.gmail.com' \ --to=andy.shevchenko@gmail.com \ --cc=adrian.hunter@intel.com \ --cc=arnd@arndb.de \ --cc=avifishman70@gmail.com \ --cc=benjaminfair@google.com \ --cc=briannorris@chromium.org \ --cc=davidgow@google.com \ --cc=devicetree@vger.kernel.org \ --cc=gsomlo@gmail.com \ --cc=joel@jms.id.au \ --cc=krakoczy@antmicro.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mmc@vger.kernel.org \ --cc=openbmc@lists.ozlabs.org \ --cc=pbrobinson@gmail.com \ --cc=skhan@linuxfoundation.org \ --cc=tali.perry1@gmail.com \ --cc=tmaimon77@gmail.com \ --cc=ulf.hansson@linaro.org \ --cc=venture@google.com \ --cc=yuenn@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.