Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij <linus.walleij@stericsson.com>]

Ohad Ben-Cohen wrote:

> Hi Linus,
> On Wed, Nov 3, 2010 at 11:22 AM, Linus Walleij
> <linus.walleij@stericsson.com> wrote:
> ...
>> +static void mmc_host_clk_gate_delayed(struct mmc_host *host)
>> +{
>> +       unsigned long tick_ns;
>> +       unsigned long freq = host->ios.clock;
>> +       unsigned long flags;
>> +       int users;
>> +
>> +       if (!freq) {
>> +               pr_err("%s: frequency set to 0 in disable function, "
>> +                      "this means the clock is already disabled.\n",
>> +                      mmc_hostname(host));
>> +               return;
>> +       }
>> +       /*
>> +        * New requests may have appeared while we were scheduling,
>> +        * then there is no reason to delay the check before
>> +        * clk_disable().
>> +        */
>> +       spin_lock_irqsave(&host->clk_lock, flags);
>> +       users = host->clk_requests;
>> +       /*
>> +        * Delay n bus cycles (at least 8 from MMC spec) before attempting
>> +        * to disable the MCI block clock. The reference count
>> +        * may have gone up again after this delay due to
>> +        * rescheduling!
>> +        */
>> +       if (!users) {

Note to self: remove the local users variable and look directly
at host->clk_requests.

>> +               spin_unlock_irqrestore(&host->clk_lock, flags);
>> +               tick_ns = DIV_ROUND_UP(1000000000, freq);
>> +               ndelay(host->clk_delay * tick_ns);
>> +       } else {
>> +               /* New users appeared while waiting for this work */
>> +               host->clk_pending_gate = false;
>> +               spin_unlock_irqrestore(&host->clk_lock, flags);
>> +               return;
>> +       }
>> +       spin_lock_irqsave(&host->clk_lock, flags);
>> +       if (!host->clk_requests) {
>> +               spin_unlock_irqrestore(&host->clk_lock, flags);
> 
> What if mmc_host_clk_ungate() is invoked (and completely executes) at
> this point (as a result of a new mmc request) ?
> 
>> +               /* this will set host->ios.clock to 0 */
>> +               mmc_gate_clock(host);
> 
> Will this clock gating not disrupt that new mmc request ?

Not that one, because the only place where ungate is called
is immediately before the request or set_ios(). So the request
or set_ios() will complete, and immediately after that
this gating will be triggered.

So the real bug is that if we get this race we don't get
the 8 MCI cycles of delay that we want.

But I guess I can replace all spinlocks with a mutex instead
and still hold it across the gate operation, since all
gate/ungate calls should be coming from process context?

That simplifies things.

I'll see if this works...

>> +               spin_lock_irqsave(&host->clk_lock, flags);
>> +               pr_debug("%s: gated MCI clock\n",
>> +                        mmc_hostname(host));
>> +       }
>> +       host->clk_pending_gate = false;
> 
> What is clk_pending_gate used for (I can only see it being assigned values) ?

Hm, a development artifact from patchset v4 2009-06-18...
It's replaced with host->clk_gated instead.

I'll remove it.

> (PS sorry for the belated posting of these questions)

No problem, I'll fix.

Chris, do you want an incremental patch or shall I spin an all-new
v10 patch?

Yours,
Linus Walleij