drivers: mmc: msm: remove clock disable in probe
Subsystems (2)
PATCH 1/1 drivers: mmc: msm: remove clock disable in probe
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.
--- drivers/mmc/host/msm_sdcc.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-)
drivers/mmc/host/msm_sdcc.c +0 -3
@@ -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);
Signature · 5 lines
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 1/1 drivers: mmc: msm: remove clock disable in probe
Hi,
Quoted 7 lines · 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 <[email protected]>
I can add acked-by and/or tested-by if needed.
~Vitaly
PATCH 1/1 drivers: mmc: msm: remove clock disable in probe
Quoted 9 lines · Vitaly Wool
Hi,
Quoted 6 lines · 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.
Quoted 5 lines
Quoted 1 line
Signed-off-by Daniel Walker<[email protected]>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
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH 1/1 drivers: mmc: msm: remove clock disable in probe
Quoted 12 lines · Saravana Kannan
Quoted 11 lines · Vitaly Wool
Hi,
Quoted 7 lines · 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<[email protected]>
Quoted 8 lines
Quoted 3 lines
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
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH 1/1 drivers: mmc: msm: remove clock disable in probe
Quoted 17 lines · Daniel Walker
Quoted 16 lines · Saravana Kannan
Quoted 14 lines · Vitaly Wool
Hi,
Quoted 7 lines · 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<[email protected]>I can add acked-by and/or tested-by if needed.
~Vitaly
Quoted 9 lines
Quoted 4 lines
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
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH 1/1 drivers: mmc: msm: remove clock disable in probe
Quoted 3 lines · Saravana Kannan
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.
Quoted 1 line
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.
Signature · 6 lines
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 1/1 drivers: mmc: msm: remove clock disable in probe
Quoted 39 lines · Sahitya Tummala
Quoted 3 lines · Saravana Kannan
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 functionmsmsdcc_request()) and at the end of that command disabling of clocks will be deferred by callingmsmsdcc_disable_clocks()(check the functionmsmsdcc_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 tommc_add_host(). Thismmc_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 callingmmc_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.
Quoted 7 lines
Quoted 1 line
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 beforeadd_hostand 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
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.