Hi Wolfram,
On 18.02.2024 22:10, Wolfram Sang wrote:
Hi Dirk,
hope you are doing well!
Thanks for doing this analysis and summary! Much appreciated.
Many thanks for answering so fast! :)
Comparing this, the questions are:
=20
* tmio uses irqsave spin locks, while the bcm2835 uses mutex for locking.
Why is irqsave spin lock used here?
The spinlock was introduced with 6ff56e0d8e02 ("mmc: tmio_mmc: handle
missing HW interrupts"). This was before my time with Renesas, but the
rule of thumb for chosing spinlocks over mutexes: they are lightweight.
If the critical sections are short and no sleeping is needed within
them, they can save context switching.
Yes, I agree, the question regarding spin lock vs. mutex was non-sense
;) For synchronizing with the interrupt handler we need to use spin
lock, as (potentially sleeping) mutex in interrupt handler are not allowed.
So I'm totally fine for using spin locks for synchronization :)
Remains the question if we are locking "enough".
Of course, this is not needed for
a timeout handler, but in other code paths this is more interesting.
* In tmio the mmc_request_done() is *outside* the lock, while for bcm2835
its *inside* the lock protected section. Why does tmio doesn't lock the
access to mmc_request_done()?
The spinlock is to protect the private data structure, 'host' in our
case. mmc_request_done() doesn't operate on it, only with MMC core
structures 'mmc_host' and 'mmc_request'. AFAICS that doesn't need
protection. The mtk-sd driver has it also outside the critical section.
But: clearing host->mrq looks like it should be *in* the critical section.
There is another case in tmio_mmc_set_ios() which also looks suspicious
at least. I'll double check all this tomorrow.
* How does tmio_mmc_reset_work() ensures that the content mrq points to a=
nd
passed to mmc_request_done() is still the correct content and hasn't chan=
ged
asynchronously in the background (due to being outside the lock)?
Because it copies 'host->mrq' to a local variable 'mrq' within a
critical section:
236 spin_lock_irqsave(&host->lock, flags);
237 mrq =3D host->mrq;
and then passes this to mmc_request_done(). It even clears host->mrq
before that to enable asynchronous processing of the next mrq:
270 host->mrq =3D NULL;
271 mmc_request_done(host->mmc, mrq);
Of course I'm not an expert on this driver ;)
So I'm not sure where this mrq comes from. In the end we just copy the
pointer to the mrq to our local variable. What I don't know if anybody
else holds a reference to it and might change/discard this mrq in
parallel/in the background? Or create a new mrq making the one we are
currently processing invalid? Just some thoughts, though.
* Why does tmio doesn't use cancel_delayed_work()? Maybe because its ensu=
red
by the interrupt locking no further work is scheduled?
If you want to cancel it, I think you should rather use
cancel_delayed_work_sync() to really know that there is nothing pending
anymore. But that function sleeps. TMIO approach is to simply bail out
on later calls:
239 /*
240 * is request already finished? Since we use a non-blocking
241 * cancel_delayed_work(), it can happen, that a .set_ios() cal=
l preempts
242 * us, so, have to check for IS_ERR(host->mrq)
243 */
244 if (IS_ERR_OR_NULL(mrq) ||
245 time_is_after_jiffies(host->last_req_ts +
246 msecs_to_jiffies(CMDREQ_TIMEOUT))) {
247 spin_unlock_irqrestore(&host->lock, flags);
248 return;
249 }
Other drivers do this as well, like alcor:
318 =20
319 /*=20
320 * If this work gets rescheduled while running, it will=20
321 * be run again afterwards but without any active request.=20
322 */=20
323 if (!host->mrq)=20
324 return;=20
325 =20
I'd think this part is OK but I will double check as well.^
I will do some testing with moving the locks, too.
In sum looking at the KASAN report and the locking in tmio_mmc_reset_work=
()
we have an uncomfortable feeling. bcm2835_timeout() looks much safer
regarding the locking. Of course that is just a feeling and can't be prov=
en.
Yes, with a mutex and putting everything inside it, it uses a big hammer
and no questions asked. I am not complaining here, that is a sensible
approach at times. We have, however, historically been introduced with
spinlocks and a more finegrained approach. I prefer to (and will do) double
check the critical sections first before considering more intrusive
approaches which would need a lot of testing, of course.
Yes, any change on the locking would need a lot of testing :(
Best regards
Dirk
