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] [thread-next>] [day] [month] [year] [list]
Message-ID: <158136367603.121156.1867941302835915258@swboyd.mtv.corp.google.com>
Date:   Mon, 10 Feb 2020 11:41:16 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Shaik Sajida Bhanu <sbhanu@...eaurora.org>,
        adrian.hunter@...el.com, mka@...omium.org, robh+dt@...nel.org,
        ulf.hansson@...aro.org
Cc:     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,
        Shaik Sajida Bhanu <sbhanu@...eaurora.org>
Subject: Re: [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver.

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.

> ---
>  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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