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

[Tom Rini <trini@konsulko.com>]

[-- 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 --]