[PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Marek Vasut]

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

Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Rasmus Villemoes]

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

Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4397 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Rasmus Villemoes]

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

Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Simon Glass]

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

Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2993 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Tom Rini]

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!

-- 
Tom

Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]