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

[David Daney <ddaney@caviumnetworks.com>]

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