Re: [PATCH] mmc: dw_mmc-exynos: fix potential external abort in resume_noirq()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ulf,

On 2018-06-11 11:35, Ulf Hansson wrote:
> On 11 June 2018 at 08:48, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
>> dw_mci_exynos_resume_noirq() performs DWMMC register access without
>> ensuring that respective clocks are enabled. This might cause external
>> abort on some systems (observed on Exynos5433 based boards). Fix this
>> by adding needed prepare_enable/disable_unprepare calls.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> ---
>>   drivers/mmc/host/dw_mmc-exynos.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
>> index 3164681108ae..6125b68726b0 100644
>> --- a/drivers/mmc/host/dw_mmc-exynos.c
>> +++ b/drivers/mmc/host/dw_mmc-exynos.c
>> @@ -193,6 +193,9 @@ static int dw_mci_exynos_resume_noirq(struct device *dev)
>>          struct dw_mci_exynos_priv_data *priv = host->priv;
>>          u32 clksel;
>>
>> +       clk_prepare_enable(host->biu_clk);
>> +       clk_prepare_enable(host->ciu_clk);
>> +
>>          if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
>>                  priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU)
>>                  clksel = mci_readl(host, CLKSEL64);
>> @@ -207,6 +210,9 @@ static int dw_mci_exynos_resume_noirq(struct device *dev)
>>                          mci_writel(host, CLKSEL, clksel);
>>          }
>>
>> +       clk_disable_unprepare(host->biu_clk);
>> +       clk_disable_unprepare(host->ciu_clk);
>> +
>>          return 0;
>>   }
>>   #else
> I looked a little closer and I am wondering if it wouldn't be possible
> to use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() instead of
> SET_SYSTEM_SLEEP_PM_OPS()?
>
> Somelike this:
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> dw_mci_exynos_resume_noirq)
>
> Then from dw_mci_exynos_resume_noirq() call pm_runtime_force_resume().
>
> I think it would simplify the code a bit, as you can rely on the
> runtime PM callbacks to deal with clk_prepare_enable() and
> clk_disable_unprepare(), unless I am mistaken.

This will not fix the problem, because mci_writel() calls in
dw_mci_exynos_resume_noirq are done unconditionally, regardless of the
controller's runtime pm state. Since commit 1d9174fbc55e after calling
pm_runtime_force_resume() there is no guarantee that device is in
runtime active state if it was runtime suspended state.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux