linux-kernel - Re: [PATCH v3 0/1] scsi: ufs: Add hibernation callbacks

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: <9330ae10-de30-89be-cf53-f50db9610532@acm.org>
Date:   Fri, 20 Jan 2023 13:12:08 -0800
From:   Bart Van Assche <bvanassche@....org>
To:     Anjana Hari <quic_ahari@...cinc.com>, agross@...nel.org,
        andersson@...nel.org, jejb@...ux.ibm.com,
        martin.petersen@...cle.com
Cc:     alim.akhtar@...sung.com, avri.altman@....com,
        konrad.dybcio@...aro.org, linux-scsi@...r.kernel.org,
        linux-kernel@...r.kernel.org, quic_narepall@...cinc.com,
        quic_nitirawa@...cinc.com, quic_rampraka@...cinc.com
Subject: Re: [PATCH v3 0/1] scsi: ufs: Add hibernation callbacks

On 1/20/23 03:33, Anjana Hari wrote:
>   - Note to Bart: Regrading the comment to pass "restore" as an
>   argument instead of adding a new member to ufs_hba structure, adding
>   new function argument in core file (ufshcd.c) is forcing us to make
>   changes to other vendor files to fix the compilation errors. Hence
>   we have retained our original change. Please let us know your inputs
>   on this. 
Storing state information in a structure member that can be passed as a
function argument makes code harder to read and to maintain than
necessary. Please address my request before this patch goes upstream. I'm
concerned if someone would try to address my request after this patch went
upstream that there would be no motivation from your side to help with
testing the refactoring patch.

I think the patch below shows that it is easy to eliminate the new 'restore'
member variable. Please note that the patch below has not been tested in any
way.

---
  drivers/ufs/core/ufshcd.c | 48 +++++++++++++++++++--------------------
  include/ufs/ufshcd.h      |  3 ---
  2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 19608f3a38f9..b5cfbc1fccc6 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9801,34 +9801,11 @@ static int ufshcd_resume(struct ufs_hba *hba)
  	/* enable the host irq as host controller would be active soon */
  	ufshcd_enable_irq(hba);

-	if (hba->restore) {
-		/* Configure UTRL and UTMRL base address registers */
-		ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr),
-			      REG_UTP_TRANSFER_REQ_LIST_BASE_L);
-		ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
-			      REG_UTP_TRANSFER_REQ_LIST_BASE_H);
-		ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
-			      REG_UTP_TASK_REQ_LIST_BASE_L);
-		ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
-			      REG_UTP_TASK_REQ_LIST_BASE_H);
-		/* Make sure that UTRL and UTMRL base address registers
-		 * are updated with the latest queue addresses. Only after
-		 * updating these addresses, we can queue the new commands.
-		 */
-		mb();
-	}
-
-	/* Resuming from hibernate, assume that link was OFF */
-	if (hba->restore)
-		ufshcd_set_link_off(hba);
-
  	goto out;

  disable_vreg:
  	ufshcd_vreg_set_lpm(hba);
  out:
-	if (hba->restore)
-		hba->restore = false;

  	if (ret)
  		ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret);
@@ -10012,10 +9989,31 @@ int ufshcd_system_restore(struct device *dev)
  {

  	struct ufs_hba *hba = dev_get_drvdata(dev);
+	int ret;

-	hba->restore = true;
-	return ufshcd_system_resume(dev);
+	ret = ufshcd_system_resume(dev);
+	if (ret)
+		return ret;
+
+	/* Configure UTRL and UTMRL base address registers */
+	ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr),
+		      REG_UTP_TRANSFER_REQ_LIST_BASE_L);
+	ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
+		      REG_UTP_TRANSFER_REQ_LIST_BASE_H);
+	ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
+		      REG_UTP_TASK_REQ_LIST_BASE_L);
+	ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
+		      REG_UTP_TASK_REQ_LIST_BASE_H);
+	/* Make sure that UTRL and UTMRL base address registers
+	 * are updated with the latest queue addresses. Only after
+	 * updating these addresses, we can queue the new commands.
+	 */
+	mb();

+	/* Resuming from hibernate, assume that link was OFF */
+	ufshcd_set_link_off(hba);
+
+	return 0;
  }
  EXPORT_SYMBOL_GPL(ufshcd_system_restore);

diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 6f50390ca262..1d6dd13e1651 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1071,9 +1071,6 @@ struct ufs_hba {
  	struct ufs_hw_queue *uhq;
  	struct ufs_hw_queue *dev_cmd_queue;
  	struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
-
-	/* Distinguish between resume and restore */
-	bool restore;
  };

  /**