[PATCH] drivers: mmc: msm: remove clock disable in probe

[PATCH] drivers: mmc: msm: remove clock disable in probe

[Daniel Walker]

The probe function adds the MMC host which can start accepting request
immediately. There is an assumption here that no requests happen
immediatly, but it's not always the case. This assumption can causes
a BUG() when the clocks are disabled. The fix is to just remove the
clock disable in the probe function.

Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/mmc/host/msm_sdcc.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index a46b9a8..36dbb71 100644
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c
@@ -1370,9 +1370,6 @@ msmsdcc_probe(struct platform_device *pdev)
 	if (host->timer.function)
 		pr_info("%s: Polling status mode enabled\n", mmc_hostname(mmc));
 
-#if BUSCLK_PWRSAVE
-	msmsdcc_disable_clocks(host, 1);
-#endif
 	return 0;
  cmd_irq_free:
 	free_irq(cmd_irqres->start, host);
-- 
1.7.0.4

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] drivers: mmc: msm: remove clock disable in probe

[Vitaly Wool]

Hi,

On Tue, Jan 18, 2011 at 7:14 PM, Daniel Walker <dwalker@codeaurora.org> wrote:
> The probe function adds the MMC host which can start accepting request
> immediately. There is an assumption here that no requests happen
> immediatly, but it's not always the case. This assumption can causes
> a BUG() when the clocks are disabled. The fix is to just remove the
> clock disable in the probe function.
>
> Signed-off-by: Daniel Walker <dwalker@codeaurora.org>

I can add acked-by and/or tested-by if needed.

~Vitaly

[PATCH] drivers: mmc: msm: remove clock disable in probe

[Saravana Kannan]

On 01/19/2011 02:50 AM, Vitaly Wool wrote:
> Hi,
>
> On Tue, Jan 18, 2011 at 7:14 PM, Daniel Walker<dwalker@codeaurora.org>  wrote:
>> The probe function adds the MMC host which can start accepting request
>> immediately. There is an assumption here that no requests happen
>> immediatly, but it's not always the case. This assumption can causes
>> a BUG() when the clocks are disabled. The fix is to just remove the
>> clock disable in the probe function.
>>
>> Signed-off-by: Daniel Walker<dwalker@codeaurora.org>
>
> I can add acked-by and/or tested-by if needed.
>
> ~Vitaly

Nack from me. The fix is incorrect. The clocks are alread refcounted in 
the clock driver. There should be no need to "leave it on because 
someone else might access it". Every code that needs the clock should 
have a clk_enable/disable around it and it would all work fine.

-Saravana


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] drivers: mmc: msm: remove clock disable in probe

[Daniel Walker]

On Thu, 2011-01-20 at 19:24 -0800, Saravana Kannan wrote:
> On 01/19/2011 02:50 AM, Vitaly Wool wrote:
> > Hi,
> >
> > On Tue, Jan 18, 2011 at 7:14 PM, Daniel Walker<dwalker@codeaurora.org>  wrote:
> >> The probe function adds the MMC host which can start accepting request
> >> immediately. There is an assumption here that no requests happen
> >> immediatly, but it's not always the case. This assumption can causes
> >> a BUG() when the clocks are disabled. The fix is to just remove the
> >> clock disable in the probe function.
> >>
> >> Signed-off-by: Daniel Walker<dwalker@codeaurora.org>
> >
> > I can add acked-by and/or tested-by if needed.
> >
> > ~Vitaly
> 
> Nack from me. The fix is incorrect. The clocks are alread refcounted in 
> the clock driver. There should be no need to "leave it on because 
> someone else might access it". Every code that needs the clock should 
> have a clk_enable/disable around it and it would all work fine.

You appear to be wrong Saravana .. The clocks are off at that point,
which means there's no need to disable them twice. If you look at the
MMC code mmc_add_host() will disable the clocks.

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] drivers: mmc: msm: remove clock disable in probe

[Saravana Kannan]

On 01/21/2011 08:58 AM, Daniel Walker wrote:
> On Thu, 2011-01-20 at 19:24 -0800, Saravana Kannan wrote:
>> On 01/19/2011 02:50 AM, Vitaly Wool wrote:
>>> Hi,
>>>
>>> On Tue, Jan 18, 2011 at 7:14 PM, Daniel Walker<dwalker@codeaurora.org>   wrote:
>>>> The probe function adds the MMC host which can start accepting request
>>>> immediately. There is an assumption here that no requests happen
>>>> immediatly, but it's not always the case. This assumption can causes
>>>> a BUG() when the clocks are disabled. The fix is to just remove the
>>>> clock disable in the probe function.
>>>>
>>>> Signed-off-by: Daniel Walker<dwalker@codeaurora.org>
>>>
>>> I can add acked-by and/or tested-by if needed.
>>>
>>> ~Vitaly
>>
>> Nack from me. The fix is incorrect. The clocks are alread refcounted in
>> the clock driver. There should be no need to "leave it on because
>> someone else might access it". Every code that needs the clock should
>> have a clk_enable/disable around it and it would all work fine.
>
> You appear to be wrong Saravana .. The clocks are off at that point,
> which means there's no need to disable them twice. If you look at the
> MMC code mmc_add_host() will disable the clocks.
>

Does the clock disable in the probe fail all the time or only sometimes?

mmc_add_host() does a lot of things, so it's hard to figure out how it 
eventually ends up disabling the clocks as you claim. The only execution 
path from add_host() that I can see disabling the clocks are the calls 
to host->ios(). But that function has an enable and disable inside it. 
So, that's already balanced.

