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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
