linux-kernel - Re: [PATCH V2] mmc: sdhci-msm: Reset GCC_SDCC

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: <daecd30e4798e7d1e4b74dfe37d67b74a6f3cde1.camel@pengutronix.de>
Date:   Tue, 15 Mar 2022 10:48:42 +0100
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Shaik Sajida Bhanu <quic_c_sbhanu@...cinc.com>,
        adrian.hunter@...el.com, agross@...nel.org,
        bjorn.andersson@...aro.org, ulf.hansson@...aro.org,
        chris@...ntf.net, gdjakov@...sol.com
Cc:     linux-mmc@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, quic_asutoshd@...cinc.com,
        quic_rampraka@...cinc.com, quic_pragalla@...cinc.com,
        quic_sartgarg@...cinc.com, quic_nitirawa@...cinc.com,
        quic_sayalil@...cinc.com
Subject: Re: [PATCH V2] mmc: sdhci-msm: Reset GCC_SDCC_BCR register for SDHC

Hi Sajida,

On Do, 2022-03-10 at 21:10 +0530, Shaik Sajida Bhanu wrote:
[...]
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-
> msm.c
> index 50c71e0..cb33c9a 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
[...]
> @@ -2482,6 +2484,45 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
>         of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
>  }
>  
> +static int sdhci_msm_gcc_reset(struct platform_device *pdev,
> +              struct sdhci_host *host)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +       int ret = 0;
> +
> +       msm_host->core_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "core_reset");

I think the "_reset" part in the name is superfluous and this reset
control should be called "core". Is this documented in the sdhci-msm
device tree binding document?

> +       if (IS_ERR(msm_host->core_reset)) {
> +               ret = PTR_ERR(msm_host->core_reset);
> +               dev_err(&pdev->dev, "core_reset unavailable (%d)\n", ret);
> +               msm_host->core_reset = NULL;

As Bjorn pointed out, this error should be returned.
reset_control_get_optional returns NULL if the optional reset control
is not specified in the device tree, so we only land here if there's a
real error.

[...]
> @@ -2529,6 +2570,13 @@ static int sdhci_msm_probe(struct
> platform_device *pdev)
>  
>         msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>  
> +       ret = sdhci_msm_gcc_reset(pdev, host);
> +       if (ret) {
> +               dev_err(&pdev->dev, "core_reset assert/deassert failed (%d)\n",
> +                                       ret);
> +               goto pltfm_free;
> +       }
> +
>         /* Setup SDCC bus voter clock. */
>         msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
>         if (!IS_ERR(msm_host->bus_clk)) {

I notice that this driver requests resources such as clocks and resets
and then immediately uses them, one by one. It would be better to
request all resources first, and only then start interacting with the
hardware. This is not an issue that can be fixed in this patch,
although maybe it could be prepared for it by separating the
reset_control_get from the _assert/deassert.

regards
Philipp