linux-kernel - Re: [PATCH 3/5] scsi: ufs: qcom: Add a readl() to make sure ref

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: <20220423114535.GB374560@thinkpad>
Date:   Sat, 23 Apr 2022 17:15:35 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Bart Van Assche <bvanassche@....org>
Cc:     martin.petersen@...cle.com, jejb@...ux.ibm.com,
        avri.altman@....com, alim.akhtar@...sung.com,
        bjorn.andersson@...aro.org, linux-arm-msm@...r.kernel.org,
        quic_asutoshd@...cinc.com, quic_cang@...cinc.com,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH 3/5] scsi: ufs: qcom: Add a readl() to make sure ref_clk
 gets enabled

On Fri, Apr 22, 2022 at 10:03:22PM -0700, Bart Van Assche wrote:
> On 4/22/22 06:21, Manivannan Sadhasivam wrote:
> > In ufs_qcom_dev_ref_clk_ctrl(), it was noted that the ref_clk needs to be
> > stable for atleast 1us. Eventhough there is wmb() to make sure the write
>                ^              ^
> Some spaces are missing.
> 
> > gets "completed", there is no guarantee that the write actually reached
> > the UFS device. There is a good chance that the write could be stored in
> > a Write Buffer (WB). In that case, eventhough the CPU waits for 1us, the
>                                          ^
> missing space----------------------------
> 

Kind of used to it ;) Will fix it.

> > ref_clk might not be stable for that period.
> > 
> > So lets do a readl() to make sure that the previous write has reached the
> > UFS device before udelay().
> > 
> > Cc: stable@...r.kernel.org
> > Fixes: f06fcc7155dc ("scsi: ufs-qcom: add QUniPro hardware support and power optimizations")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > ---
> >   drivers/scsi/ufs/ufs-qcom.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > index 5f0a8f646eb5..5b9986c63eed 100644
> > --- a/drivers/scsi/ufs/ufs-qcom.c
> > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > @@ -690,6 +690,12 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
> >   		/* ensure that ref_clk is enabled/disabled before we return */
> >   		wmb();
> > +		/*
> > +		 * Make sure the write to ref_clk reaches the destination and
> > +		 * not stored in a Write Buffer (WB).
> > +		 */
> > +		readl(host->dev_ref_clk_ctrl_mmio);
> > +
> >   		/*
> >   		 * If we call hibern8 exit after this, we need to make sure that
> >   		 * device ref_clk is stable for at least 1us before the hibern8
> 
> The comment above the wmb() call looks wrong to me. How about removing that
> wmb() call?
> 

Hmm, yes it could be removed as well. readl() on weakly ordered architectures
include a control dependency [1] so there is no way the instructions after it
can be speculated.

Thanks,
Mani

[1] https://www.spinics.net/lists/arm-kernel/msg689858.html

> Thanks,
> 
> Bart.