To me this still looks like a clock enable/disable mismatch. My best 
*guess* would be that the deferred disable code in 
msmsdcc_disable_clocks() might be missing a corner case and causing a 
disable to happen more that it should.

I will look further into this if I have time, but at this point I'm 
still skeptical whether this is the right fix. The reason I went from 
"sure" to "skeptical" is due to the deferred clock delay code in the driver.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] drivers: mmc: msm: remove clock disable in probe

[Sahitya Tummala]

On Fri, 2011-01-21 at 11:16 -0800, Saravana Kannan wrote:

> I will look further into this if I have time, but at this point I'm 
> still skeptical whether this is the right fix. The reason I went from 
> "sure" to "skeptical" is due to the deferred clock delay code in the driver.

Saravana,

Let me try to make things clear regarding clocks management in this
driver - 

1. Before sending any MMC command msmsdcc_enable_clocks() will be called
(check the function msmsdcc_request()) and at the end of that command
disabling of clocks will be deferred by calling msmsdcc_disable_clocks()
(check the function msmsdcc_request_end()).

2. The driver ensures that clocks are enabled/disabled only once for any
number of calls to msmsdcc_enable_clocks/msmsdcc_disable_clocks. Thus,
there is no harm in calling msmsdcc_disable_clocks/msmsdcc_enable_clocks
twice.

Now coming to the actual problem -

During msmsdcc_probe(), clocks are enabled and at the end of probe
clocks are disabled in the current code. But just before disabling
clocks, there is a call to mmc_add_host(). This mmc_add_host() will
schedule the detect work to run in another thread context. The detect
work will send MMC commands for card detection. Thus, if clocks are
disabled immediately after calling mmc_add_host(), then we might end up
turning off clocks while the detect work commands are in progress.

Fix to this problem ?

1. The clocks will anyways be turned off at the end of MMC commands that
are sent by detect work.  Thus,  there is no need to disable clocks at
the end of probe. This is exactly the fix that Daniel submitted.

2. Otherwise, we can also move disabling of clocks just before
mmc_add_host() so that clocks are enabled and disabled in probe itself.
>From there on, clocks will be enabled for any MMC command and disabled
at the end of that request.

I would prefer Daniel's fix because the clocks are anyway going to be
turned on soon in mmc_add_host() and thus there will be no real
advantage of disabling before add_host and enabling immediately for
detect work MMC commands. Please let us know your comments.

-- 
Thanks,
Sahitya.
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] drivers: mmc: msm: remove clock disable in probe

[Saravana Kannan]

On 01/24/2011 04:44 AM, Sahitya Tummala wrote:
> On Fri, 2011-01-21 at 11:16 -0800, Saravana Kannan wrote:
>
>> I will look further into this if I have time, but at this point I'm
>> still skeptical whether this is the right fix. The reason I went from
>> "sure" to "skeptical" is due to the deferred clock delay code in the driver.
>
> Saravana,
>
> Let me try to make things clear regarding clocks management in this
> driver -
>
> 1. Before sending any MMC command msmsdcc_enable_clocks() will be called
> (check the function msmsdcc_request()) and at the end of that command
> disabling of clocks will be deferred by calling msmsdcc_disable_clocks()
> (check the function msmsdcc_request_end()).
>
> 2. The driver ensures that clocks are enabled/disabled only once for any
> number of calls to msmsdcc_enable_clocks/msmsdcc_disable_clocks. Thus,
> there is no harm in calling msmsdcc_disable_clocks/msmsdcc_enable_clocks
> twice.
>
> Now coming to the actual problem -
>
> During msmsdcc_probe(), clocks are enabled and at the end of probe
> clocks are disabled in the current code. But just before disabling
> clocks, there is a call to mmc_add_host(). This mmc_add_host() will
> schedule the detect work to run in another thread context. The detect
> work will send MMC commands for card detection. Thus, if clocks are
> disabled immediately after calling mmc_add_host(), then we might end up
> turning off clocks while the detect work commands are in progress.
>
> Fix to this problem ?
>
> 1. The clocks will anyways be turned off at the end of MMC commands that
> are sent by detect work.  Thus,  there is no need to disable clocks at
> the end of probe. This is exactly the fix that Daniel submitted.
>
> 2. Otherwise, we can also move disabling of clocks just before
> mmc_add_host() so that clocks are enabled and disabled in probe itself.
>> From there on, clocks will be enabled for any MMC command and disabled
> at the end of that request.
>
> I would prefer Daniel's fix because the clocks are anyway going to be
> turned on soon in mmc_add_host() and thus there will be no real
> advantage of disabling before add_host and enabling immediately for
> detect work MMC commands. Please let us know your comments.

Sahitya,

Thanks a lot for the detailed explanation. It certainly makes it easier 
to understand what's going on.

Going to the fix, the 2nd way would work without unnecessarily turning 
on/off clocks if you didn't ensure that clk_enable/clk_disable is not 
called more than once. They already maintain ref count, so there is no 
need for doing it again in your driver.

Also, having the driver do something like "if I already disabled the 
clock, then ignore this new disable request I got", _could_ mask a lot 
of incorrect behavior.

Having said that, the way the msm sdcc driver is designed today, I'm 
willing to consider Daniel's patch as acceptable first step fix. You 
might still want to look into seeing if this could be modified to take 
advantage of the ref counting done in the clock code.

Thanks,
Saravana


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.