On Tue, Sep 10, 2024 at 11:34:11AM +0200, Rasmus Villemoes wrote: > Tom Rini <[email protected]> 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
signature.asc
Description: PGP signature
