linux-kernel - Re: [PATCH v1] ufs: ufs-qcom: Align programming sequence of Shared ICE for UFS controller v5

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: <pewnau4ltrf2yu3xxdq6rs6xhz45zlo3dt3jnkzhxitmezz2ft@2k7pgpoz5iey>
Date: Wed, 6 Aug 2025 23:19:13 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Palash Kambar <quic_pkambar@...cinc.com>
Cc: James.Bottomley@...senpartnership.com, martin.petersen@...cle.com, 
	linux-arm-msm@...r.kernel.org, linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org, 
	quic_nitirawa@...cinc.com
Subject: Re: [PATCH v1] ufs: ufs-qcom: Align programming sequence of Shared
 ICE for UFS controller v5

On Wed, Aug 06, 2025 at 06:11:09PM GMT, Palash Kambar wrote:
> 
> 
> On 8/6/2025 4:44 PM, Manivannan Sadhasivam wrote:
> > On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
> >> Disable of AES core in Shared ICE is not supported during power
> >> collapse for UFS Host Controller V5.0.
> >>
> >> Hence follow below steps to reset the ICE upon exiting power collapse
> >> and align with Hw programming guide.
> >>
> >> a. Write 0x18 to UFS_MEM_ICE_CFG
> >> b. Write 0x0 to UFS_MEM_ICE_CFG
> >>
> >> Signed-off-by: Palash Kambar <quic_pkambar@...cinc.com>
> >> ---
> >>  drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
> >>  drivers/ufs/host/ufs-qcom.h |  2 ++
> >>  2 files changed, 26 insertions(+)
> >>
> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> >> index 444a09265ded..2744614bbc32 100644
> >> --- a/drivers/ufs/host/ufs-qcom.c
> >> +++ b/drivers/ufs/host/ufs-qcom.c
> >> @@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
> >>  	if (ufs_qcom_is_link_off(hba) && host->device_reset)
> >>  		ufs_qcom_device_reset_ctrl(hba, true);
> >>  
> >> +	host->vdd_hba_pc = true;
> > 
> > What does this variable correspond to?
> Hi Manivannan,
> 
> It corresponds to power collapse, will rename it for better readability.
> 

What is 'power collapse' from UFS perspective?

> > 
> >> +
> >>  	return ufs_qcom_ice_suspend(host);
> >>  }
> >>  
> >> @@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >>  	return ufs_qcom_ice_resume(host);
> >>  }
> >>  
> >> +static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
> >> +				    enum uic_cmd_dme uic_cmd,
> >> +				    enum ufs_notify_change_status status)
> >> +{
> >> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> >> +
> >> +	/* Apply shared ICE WA */
> > 
> > Are you really sure it is *shared ICE*?
> 
>  Yes Manivannan, I am.
> 

Well, there are two kind of registers defined in the internal doc that I can
see: UFS_MEM_ICE and UFS_MEM_SHARED_ICE. And hence the question.

> > 
> >> +	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
> >> +	    status == POST_CHANGE &&
> >> +	    host->hw_ver.major == 0x5 &&
> >> +	    host->hw_ver.minor == 0x0 &&
> >> +	    host->hw_ver.step == 0x0 &&
> >> +	    host->vdd_hba_pc) {
> >> +		host->vdd_hba_pc = false;
> >> +		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);
> > 
> > Define the actual bits instead of writing magic values.
> 
> Sure.
> 
> > 
> >> +		ufshcd_readl(hba, UFS_MEM_ICE);
> >> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
> >> +		ufshcd_readl(hba, UFS_MEM_ICE);
> > 
> > Why do you need readl()? Writes to device memory won't get reordered.
> 
> Since these are hardware register, there is a potential for reordering.
> 

Really? Who said that? Please cite the reference.

> > 
> >> +	}
> >> +}
> >> +
> >>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
> >>  {
> >>  	if (host->dev_ref_clk_ctrl_mmio &&
> >> @@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
> >>  	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
> >>  	.link_startup_notify    = ufs_qcom_link_startup_notify,
> >>  	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
> >> +	.hibern8_notify		= ufs_qcom_hibern8_notify,
> > 
> > This callback is not called anywhere. Regardeless of that, why can't you use
> > ufs_qcom_clk_scale_notify()?
> > 
> 
> According to the HPG guidelines, as part of this workaround, we are required to reset the ICE controller during the Hibern8 exit sequence when the UFS controller resumes from power collapse. Therefore, this reset logic has been added to the H8 exit notifier callback.
> 

Please wrap the replies to 80 column.

Well, we do call ufshcd_uic_hibern8_exit() from these callbacks. So why can't
you reset the ICE after calling ufshcd_uic_hibern8_exit() here?

> The ufs_clk_scale_notify function is invoked whenever clock scaling (up or down) occurs, regardless of whether a power collapse has taken place. Hence, the ICE controller reset was specifically added to the H8 exit notifier to ensure it is executed only in the appropriate context.
> 

Please define what 'power collapse' mean here. And as I said before, you are
not at all calling this newly introduced callback.

- Mani

-- 
மணிவண்ணன் சதாசிவம்