[PATCH v6] mmc: Poll CD in case cyclic framework is enabled
In case the cyclic framework is enabled, poll the card detect of already initialized cards and deinitialize them in case they are removed. Since the card initialization is a longer process and card initialization is done on first access to an uninitialized card anyway, avoid initializing newly detected uninitialized cards in the cyclic callback.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Peng Fan peng.fan@nxp.com Cc: Simon Glass sjg@chromium.org --- V2: Move the cyclic registration/unregistration into mmc init/deinit V3: Replace if (CONFIG_IS_ENABLED(CYCLIC)...) with #if as the former does not work with structure members V4: Stuff the code with CONFIG_IS_ENABLED() variants to avoid #ifdefs V5: Rebase on u-boot/next V6: Rebase on u-boot/next --- drivers/mmc/mmc.c | 25 +++++++++++++++++++++++++ include/mmc.h | 3 +++ 2 files changed, 28 insertions(+)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 0b881f11b4a..c787ff6bc49 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -3039,6 +3039,20 @@ static int mmc_complete_init(struct mmc *mmc) return err; }
+static void __maybe_unused mmc_cyclic_cd_poll(struct cyclic_info *c) +{ + struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct mmc, cyclic)), (NULL)); + + if (!m->has_init) + return; + + if (mmc_getcd(m)) + return; + + mmc_deinit(m); + m->has_init = 0; +} + int mmc_init(struct mmc *mmc) { int err = 0; @@ -3061,6 +3075,14 @@ int mmc_init(struct mmc *mmc) if (err) pr_info("%s: %d, time %lu\n", __func__, err, get_timer(start));
+ if (CONFIG_IS_ENABLED(CYCLIC, (!mmc->cyclic.func), (NULL))) { + /* Register cyclic function for card detect polling */ + CONFIG_IS_ENABLED(CYCLIC, (cyclic_register(&mmc->cyclic, + mmc_cyclic_cd_poll, + 100 * 1000, + mmc->cfg->name))); + } + return err; }
@@ -3068,6 +3090,9 @@ int mmc_deinit(struct mmc *mmc) { u32 caps_filtered;
+ if (CONFIG_IS_ENABLED(CYCLIC, (mmc->cyclic.func), (NULL))) + CONFIG_IS_ENABLED(CYCLIC, (cyclic_unregister(&mmc->cyclic))); + if (!CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) && !CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) && !CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)) diff --git a/include/mmc.h b/include/mmc.h index f508cd15700..0044ff8bef7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -14,6 +14,7 @@ #include <linux/sizes.h> #include <linux/compiler.h> #include <linux/dma-direction.h> +#include <cyclic.h> #include <part.h>
struct bd_info; @@ -757,6 +758,8 @@ struct mmc { bool hs400_tuning:1;
enum bus_mode user_speed_mode; /* input speed mode from user */ + + CONFIG_IS_ENABLED(CYCLIC, (struct cyclic_info cyclic)); };
#if CONFIG_IS_ENABLED(DM_MMC)
Marek Vasut marek.vasut+renesas@mailbox.org writes:
In case the cyclic framework is enabled, poll the card detect of already initialized cards and deinitialize them in case they are removed. Since the card initialization is a longer process and card initialization is done on first access to an uninitialized card anyway, avoid initializing newly detected uninitialized cards in the cyclic callback.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Peng Fan peng.fan@nxp.com Cc: Simon Glass sjg@chromium.org
V2: Move the cyclic registration/unregistration into mmc init/deinit V3: Replace if (CONFIG_IS_ENABLED(CYCLIC)...) with #if as the former does not work with structure members V4: Stuff the code with CONFIG_IS_ENABLED() variants to avoid #ifdefs V5: Rebase on u-boot/next V6: Rebase on u-boot/next
drivers/mmc/mmc.c | 25 +++++++++++++++++++++++++ include/mmc.h | 3 +++ 2 files changed, 28 insertions(+)
[rearranging hunks for easier reading]
diff --git a/include/mmc.h b/include/mmc.h index f508cd15700..0044ff8bef7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -14,6 +14,7 @@ #include <linux/sizes.h> #include <linux/compiler.h> #include <linux/dma-direction.h> +#include <cyclic.h> #include <part.h>
struct bd_info; @@ -757,6 +758,8 @@ struct mmc { bool hs400_tuning:1;
enum bus_mode user_speed_mode; /* input speed mode from user */
- CONFIG_IS_ENABLED(CYCLIC, (struct cyclic_info cyclic));
I think that you can simplify this quite a lot by dropping all of the the CONFIG_IS_ENABLED stuff. If CYCLIC is not enabled, struct cyclic_info is an empty struct, so takes up no space, but it still exists in struct mmc, allowing it to be referenced in C code that need not be guarded.
};
#if CONFIG_IS_ENABLED(DM_MMC)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 0b881f11b4a..c787ff6bc49 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -3039,6 +3039,20 @@ static int mmc_complete_init(struct mmc *mmc) return err; }
+static void __maybe_unused mmc_cyclic_cd_poll(struct cyclic_info *c) +{
You can drop __maybe_unused.
- struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct mmc, cyclic)), (NULL));
No need for the CONFIG_IS_ENABLED(), container_of works just fine when the cyclic member exists unconditionally.
- if (!m->has_init)
return;
(I'd assume that in the !CYCLIC case the compiler might warn about this unconditional NULL deref; that you avoid with the above.)
- if (mmc_getcd(m))
return;- mmc_deinit(m);
- m->has_init = 0;
+}
int mmc_init(struct mmc *mmc) { int err = 0; @@ -3061,6 +3075,14 @@ int mmc_init(struct mmc *mmc) if (err) pr_info("%s: %d, time %lu\n", __func__, err, get_timer(start));
- if (CONFIG_IS_ENABLED(CYCLIC, (!mmc->cyclic.func), (NULL))) {
/* Register cyclic function for card detect polling */CONFIG_IS_ENABLED(CYCLIC, (cyclic_register(&mmc->cyclic,mmc_cyclic_cd_poll,100 * 1000,mmc->cfg->name)));- }
No need for any of the CONFIG_IS_ENABLED nesting. Just do cyclic_register() - that's a no-op when !CYCLIC, and the compiler will see the reference to mmc_cyclic_cd_poll, so not warn about it being unused, yet also see that it is not actually called, so elide it from the compiled code.
return err; }
@@ -3068,6 +3090,9 @@ int mmc_deinit(struct mmc *mmc) { u32 caps_filtered;
- if (CONFIG_IS_ENABLED(CYCLIC, (mmc->cyclic.func), (NULL)))
CONFIG_IS_ENABLED(CYCLIC, (cyclic_unregister(&mmc->cyclic)));
Again, just do cyclic_unregister() unconditionally.
Rasmus
On Mon, Sep 09, 2024 at 10:46:21AM +0200, Rasmus Villemoes wrote:
Marek Vasut marek.vasut+renesas@mailbox.org writes:
In case the cyclic framework is enabled, poll the card detect of already initialized cards and deinitialize them in case they are removed. Since the card initialization is a longer process and card initialization is done on first access to an uninitialized card anyway, avoid initializing newly detected uninitialized cards in the cyclic callback.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Peng Fan peng.fan@nxp.com Cc: Simon Glass sjg@chromium.org
V2: Move the cyclic registration/unregistration into mmc init/deinit V3: Replace if (CONFIG_IS_ENABLED(CYCLIC)...) with #if as the former does not work with structure members V4: Stuff the code with CONFIG_IS_ENABLED() variants to avoid #ifdefs V5: Rebase on u-boot/next V6: Rebase on u-boot/next
drivers/mmc/mmc.c | 25 +++++++++++++++++++++++++ include/mmc.h | 3 +++ 2 files changed, 28 insertions(+)
[rearranging hunks for easier reading]
diff --git a/include/mmc.h b/include/mmc.h index f508cd15700..0044ff8bef7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -14,6 +14,7 @@ #include <linux/sizes.h> #include <linux/compiler.h> #include <linux/dma-direction.h> +#include <cyclic.h> #include <part.h>
struct bd_info; @@ -757,6 +758,8 @@ struct mmc { bool hs400_tuning:1;
enum bus_mode user_speed_mode; /* input speed mode from user */
- CONFIG_IS_ENABLED(CYCLIC, (struct cyclic_info cyclic));
I think that you can simplify this quite a lot by dropping all of the the CONFIG_IS_ENABLED stuff. If CYCLIC is not enabled, struct cyclic_info is an empty struct, so takes up no space, but it still exists in struct mmc, allowing it to be referenced in C code that need not be guarded.
};
#if CONFIG_IS_ENABLED(DM_MMC)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 0b881f11b4a..c787ff6bc49 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -3039,6 +3039,20 @@ static int mmc_complete_init(struct mmc *mmc) return err; }
+static void __maybe_unused mmc_cyclic_cd_poll(struct cyclic_info *c) +{
You can drop __maybe_unused.
- struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct mmc, cyclic)), (NULL));
No need for the CONFIG_IS_ENABLED(), container_of works just fine when the cyclic member exists unconditionally.
- if (!m->has_init)
return;(I'd assume that in the !CYCLIC case the compiler might warn about this unconditional NULL deref; that you avoid with the above.)
- if (mmc_getcd(m))
return;- mmc_deinit(m);
- m->has_init = 0;
+}
int mmc_init(struct mmc *mmc) { int err = 0; @@ -3061,6 +3075,14 @@ int mmc_init(struct mmc *mmc) if (err) pr_info("%s: %d, time %lu\n", __func__, err, get_timer(start));
- if (CONFIG_IS_ENABLED(CYCLIC, (!mmc->cyclic.func), (NULL))) {
/* Register cyclic function for card detect polling */CONFIG_IS_ENABLED(CYCLIC, (cyclic_register(&mmc->cyclic,mmc_cyclic_cd_poll,100 * 1000,mmc->cfg->name)));- }
No need for any of the CONFIG_IS_ENABLED nesting. Just do cyclic_register() - that's a no-op when !CYCLIC, and the compiler will see the reference to mmc_cyclic_cd_poll, so not warn about it being unused, yet also see that it is not actually called, so elide it from the compiled code.
return err; }
@@ -3068,6 +3090,9 @@ int mmc_deinit(struct mmc *mmc) { u32 caps_filtered;
- if (CONFIG_IS_ENABLED(CYCLIC, (mmc->cyclic.func), (NULL)))
CONFIG_IS_ENABLED(CYCLIC, (cyclic_unregister(&mmc->cyclic)));Again, just do cyclic_unregister() unconditionally.
The challenge here is that Simon asked for all of this as part of feedback for v3. What are your thoughts here, Simon?
Tom Rini trini@konsulko.com writes:
On Mon, Sep 09, 2024 at 10:46:21AM +0200, Rasmus Villemoes wrote:
Again, just do cyclic_unregister() unconditionally.
The challenge here is that Simon asked for all of this as part of feedback for v3. What are your thoughts here, Simon?
No, AFAICT he asked for not adding new ifdefs to C code. But if the existence of the cyclic member of struct mmc is conditional (whether via an ifdef or via the CONFIG_IS_ENABLED(FOO, (), ()) construction), one is forced to have ifdefs or that very same CONFIG_IS_ENABLED(FOO, (), ()) in C code. Which makes the whole thing rather unreadble IMO.
Which is why I did the series to convert the cyclic_info to something that you embed in your client struct, and which goes away (has size 0) when !CYCLIC, but still syntactically exists, so C code can still just do &mmc->cyclic and everything works. No ifdefs or nested uses of CONFIG_IS_ENABLED() anywhere, and no need to guard the callback function or mark it maybe_unused.
So I tried fetching this patch, build with and without CYCLIC, then do all the simplifications I suggest above, and build again with and without cyclic. No build errors or warning as I expected, but, comparing the object code does reveal something that I need to ask about.
Assuming CONFIG_CYCLIC and unwrapping all the CONFIG_IS_ENABLED stuff, mmc_init() does
if (!mmc->cyclic.func) cyclic_register()
while mmc_deinit() does
if (mmc->cyclic.func) cyclic_unregister()
There are some lifetime issues here that I think are pretty non-obvious. mmc_deinit() can get called from the cyclic callback itself, but nothing ever clears ->cyclic.func, so can't mmc_deinit() also later be called from, say, mmc_blk_remove() ?
I also find it a bit odd that cyclic_register() is done regardless of whether mmc->has_init got set to 0 or 1 (i.e. whether mmc_complete_init() has failed). So can mmc_init() end up returning failure, yet still have registered the cyclic function?
And what if mmc_init() is succesfully called, later mmc_deinit() succesfully called, and then mmc_init() again and finally mmc_deinit() once more. The first will set ->cyclic.func via the register call, the second will unregister because ->cyclic.func is set, the third will _not_ register again because ->cyclic.func is (still) set, and then the fourth will crash because ->cyclic.func is set so cyclic_unregister() is called on something which is not in the list. But maybe that simply can't happen at all because mmc_init() is called at most once? But then why the conditional on ->cyclic.func in the first place?
Rasmus
On Tue, Sep 10, 2024 at 11:34:11AM +0200, Rasmus Villemoes wrote:
Tom Rini trini@konsulko.com writes:
On Mon, Sep 09, 2024 at 10:46:21AM +0200, Rasmus Villemoes wrote:
Again, just do cyclic_unregister() unconditionally.
The challenge here is that Simon asked for all of this as part of feedback for v3. What are your thoughts here, Simon?
No, AFAICT he asked for not adding new ifdefs to C code. But if the existence of the cyclic member of struct mmc is conditional (whether via an ifdef or via the CONFIG_IS_ENABLED(FOO, (), ()) construction), one is forced to have ifdefs or that very same CONFIG_IS_ENABLED(FOO, (), ()) in C code. Which makes the whole thing rather unreadble IMO.
Which is why I did the series to convert the cyclic_info to something that you embed in your client struct, and which goes away (has size 0) when !CYCLIC, but still syntactically exists, so C code can still just do &mmc->cyclic and everything works. No ifdefs or nested uses of CONFIG_IS_ENABLED() anywhere, and no need to guard the callback function or mark it maybe_unused.
So I tried fetching this patch, build with and without CYCLIC, then do all the simplifications I suggest above, and build again with and without cyclic. No build errors or warning as I expected, but, comparing the object code does reveal something that I need to ask about.
Assuming CONFIG_CYCLIC and unwrapping all the CONFIG_IS_ENABLED stuff, mmc_init() does
if (!mmc->cyclic.func) cyclic_register()
while mmc_deinit() does
if (mmc->cyclic.func) cyclic_unregister()
There are some lifetime issues here that I think are pretty non-obvious. mmc_deinit() can get called from the cyclic callback itself, but nothing ever clears ->cyclic.func, so can't mmc_deinit() also later be called from, say, mmc_blk_remove() ?
I also find it a bit odd that cyclic_register() is done regardless of whether mmc->has_init got set to 0 or 1 (i.e. whether mmc_complete_init() has failed). So can mmc_init() end up returning failure, yet still have registered the cyclic function?
And what if mmc_init() is succesfully called, later mmc_deinit() succesfully called, and then mmc_init() again and finally mmc_deinit() once more. The first will set ->cyclic.func via the register call, the second will unregister because ->cyclic.func is set, the third will _not_ register again because ->cyclic.func is (still) set, and then the fourth will crash because ->cyclic.func is set so cyclic_unregister() is called on something which is not in the list. But maybe that simply can't happen at all because mmc_init() is called at most once? But then why the conditional on ->cyclic.func in the first place?
That's all a very good question that I don't think anyone can answer without going through the cases, unfortunately.
Hi Marek,
On Fri, 6 Sept 2024 at 11:11, Marek Vasut marek.vasut+renesas@mailbox.org wrote:
In case the cyclic framework is enabled, poll the card detect of already initialized cards and deinitialize them in case they are removed. Since the card initialization is a longer process and card initialization is done on first access to an uninitialized card anyway, avoid initializing newly detected uninitialized cards in the cyclic callback.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Peng Fan peng.fan@nxp.com Cc: Simon Glass sjg@chromium.org
V2: Move the cyclic registration/unregistration into mmc init/deinit V3: Replace if (CONFIG_IS_ENABLED(CYCLIC)...) with #if as the former does not work with structure members V4: Stuff the code with CONFIG_IS_ENABLED() variants to avoid #ifdefs V5: Rebase on u-boot/next V6: Rebase on u-boot/next
drivers/mmc/mmc.c | 25 +++++++++++++++++++++++++ include/mmc.h | 3 +++ 2 files changed, 28 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Some things below to do now or later.
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 0b881f11b4a..c787ff6bc49 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -3039,6 +3039,20 @@ static int mmc_complete_init(struct mmc *mmc) return err; }
+static void __maybe_unused mmc_cyclic_cd_poll(struct cyclic_info *c)
Add a comment here to explain what is going on - e.g. stuff from your comment message.
+{
struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct mmc, cyclic)), (NULL));if (!m->has_init)return;if (mmc_getcd(m))return;mmc_deinit(m);m->has_init = 0;+}
int mmc_init(struct mmc *mmc) { int err = 0; @@ -3061,6 +3075,14 @@ int mmc_init(struct mmc *mmc) if (err) pr_info("%s: %d, time %lu\n", __func__, err, get_timer(start));
if (CONFIG_IS_ENABLED(CYCLIC, (!mmc->cyclic.func), (NULL))) {/* Register cyclic function for card detect polling */CONFIG_IS_ENABLED(CYCLIC, (cyclic_register(&mmc->cyclic,mmc_cyclic_cd_poll,100 * 1000,mmc->cfg->name)));}
This seems a bit confusing. How about:
if (CONFIG_IS_ENABLED(CYCLIC) && !cyclic_mmc->cyclic.func)
then in cyclic.h something like
static inline bool cyclic_valid(struct cyclic *cyc) { #if CONFIG_IS_ENABLED(CYCLIC) return cyc->func; #else return false; #endif }
We really should not have clients looking at the 'func' to decide if something is active.
You can also turn cyclic_register into a macro (empty if cyclic not enabled) to avoid the problem of ->cyclic not existing.
return err;}
@@ -3068,6 +3090,9 @@ int mmc_deinit(struct mmc *mmc) { u32 caps_filtered;
if (CONFIG_IS_ENABLED(CYCLIC, (mmc->cyclic.func), (NULL)))CONFIG_IS_ENABLED(CYCLIC, (cyclic_unregister(&mmc->cyclic)));
Same comment here.
if (!CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) && !CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) && !CONFIG_IS_ENABLED(MMC_HS400_SUPPORT))diff --git a/include/mmc.h b/include/mmc.h index f508cd15700..0044ff8bef7 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -14,6 +14,7 @@ #include <linux/sizes.h> #include <linux/compiler.h> #include <linux/dma-direction.h> +#include <cyclic.h> #include <part.h>
struct bd_info; @@ -757,6 +758,8 @@ struct mmc { bool hs400_tuning:1;
enum bus_mode user_speed_mode; /* input speed mode from user */
CONFIG_IS_ENABLED(CYCLIC, (struct cyclic_info cyclic));};
#if CONFIG_IS_ENABLED(DM_MMC)
2.45.2
Regards, Simon
On Fri, 06 Sep 2024 19:10:42 +0200, Marek Vasut wrote:
In case the cyclic framework is enabled, poll the card detect of already initialized cards and deinitialize them in case they are removed. Since the card initialization is a longer process and card initialization is done on first access to an uninitialized card anyway, avoid initializing newly detected uninitialized cards in the cyclic callback.
[...]
Applied to u-boot/next, thanks!
On Fri, Sep 06, 2024 at 07:10:42PM +0200, Marek Vasut wrote:
In case the cyclic framework is enabled, poll the card detect of already initialized cards and deinitialize them in case they are removed. Since the card initialization is a longer process and card initialization is done on first access to an uninitialized card anyway, avoid initializing newly detected uninitialized cards in the cyclic callback.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Peng Fan peng.fan@nxp.com Cc: Simon Glass sjg@chromium.org
V2: Move the cyclic registration/unregistration into mmc init/deinit V3: Replace if (CONFIG_IS_ENABLED(CYCLIC)...) with #if as the former does not work with structure members V4: Stuff the code with CONFIG_IS_ENABLED() variants to avoid #ifdefs V5: Rebase on u-boot/next V6: Rebase on u-boot/next
To summarize my thoughts on the thread, I've applied this patch because it moves things forward while fixing usability problems on hardware. But there's certainly room to clean up the framework, which can be done on top of this patch as well rather than further delay fixing the problem here. Thank all.
participants (4)
-
Marek Vasut -
Rasmus Villemoes -
Simon Glass -
Tom Rini