linux-kernel - Re: [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 11 Feb 2020 20:08:02 +0530
From:   sbhanu@...eaurora.org
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     adrian.hunter@...el.com, mka@...omium.org, robh+dt@...nel.org,
        ulf.hansson@...aro.org, asutoshd@...eaurora.org,
        stummala@...eaurora.org, sayalil@...eaurora.org,
        cang@...eaurora.org, rampraka@...eaurora.org,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        agross@...nel.org, bjorn.andersson@...aro.org,
        devicetree-owner@...r.kernel.org
Subject: Re: [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks
 of sdhci-msm platform driver.

On 2020-02-11 01:11, Stephen Boyd wrote:
> Quoting Shaik Sajida Bhanu (2020-02-07 05:20:50)
>> The existing suspend/resume callbacks of sdhci-msm driver are just
>> gating/un-gating the clocks. During suspend cycle more can be done
>> like disabling controller, interrupts and card detection.
>> 
>> So updating the system pm callbacks for performing these extra
>> actions besides controlling the clocks.
>> 
>> Signed-off-by: Shaik Sajida Bhanu <sbhanu@...eaurora.org>
>> 
>> Changes since V1:
>>         Addressed review comments
> 
> Please don't write this. Instead, describe what's actually different so
> the reader doesn't have to go figure out what the review comments were.
> 

sure

>> ---
>>  drivers/mmc/host/sdhci-msm.c | 50 
>> ++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 48 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/sdhci-msm.c 
>> b/drivers/mmc/host/sdhci-msm.c
>> index c3a160c..e30c8a3 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -2159,9 +2159,55 @@ static __maybe_unused int 
>> sdhci_msm_runtime_resume(struct device *dev)
>>         return 0;
>>  }
>> 
>> +static int sdhci_msm_suspend(struct device *dev)
>> +{
>> +       struct sdhci_host *host = dev_get_drvdata(dev);
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct sdhci_msm_host *msm_host = 
>> sdhci_pltfm_priv(pltfm_host);
>> +       int ret;
>> +
>> +       if (host->mmc->caps2 & MMC_CAP2_CQE) {
>> +               ret = cqhci_suspend(host->mmc);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       ret = sdhci_suspend_host(host);
>> +       if (ret)
>> +               return ret;
>> +       /* Disable pwr-irq since SDHC would be inactive */
>> +       disable_irq(msm_host->pwr_irq);
> 
> Why do we need to do this? If it's inactive then the irq won't be 
> raised
> by the inactive hardware. Given that we're going to suspend the device,
> the irq won't matter unless it's marked for wakeup. Please remove this
> irq enable/disable logic, or explain why it's really needed.
> 

You are right. This is not needed.
We have checked more on this and interrupt are getting disabled in 
suspend_device_irqs().
Will remove this.

>> +
>> +       return pm_runtime_force_suspend(dev);
>> +}
>> +