> -----Original Message----- > From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx] > Sent: Wednesday, March 03, 2010 4:16 AM > To: Madhusudhan > Cc: 'LKML'; linux-omap@xxxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx > Subject: RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking > > On Tue, 2 Mar 2010, Madhusudhan wrote: > > > Conditional locking on (!in_interrupt()) is broken by design and there > > > is no reason to keep the host->irq_lock across the call to > > > mmc_request_done(). Also the host->protect_card magic hack does not > > > depend on the context > > > > > > > Can you please elaborate why the existing logic is broken? > > Locks are only to be held to serialize data or state. > > The mmc_request_done() call does _NOT_ require that at all. So > dropping the lock there is the right thing to do. > > Also conditional locking on in_interrupt() is generally a nono as it > relies on assumptions which are not necessarily true in all > circumstances. Just one simple example: interrupt threading will make > it explode nicely and it did already with the realtime patches > applied. > > Such code constructs prevent us to do generic changes to the kernel > behaviour without any real good reason. > > > It locks at the new request and unlocks just before issuing the cmd. > Further > > IRQ handler has these calls hence the !in_interrupt check. > > Aside of the conditional locking I have several issues with that code: > > 1) The code flow is massively unreadable: > > omap_hsmmc_start_command() > { > ..... > if (!in_interrupt()) > spin_unlock_irq(); > } > > omap_hsmmc_request() > { > if (!in_interrupt()) > spin_lock_irq(); > > omap_hsmmc_start_command(); > } > > We generally want to see the lock/unlock pairs in one function and not > having to figure out where the heck unlock happens. > > 2) The point of unlocking is patently wrong > > omap_hsmmc_start_command() > { > ..... > if (!in_interrupt()) > spin_unlock_irq(); > ---> > OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg); > ---> > OMAP_HSMMC_WRITE(host->base, CMD, cmdreg); > } > > What happens, if you get a spurious interrupt here ? Same for SMP, > though you are probably protected by the core mmc code request > serialization there. > > > How does this patch improve that? In fact with your patch for a data > > transfer cmd there are several lock-unlock calls. > > 1) The patch simply removes conditional locking and moves the lock > sections to those places which protect something. Aside of that it > makes the code easier to understand. > > 2) What's the point of not having those lock/unlocks ? On UP the > spinlock is a NOOP anyway, so you won't even notice. On SMP you > won't notice either, simply because the lock is cache hot and > almost never contended. > Sounds reasonable.Readbility is still a factor but works for me as the main intention here is to remove the in_interrupt conditional. Acked-by: Madhusudhan Chikkature<madhu.cr@xxxxxx> Best Regards, Madhu > Thanks, > > tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
