Hi Doug, On 16 October 2013 17:43, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > On Wed, Oct 16, 2013 at 2:49 AM, James Hogan <james.hogan@xxxxxxxxxx> wrote: >>> We can't just use the standard host->lock since that lock is not irq >>> safe and mmc_signal_sdio_irq() (called from interrupt context) calls >>> dw_mci_enable_sdio_irq(). Add a new irq-safe lock to protect INTMASK. >>> >>> An alternate solution to this is to punt mmc_signal_sdio_irq() to the >>> tasklet and then protect INTMASK modifications by the standard host >>> lock. This seemed like a bit more of a high-latency change. >> >> A probably lighter-weight alternative to that alternative is to just >> make the existing lock irq safe. Has this been considered? >> >> I'm not entirely convinced it's worth having a separate lock rather than >> changing the existing one, but the patch still appears to be correct, so: >> Reviewed-by: James Hogan <james.hogan@xxxxxxxxxx> > > I did look at that alternate solution and rejected it, but am happy to > send that up if people think it's better. Here's why I rejected it: > > 1. It looks like someone has gone through quite a bit of work to _not_ > grab the existing lock in the IRQ handler. The IRQ handler always > pushes all real work over to the tasklet. I can only assume that they > did this for some good reason and I'd hate to switch it without > knowing for sure why. > > 2. We might have performance problems if we blindly changed the > existing code to an IRQ-safe spinlock. We hold the existing > "host->lock" spinlock for the entire duration of > dw_mci_tasklet_func(). That's a pretty intense chunk of code, full of > nested loops (some with 500ms timeouts!), mdelay(20) calls to handle > some errors, etc. I say "might" because I think that the expected > case is that code runs pretty quickly. I ran some brief tests on one > system and saw a worst case time of 170us and an common case time of > ~10us. Are we OK with having interrupts disabled for that long? Are > we OK with having the dw_mci interrupt handler potentially spin on a > spinlock for that long? > Fair enough, I'm convinced now. That code did look pretty heavy when I looked at it too. > > I don't think it would be terrible to look at reworking the way that > dw_mmc handles interrupts, but that seems pretty major. Yeh, I suppose at least the potentially large delays are all for exceptional cases, so it's not a critical problem. > BTW: is the cost of an extra lock actually that high? I don't know tbh. In this case the spinlock usage doesn't appear to actually overlap. > ...or are you talking the cost in terms of code complexity? In this case it'd only be a space and code complexity thing I think. I suppose in some cases the benefit of finer-grained locking is probably pretty marginal, but there's a good case for it here. It might be worth renaming the lock to irq_lock or something like that so it's clear it doesn't have to protect only for INTMASK in the future - up to you. Cheers James -- 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
