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

Ulf Hansson
2016-04-21T12:44:10Z

[...]

>> 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.

>
>> >>> I am not sure how completions would be of use, perhaps you could
>> >>> elaborate.
>> >>
>> >>
>> >> Completions are used when you have one thread waiting for an event,
>> >> which is often an interrupt: the process calls
>> >> wait_for_completion(&completion); and the interrupt handler calls
>> >> complete(&completion);
>> >>
>> >> It seems that you are using the semaphore for two reasons here (I
>> >> only read it briefly so I may be wrong):
>> >> waiting for the interrupt handler and serializing against another
>> >> thread. In this case you need both a mutex (to guarantee mutual
>> >> exclusion) and a completion (to wait for the interrupt handler
>> >> to finish).
>> >>
>> >
>> > The way the MMC driver works is that the driver's .request() method is
>> > called to initiate a request. After .request() is finished, it returns
>> > back to the kernel so other work can be done.
>>
>> Correct.
>>
>> 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.

[...]

>>
>>
>> In the ->request() callback:
>> ....
>> mutex_lock()
>> spin_lock_irqsave()
>>
>> host->mrq = mrq;
>>
>> spin_unlock_irqrestore()
>> ...
>> ---------------------
>>
>> In the hard IRQ handler:
>> ...
>> spin_lock()
>>
>> if (!host->mrq)
>> return IRQ_HANDLED;
>> else
>> return IRQ_WAKE_THREAD;
>>
>> spin_unlock()
>> ...
>> ---------------------
>>
>> In the threaded IRQ handler:
>> ...
>> spin_lock_irqsave()
>>
>> mrq = host->mrq;
>>
>> spin_unlock_irqrestore()
>> ...
>> process request...
>> ...
>> when request completed:
>> ...
>> spin_lock_irqsave()
>>
>> host->mrq = NULL;
>>
>> spin_unlock_irqrestore()
>> mutex_unlock()
>> ...
>> mmc_request_done()
>> ---------------------
>>
>> 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?

Kind regards
Uffe