RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Thomas Gleixner]

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.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/