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

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

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