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: <SJ0PR02MB84497927196D953D113F8337CD119@SJ0PR02MB8449.namprd02.prod.outlook.com>
Date:   Wed, 16 Mar 2022 13:15:47 +0000
From:   "Sajida Bhanu (Temp) (QUIC)" <quic_c_sbhanu@...cinc.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>,
        "Sajida Bhanu (Temp) (QUIC)" <quic_c_sbhanu@...cinc.com>,
        "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "agross@...nel.org" <agross@...nel.org>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "chris@...ntf.net" <chris@...ntf.net>,
        "gdjakov@...sol.com" <gdjakov@...sol.com>
CC:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Asutosh Das (QUIC)" <quic_asutoshd@...cinc.com>,
        "Ram Prakash Gupta (QUIC)" <quic_rampraka@...cinc.com>,
        "Pradeep Pragallapati (QUIC)" <quic_pragalla@...cinc.com>,
        "Sarthak Garg (QUIC)" <quic_sartgarg@...cinc.com>,
        "Nitin Rawat (QUIC)" <quic_nitirawa@...cinc.com>,
        "Sayali Lokhande (QUIC)" <quic_sayalil@...cinc.com>
Subject: RE: [PATCH V2] mmc: sdhci-msm: Reset GCC_SDCC_BCR register for SDHC

Hi,

Thanks for the review.

Please find the inline comments.

Thanks,
Sajida
> -----Original Message-----
> From: Philipp Zabel <p.zabel@...gutronix.de>
> Sent: Tuesday, March 15, 2022 3:19 PM
> To: Sajida Bhanu (Temp) (QUIC) <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; Asutosh Das (QUIC) <quic_asutoshd@...cinc.com>;
> Ram Prakash Gupta (QUIC) <quic_rampraka@...cinc.com>; Pradeep
> Pragallapati (QUIC) <quic_pragalla@...cinc.com>; Sarthak Garg (QUIC)
> <quic_sartgarg@...cinc.com>; Nitin Rawat (QUIC)
> <quic_nitirawa@...cinc.com>; Sayali Lokhande (QUIC)
> <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?
Followed existing clients...
No its not added in the dt-binbing,  will add dt-binding patch in patch version.
> 
> > +       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.
> 
Ok
> [...]
> > @@ -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.
>
 Sure, we will discuss this proposal internally and update.
> regards
> Philipp