Re: [RESEND PATCH v7 2/2] mmc: OCTEON: Add host driver for OCTEON MMC controller

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 21 April 2016 14:44:08 Ulf Hansson wrote:
> [...]
> 
> >> So the question I have is *why* do you have to be in IRQ context when
> >> using the semaphore...
> >>
> >> I would rather see that you use a threaded IRQ handler, perhaps in
> >> conjunction with a hard IRQ handler if that is needed.
> >
> > That does not solve the problem though: it is not allowed for a mutex
> > to be taken in the request function but released in the interrupt,
> > both have to be in the same thread.
> >
> > Using a threaded IRQ handler would help by avoiding the spinlock
> > inside of it (it could be replaced with a mutex there), but it
> > doesn't solve the problem of serializing between the slots.
> 
> My point is, the may not need to be released in the IRQ handler, but
> instead in the threaded IRQ handler.

That doesn't change anything.

> >> Although to clarify a bit more, the mmc core invokes *all* mmc host
> >> ops callbacks from non-atomic context.
> >
> > Oh, so you mean the .request() function must not sleep and cannot
> > call mutex_lock() or down() or wait_event()?
> 
> No.
> 
> *non-atomic* context. :-)
> 
> I should probably said thread/process context instead.
> 

My mistake, you were being clear enough here, I just misread.

> >> In the ->request() callback:
> >> ....
> >> mutex_lock()

> >> In the threaded IRQ handler:
> >> ...
> >> mutex_unlock()
> >>
> >> Do you think something along these lines should work for your case?
> >
> > This is the case I described above, it is against the rules for mutexes()
> > and you will get a lockdep warning if you attempt this.
> 
> No.
> 
> Considering my comments in this reply, can you please re-think and see
> if my solution could make sense?

The problem is not that mutex_unlock() has to be called from non-atomic
context (it doesn't, you are allowed  to call mutex_unlock while holding
a spinlock), but instead the problem is that it has to be done from the
same thread that called mutex_lock(), and the threaded IRQ handler
is never the same thread that is currently holding the mutex.

My suggestion with using

	wait_event(host->wq, !cmpxchg(host->current_req, NULL, mrq));

should sufficiently solve the problem, but the suggestion of using
a kthread (even though not needed for taking a mutex) would still
have some advantages and one disadvantage:

+ We never need to spin in the irq context (also achievable using
  a threaded handler)
+ The request callback always returns immediately after queuing up
  the request to the kthread, rather than blocking for a potentially
  long time while waiting for an operation in another slot to complete
+ it very easily avoids the problem of serialization between
  the slots, and ensures that each slot gets an equal chance to
  send the next request.
- you get a slightly higher latency for waking up the kthread in
  oder to do a simple request (comparable amount of latency that
  is introduced by an irq thread).

	Arnd