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

[David Daney <ddaney@caviumnetworks.com>]

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.

>
>> +#if 0
>> +#define octeon_mmc_dbg trace_printk
>> +#else
>> +static inline void octeon_mmc_dbg(const char *s, ...) { }
>> +#endif
>
> Remove this and use dev_dbg() or pr_debug(), it does the same thing.

It is not the same thing.  pr_debug has *way* more overhead than 
trace_printk has it also acquires locks that can cause system lockups to 
happen.  The driver doesn't work with pr_debug().

We could just remove this *and* all calls to octeon_mmc_dbg, but 
switching to pr_debug() is not an option.

>
>> +static irqreturn_t octeon_mmc_interrupt(int irq, void *dev_id)
>
> This function is rather long, can you split it up a bit for
> readability?
>
>> +{
>> +	struct octeon_mmc_host *host = dev_id;
>> +	union cvmx_mio_emm_int emm_int;
>> +	struct mmc_request	*req;
>> +	bool host_done;
>> +	union cvmx_mio_emm_rsp_sts rsp_sts;
>> +	unsigned long flags = 0;
>> +
>> +	if (host->need_irq_handler_lock)
>> +		spin_lock_irqsave(&host->irq_handler_lock, flags);
>> +	else
>> +		__acquire(&host->irq_handler_lock);
>
> The locking seems odd, why do you only sometimes have to take the lock,

In the cn78xx_style case there are multiple irqs with this handler.  in 
the !cn78xx_style case there is a single irq.

The multiple irq case is what we are protecting.  Without the spinlock, 
there are races between the handler threads of the several irqs that can 
fire.

> and why do you disable interrupts from within the irq handler?
>

That may be gratuitous, although in the threaded interrupt handler case 
it may be needed.  I guess that has to be investigated.

David Daney