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

David Daney
2016-04-19T23:27:10Z

On 04/19/2016 03:09 PM, Arnd Bergmann wrote:
> On Tuesday 19 April 2016 14:45:35 David Daney wrote:
>> On 04/19/2016 01:46 PM, Arnd Bergmann wrote:
>>> On Thursday 31 March 2016 16:26:53 Matt Redfearn wrote:
>>>> +struct octeon_mmc_host {
>>>> + u64 base;
>>>> + u64 ndf_base;
>>>> + u64 emm_cfg;
>>>> + u64 n_minus_one; /* OCTEON II workaround location */
>>>> + int last_slot;
>>>> +
>>>> + struct semaphore mmc_serializer;
>>>
>>> Please don't add any new semaphores to the kernel, use a mutex or
>>> a completion instead.
>>
>> The last time I checked, a mutex could not be used from interrupt context.
>>
>> Since we are in interrupt context and we really want mutex-like behavior
>> here, it seems like a semaphore is just the thing we need.
>>
>> 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.

From the interrupt handler, when the request is complete, the interrupt
handler calls req->done(req); to terminate the whole thing.

So we have:

CPU-A CPU-B CPU-C

octeon_mmc_request(0) . .
down() . .
queue_request(0); . .
return; . .
other_useful_work . .
. . .
. . .
. . .
octeon_mmc_request(1) . .
down() -> blocks . .
octeon_mmc_interrupt() .
up() -> unblocks .
down() <-unblocks req->done(0) .
queue_request(1); return; .
return; . .
other_useful_work . .
. . octeon_mmc_interrupt()
. . up()
. . req->done(1)
. . return;
. . .

We don't want to have the thread on CPU-A wait around in an extra mutex
or completion for the command to finish. The MMC core already has its
own request waiting code, but it doesn't handle the concept of a slot.
These commands can take hundreds or thousands of mS to terminate. The
whole idea of the MMC framework is to queue the request and get back to
doing other work ASAP.

In the case of this octeon_mmc driver we need to serialize the commands
issued to multiple slots, for this we use the semaphore. If you don't
like struct semaphore, we would have to invent a proprietary wait queue
mechanism that has semantics nearly identical to struct semaphore, and
people would complain that we are reinventing the semaphore.

It doesn't seem clean to cobble up multiple waiting structures
(completion + mutex + logic that surely would contain errors) where a
single (well debugged) struct semaphore does what we want.

David Daney