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