[PATCH v3 0/5] Prepare for upstreaming Pixel 6 and 7 UFS support

[PATCH v3 0/5] Prepare for upstreaming Pixel 6 and 7 UFS support

[Bart Van Assche]

Hi Martin,

The UFS controller in the Google Pixel 6 and 7 phones requires that SCSI
command processing is suspended while reprogramming encryption keys. The
patches in this series are a first step towards integrating support in the
upstream kernel for the UFS controller in the Pixel 6 and 7. Please consider
these patches for the next merge window.

Note: instructions for downloading the Pixel kernel source code are available
at https://source.android.com/setup/build/building-kernels.

Thanks,

Bart.

Changes compared to v2:
- Addressed more review comments from Avri.

Changes compared to v1:
- Addressed Avri's review comments.
- Added patch "Allow UFS host drivers to override the sg entry size".

Bart Van Assche (4):
  scsi: ufs: Reduce the clock scaling latency
  scsi: ufs: Move a clock scaling check
  scsi: ufs: Pass the clock scaling timeout as an argument
  scsi: ufs: Add suspend/resume SCSI command processing support

Eric Biggers (1):
  scsi: ufs: Allow UFS host drivers to override the sg entry size

 drivers/ufs/core/ufshcd.c | 89 +++++++++++++++++++++++++++------------
 drivers/ufs/host/Kconfig  | 10 +++++
 include/ufs/ufshcd.h      | 35 +++++++++++++++
 include/ufs/ufshci.h      |  9 +++-
 4 files changed, 114 insertions(+), 29 deletions(-)

[PATCH v3 1/5] scsi: ufs: Reduce the clock scaling latency

[Bart Van Assche]

Wait at most 20 ms before rechecking the doorbells instead of waiting
for a potentially long time between doorbell checks.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 768cb49d269c..81c20e315dba 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1154,7 +1154,7 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 		}
 
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
-		schedule();
+		io_schedule_timeout(msecs_to_jiffies(20));
 		if (ktime_to_us(ktime_sub(ktime_get(), start)) >
 		    wait_timeout_us) {
 			timeout = true;

[PATCH v3 2/5] scsi: ufs: Move a clock scaling check

[Bart Van Assche]

Move a check related to clock scaling into ufshcd_devfreq_scale(). This
patch prepares for adding a second ufshcd_clock_scaling_prepare()
caller in a function not related to clock scaling.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 81c20e315dba..195261e3521c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1236,8 +1236,7 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 	ufshcd_scsi_block_requests(hba);
 	down_write(&hba->clk_scaling_lock);
 
-	if (!hba->clk_scaling.is_allowed ||
-	    ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
+	if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
 		ret = -EBUSY;
 		up_write(&hba->clk_scaling_lock);
 		ufshcd_scsi_unblock_requests(hba);
@@ -1275,10 +1274,18 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 	int ret = 0;
 	bool is_writelock = true;
 
+	if (!hba->clk_scaling.is_allowed)
+		return -EBUSY;
+
 	ret = ufshcd_clock_scaling_prepare(hba);
 	if (ret)
 		return ret;
 
+	if (!hba->clk_scaling.is_allowed) {
+		ret = -EBUSY;
+		goto out_unprepare;
+	}
+
 	/* scale down the gear before scaling down clocks */
 	if (!scale_up) {
 		ret = ufshcd_scale_gear(hba, false);

[PATCH v3 3/5] scsi: ufs: Pass the clock scaling timeout as an argument

[Bart Van Assche]

Prepare for adding an additional ufshcd_clock_scaling_prepare() call
with a different timeout.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 195261e3521c..7b2948592c4a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1121,6 +1121,12 @@ static u32 ufshcd_pending_cmds(struct ufs_hba *hba)
 	return pending;
 }
 
+/*
+ * Wait until all pending SCSI commands and TMFs have finished or the timeout
+ * has expired.
+ *
+ * Return: 0 upon success; -EBUSY upon timeout.
+ */
 static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 					u64 wait_timeout_us)
 {
@@ -1225,9 +1231,14 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
 	return ret;
 }
 
-static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
+/*
+ * Wait until all pending SCSI commands and TMFs have finished or the timeout
+ * has expired.
+ *
+ * Return: 0 upon success; -EBUSY upon timeout.
+ */
+static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba, u64 timeout_us)
 {
-	#define DOORBELL_CLR_TOUT_US		(1000 * 1000) /* 1 sec */
 	int ret = 0;
 	/*
 	 * make sure that there are no outstanding requests when
@@ -1236,7 +1247,7 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 	ufshcd_scsi_block_requests(hba);
 	down_write(&hba->clk_scaling_lock);
 
-	if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
+	if (ufshcd_wait_for_doorbell_clr(hba, timeout_us)) {
 		ret = -EBUSY;
 		up_write(&hba->clk_scaling_lock);
 		ufshcd_scsi_unblock_requests(hba);
@@ -1277,7 +1288,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 	if (!hba->clk_scaling.is_allowed)
 		return -EBUSY;
 
-	ret = ufshcd_clock_scaling_prepare(hba);
+	ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
 	if (ret)
 		return ret;
 

[PATCH v3 4/5] scsi: ufs: Add suspend/resume SCSI command processing support

[Bart Van Assche]

This functionality is needed by UFS drivers to e.g. suspend SCSI command
processing while reprogramming encryption keys if the hardware does not
support concurrent I/O and key reprogramming. This patch prepares for
adding support in the upstream kernel for the Pixel 6 and 7 UFS
controllers.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 20 ++++++++++++++++++++
 include/ufs/ufshcd.h      |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7b2948592c4a..fa1c84731b8e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1707,6 +1707,26 @@ static void ufshcd_ungate_work(struct work_struct *work)
 	ufshcd_scsi_unblock_requests(hba);
 }
 
+/*
+ * Block processing of new SCSI commands and wait until pending SCSI
+ * commands and TMFs have finished. ufshcd_exec_dev_cmd() and
+ * ufshcd_issue_devman_upiu_cmd() are not affected by this function.
+ *
+ * Return: 0 upon success; -EBUSY upon timeout.
+ */
+int ufshcd_freeze_scsi_devs(struct ufs_hba *hba, u64 timeout_us)
+{
+	return ufshcd_clock_scaling_prepare(hba, timeout_us);
+}
+EXPORT_SYMBOL_GPL(ufshcd_freeze_scsi_devs);
+
+/* Resume processing of SCSI commands. */
+void ufshcd_unfreeze_scsi_devs(struct ufs_hba *hba)
+{
+	ufshcd_clock_scaling_unprepare(hba, true);
+}
+EXPORT_SYMBOL_GPL(ufshcd_unfreeze_scsi_devs);
+
 /**
  * ufshcd_hold - Enable clocks that were gated earlier due to ufshcd_release.
  * Also, exit from hibern8 mode and set the link as active.
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 5cf81dff60aa..bd45818bf0e8 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1186,6 +1186,9 @@ void ufshcd_release(struct ufs_hba *hba);
 
 void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value);
 
+int ufshcd_freeze_scsi_devs(struct ufs_hba *hba, u64 timeout_us);
+void ufshcd_unfreeze_scsi_devs(struct ufs_hba *hba);
+
 void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 				  int *desc_length);
 

RE: [PATCH v3 4/5] scsi: ufs: Add suspend/resume SCSI command processing support

[Avri Altman]

 
> This functionality is needed by UFS drivers to e.g. suspend SCSI command
> processing while reprogramming encryption keys if the hardware does not
> support concurrent I/O and key reprogramming. This patch prepares for adding
> support in the upstream kernel for the Pixel 6 and 7 UFS controllers.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
I guess that you are planning to upstream the code that uses it, hence:

Reviewed-by: Avri Altman <avri.altman@wdc.com>


> ---
>  drivers/ufs/core/ufshcd.c | 20 ++++++++++++++++++++
>  include/ufs/ufshcd.h      |  3 +++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 7b2948592c4a..fa1c84731b8e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1707,6 +1707,26 @@ static void ufshcd_ungate_work(struct work_struct
> *work)
>         ufshcd_scsi_unblock_requests(hba);
>  }
> 
> +/*
> + * Block processing of new SCSI commands and wait until pending SCSI
> + * commands and TMFs have finished. ufshcd_exec_dev_cmd() and
> + * ufshcd_issue_devman_upiu_cmd() are not affected by this function.
> + *
> + * Return: 0 upon success; -EBUSY upon timeout.
> + */
> +int ufshcd_freeze_scsi_devs(struct ufs_hba *hba, u64 timeout_us) {
> +       return ufshcd_clock_scaling_prepare(hba, timeout_us); }
> +EXPORT_SYMBOL_GPL(ufshcd_freeze_scsi_devs);
> +
> +/* Resume processing of SCSI commands. */ void
> +ufshcd_unfreeze_scsi_devs(struct ufs_hba *hba) {
> +       ufshcd_clock_scaling_unprepare(hba, true); }
> +EXPORT_SYMBOL_GPL(ufshcd_unfreeze_scsi_devs);
> +
>  /**
>   * ufshcd_hold - Enable clocks that were gated earlier due to ufshcd_release.
>   * Also, exit from hibern8 mode and set the link as active.
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index
> 5cf81dff60aa..bd45818bf0e8 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1186,6 +1186,9 @@ void ufshcd_release(struct ufs_hba *hba);
> 
>  void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value);
> 
> +int ufshcd_freeze_scsi_devs(struct ufs_hba *hba, u64 timeout_us); void
> +ufshcd_unfreeze_scsi_devs(struct ufs_hba *hba);
> +
>  void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn
> desc_id,
>                                   int *desc_length);

Re: [PATCH v3 4/5] scsi: ufs: Add suspend/resume SCSI command processing support

[Bart Van Assche]

On 11/10/22 03:07, Avri Altman wrote:
> I guess that you are planning to upstream the code that uses it [ ... ]

The code that uses these functions will be sent upstream by Linaro. 
Linaro has been asked to do the majority of the work of upstreaming the 
Pixel 6 kernel code. My goal with this patch series is to minimize the 
differences between the AOSP UFS code and the upstream UFS code.

Thanks,

Bart.

[PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Bart Van Assche]

From: Eric Biggers <ebiggers@google.com>

Modify the UFSHCD core to allow 'struct ufshcd_sg_entry' to be
variable-length. The default is the standard length, but variants can
override ufs_hba::sg_entry_size with a larger value if there are
vendor-specific fields following the standard ones.

This is needed to support inline encryption with ufs-exynos (FMP).

Cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
[ bvanassche: edited commit message and introduced CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE ]
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 39 ++++++++++++++++++---------------------
 drivers/ufs/host/Kconfig  | 10 ++++++++++
 include/ufs/ufshcd.h      | 32 ++++++++++++++++++++++++++++++++
 include/ufs/ufshci.h      |  9 +++++++--
 4 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index fa1c84731b8e..6df24aed970f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -528,7 +528,7 @@ void ufshcd_print_trs(struct ufs_hba *hba, unsigned long bitmap, bool pr_prdt)
 		prdt_length = le16_to_cpu(
 			lrbp->utr_descriptor_ptr->prd_table_length);
 		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
-			prdt_length /= sizeof(struct ufshcd_sg_entry);
+			prdt_length /= ufshcd_sg_entry_size(hba);
 
 		dev_err(hba->dev,
 			"UPIU[%d] - PRDT - %d entries  phys@0x%llx\n",
@@ -537,7 +537,7 @@ void ufshcd_print_trs(struct ufs_hba *hba, unsigned long bitmap, bool pr_prdt)
 
 		if (pr_prdt)
 			ufshcd_hex_dump("UPIU PRDT: ", lrbp->ucd_prdt_ptr,
-				sizeof(struct ufshcd_sg_entry) * prdt_length);
+				ufshcd_sg_entry_size(hba) * prdt_length);
 	}
 }
 
@@ -2445,7 +2445,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
  */
 static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 {
-	struct ufshcd_sg_entry *prd_table;
+	struct ufshcd_sg_entry *prd;
 	struct scatterlist *sg;
 	struct scsi_cmnd *cmd;
 	int sg_segments;
@@ -2460,13 +2460,12 @@ static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 
 		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
 			lrbp->utr_descriptor_ptr->prd_table_length =
-				cpu_to_le16((sg_segments *
-					sizeof(struct ufshcd_sg_entry)));
+				cpu_to_le16(sg_segments * ufshcd_sg_entry_size(hba));
 		else
 			lrbp->utr_descriptor_ptr->prd_table_length =
 				cpu_to_le16(sg_segments);
 
-		prd_table = lrbp->ucd_prdt_ptr;
+		prd = lrbp->ucd_prdt_ptr;
 
 		scsi_for_each_sg(cmd, sg, sg_segments, i) {
 			const unsigned int len = sg_dma_len(sg);
@@ -2480,9 +2479,10 @@ static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 			 * indicates 4 bytes, '7' indicates 8 bytes, etc."
 			 */
 			WARN_ONCE(len > 256 * 1024, "len = %#x\n", len);
-			prd_table[i].size = cpu_to_le32(len - 1);
-			prd_table[i].addr = cpu_to_le64(sg->dma_address);
-			prd_table[i].reserved = 0;
+			prd->size = cpu_to_le32(len - 1);
+			prd->addr = cpu_to_le64(sg->dma_address);
+			prd->reserved = 0;
+			prd = (void *)prd + ufshcd_sg_entry_size(hba);
 		}
 	} else {
 		lrbp->utr_descriptor_ptr->prd_table_length = 0;
@@ -2772,10 +2772,11 @@ static void ufshcd_map_queues(struct Scsi_Host *shost)
 
 static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
 {
-	struct utp_transfer_cmd_desc *cmd_descp = hba->ucdl_base_addr;
+	struct utp_transfer_cmd_desc *cmd_descp = (void *)hba->ucdl_base_addr +
+		i * sizeof_utp_transfer_cmd_desc(hba);
 	struct utp_transfer_req_desc *utrdlp = hba->utrdl_base_addr;
 	dma_addr_t cmd_desc_element_addr = hba->ucdl_dma_addr +
-		i * sizeof(struct utp_transfer_cmd_desc);
+		i * sizeof_utp_transfer_cmd_desc(hba);
 	u16 response_offset = offsetof(struct utp_transfer_cmd_desc,
 				       response_upiu);
 	u16 prdt_offset = offsetof(struct utp_transfer_cmd_desc, prd_table);
@@ -2783,11 +2784,11 @@ static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
 	lrb->utr_descriptor_ptr = utrdlp + i;
 	lrb->utrd_dma_addr = hba->utrdl_dma_addr +
 		i * sizeof(struct utp_transfer_req_desc);
-	lrb->ucd_req_ptr = (struct utp_upiu_req *)(cmd_descp + i);
+	lrb->ucd_req_ptr = (struct utp_upiu_req *)cmd_descp->command_upiu;
 	lrb->ucd_req_dma_addr = cmd_desc_element_addr;
-	lrb->ucd_rsp_ptr = (struct utp_upiu_rsp *)cmd_descp[i].response_upiu;
+	lrb->ucd_rsp_ptr = (struct utp_upiu_rsp *)cmd_descp->response_upiu;
 	lrb->ucd_rsp_dma_addr = cmd_desc_element_addr + response_offset;
-	lrb->ucd_prdt_ptr = cmd_descp[i].prd_table;
+	lrb->ucd_prdt_ptr = (struct ufshcd_sg_entry *)cmd_descp->prd_table;
 	lrb->ucd_prdt_dma_addr = cmd_desc_element_addr + prdt_offset;
 }
 
@@ -3696,7 +3697,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
 	size_t utmrdl_size, utrdl_size, ucdl_size;
 
 	/* Allocate memory for UTP command descriptors */
-	ucdl_size = (sizeof(struct utp_transfer_cmd_desc) * hba->nutrs);
+	ucdl_size = sizeof_utp_transfer_cmd_desc(hba) * hba->nutrs;
 	hba->ucdl_base_addr = dmam_alloc_coherent(hba->dev,
 						  ucdl_size,
 						  &hba->ucdl_dma_addr,
@@ -3790,7 +3791,7 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
 	prdt_offset =
 		offsetof(struct utp_transfer_cmd_desc, prd_table);
 
-	cmd_desc_size = sizeof(struct utp_transfer_cmd_desc);
+	cmd_desc_size = sizeof_utp_transfer_cmd_desc(hba);
 	cmd_desc_dma_addr = hba->ucdl_dma_addr;
 
 	for (i = 0; i < hba->nutrs; i++) {
@@ -9661,6 +9662,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 	hba->dev = dev;
 	hba->dev_ref_clk_freq = REF_CLK_FREQ_INVAL;
 	hba->nop_out_timeout = NOP_OUT_TIMEOUT;
+	ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
 	INIT_LIST_HEAD(&hba->clk_list_head);
 	spin_lock_init(&hba->outstanding_lock);
 
@@ -10039,11 +10041,6 @@ static int __init ufshcd_core_init(void)
 {
 	int ret;
 
-	/* Verify that there are no gaps in struct utp_transfer_cmd_desc. */
-	static_assert(sizeof(struct utp_transfer_cmd_desc) ==
-		      2 * ALIGNED_UPIU_SIZE +
-			      SG_ALL * sizeof(struct ufshcd_sg_entry));
-
 	ufs_debugfs_init();
 
 	ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
index 4cc2dbd79ed0..49017abdac92 100644
--- a/drivers/ufs/host/Kconfig
+++ b/drivers/ufs/host/Kconfig
@@ -124,3 +124,13 @@ config SCSI_UFS_EXYNOS
 
 	  Select this if you have UFS host controller on Samsung Exynos SoC.
 	  If unsure, say N.
+
+config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
+	bool "Variable size UTP physical region descriptor"
+	help
+	  In the UFSHCI 3.0 standard the Physical Region Descriptor (PRD) is a
+	  data structure used for transferring data between host and UFS
+	  device. This data structure describes a single region in physical
+	  memory. Although the standard requires that this data structure has a
+	  size of 16 bytes, for some controllers this data structure has a
+	  different size. Enable this option for UFS controllers that need it.
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index bd45818bf0e8..c6854514e40e 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -754,6 +754,9 @@ struct ufs_hba_monitor {
  * @vops: pointer to variant specific operations
  * @vps: pointer to variant specific parameters
  * @priv: pointer to variant specific private data
+#ifdef CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
+ * @sg_entry_size: size of struct ufshcd_sg_entry (may include variant fields)
+#endif
  * @irq: Irq number of the controller
  * @is_irq_enabled: whether or not the UFS controller interrupt is enabled.
  * @dev_ref_clk_freq: reference clock frequency
@@ -877,6 +880,9 @@ struct ufs_hba {
 	const struct ufs_hba_variant_ops *vops;
 	struct ufs_hba_variant_params *vps;
 	void *priv;
+#ifdef CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
+	size_t sg_entry_size;
+#endif
 	unsigned int irq;
 	bool is_irq_enabled;
 	enum ufs_ref_clk_freq dev_ref_clk_freq;
@@ -980,6 +986,32 @@ struct ufs_hba {
 	bool complete_put;
 };
 
+#ifdef CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
+static inline size_t ufshcd_sg_entry_size(const struct ufs_hba *hba)
+{
+	return hba->sg_entry_size;
+}
+
+static inline void ufshcd_set_sg_entry_size(struct ufs_hba *hba, size_t sg_entry_size)
+{
+	WARN_ON_ONCE(sg_entry_size < sizeof(struct ufshcd_sg_entry));
+	hba->sg_entry_size = sg_entry_size;
+}
+#else
+static inline size_t ufshcd_sg_entry_size(const struct ufs_hba *hba)
+{
+	return sizeof(struct ufshcd_sg_entry);
+}
+
+#define ufshcd_set_sg_entry_size(hba, sg_entry_size)                   \
+	({ (void)(hba); BUILD_BUG_ON(sg_entry_size != sizeof(struct ufshcd_sg_entry)); })
+#endif
+
+static inline size_t sizeof_utp_transfer_cmd_desc(const struct ufs_hba *hba)
+{
+	return sizeof(struct utp_transfer_cmd_desc) + SG_ALL * ufshcd_sg_entry_size(hba);
+}
+
 /* Returns true if clocks can be gated. Otherwise false */
 static inline bool ufshcd_is_clkgating_allowed(struct ufs_hba *hba)
 {
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index f525566a0864..e145a478afa2 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -422,18 +422,23 @@ struct ufshcd_sg_entry {
 	__le64    addr;
 	__le32    reserved;
 	__le32    size;
+	/*
+	 * followed by variant-specific fields if
+	 * CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE has been defined.
+	 */
 };
 
 /**
  * struct utp_transfer_cmd_desc - UTP Command Descriptor (UCD)
  * @command_upiu: Command UPIU Frame address
  * @response_upiu: Response UPIU Frame address
- * @prd_table: Physical Region Descriptor
+ * @prd_table: Physical Region Descriptor: an array of SG_ALL struct
+ *	ufshcd_sg_entry's.  Variant-specific fields may be present after each.
  */
 struct utp_transfer_cmd_desc {
 	u8 command_upiu[ALIGNED_UPIU_SIZE];
 	u8 response_upiu[ALIGNED_UPIU_SIZE];
-	struct ufshcd_sg_entry    prd_table[SG_ALL];
+	u8 prd_table[];
 };
 
 /**

RE: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Avri Altman]

> From: Eric Biggers <ebiggers@google.com>
> 
> Modify the UFSHCD core to allow 'struct ufshcd_sg_entry' to be
> variable-length. The default is the standard length, but variants can
> override ufs_hba::sg_entry_size with a larger value if there are
> vendor-specific fields following the standard ones.
> 
> This is needed to support inline encryption with ufs-exynos (FMP).
> 
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> [ bvanassche: edited commit message and introduced
> CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE ]
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Christoph Hellwig]

On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Modify the UFSHCD core to allow 'struct ufshcd_sg_entry' to be
> variable-length. The default is the standard length, but variants can
> override ufs_hba::sg_entry_size with a larger value if there are
> vendor-specific fields following the standard ones.

There is absolutely nothing 'vendor' in here, it is all implementation
specifc.  I have no idea why no touching ufs can grasp this.

Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Bart Van Assche]

On 11/9/22 04:34, Christoph Hellwig wrote:
> On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote:
>> From: Eric Biggers <ebiggers@google.com>
>>
>> Modify the UFSHCD core to allow 'struct ufshcd_sg_entry' to be
>> variable-length. The default is the standard length, but variants can
>> override ufs_hba::sg_entry_size with a larger value if there are
>> vendor-specific fields following the standard ones.
> 
> There is absolutely nothing 'vendor' in here, it is all implementation
> specifc.  I have no idea why no touching ufs can grasp this.

Hi Christoph,

I'm not sure how to interpret your reply. Anyway, this patch is required 
to use the encryption functionality of the UFS Exynos controller. The 
"vendor-specific fields" text in the patch description refers to the 
encryption fields since these follow the data buffer when using the 
Exynos controller. Although it makes me unhappy that the UFS Exynos 
controller is not compliant with the UFS specification, since it is 
being used widely I think we need support for this controller in the 
upstream kernel.

Thanks,

Bart.

Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Christoph Hellwig]

On Wed, Nov 09, 2022 at 09:29:47AM -0800, Bart Van Assche wrote:
> I'm not sure how to interpret your reply. Anyway, this patch is required to
> use the encryption functionality of the UFS Exynos controller. The
> "vendor-specific fields" text in the patch description refers to the
> encryption fields since these follow the data buffer when using the Exynos
> controller. Although it makes me unhappy that the UFS Exynos controller is
> not compliant with the UFS specification, since it is being used widely I
> think we need support for this controller in the upstream kernel.

The fact that in UFS no one sticks to the standard, and not one but
us in the kernel being more strict and your employer sticking to that
can fix it.

But that's not the point here - the point is that such fields are
always implementation specific and never vendor specific.  Any
particular vendor can, and often does, have various different
implementation specific derivations from or extensions to a spec.

Just like there is no 'vendor' driver as there are plenty different
drivers for the same device class from the same manufacturer.

RE: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Avri Altman]

> On Wed, Nov 09, 2022 at 09:29:47AM -0800, Bart Van Assche wrote:
> > I'm not sure how to interpret your reply. Anyway, this patch is
> > required to use the encryption functionality of the UFS Exynos
> > controller. The "vendor-specific fields" text in the patch description
> > refers to the encryption fields since these follow the data buffer
> > when using the Exynos controller. Although it makes me unhappy that
> > the UFS Exynos controller is not compliant with the UFS specification,
> > since it is being used widely I think we need support for this controller in
> the upstream kernel.
> 
> The fact that in UFS no one sticks to the standard, and not one but us in the
> kernel being more strict and your employer sticking to that can fix it.
This part is usually done by hw.
It is usually acceptable to temporarily use quirks while the hw get fixed.

Thanks,
Avri 

> 
> But that's not the point here - the point is that such fields are always
> implementation specific and never vendor specific.  Any particular vendor
> can, and often does, have various different implementation specific
> derivations from or extensions to a spec.
> 
> Just like there is no 'vendor' driver as there are plenty different drivers for
> the same device class from the same manufacturer.

Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Bart Van Assche]

On 11/15/22 00:56, Avri Altman wrote:
> It is usually acceptable to temporarily use quirks while the hw get fixed.

The behavior supported by this patch is not a hardware bug - my 
understanding is that it is behavior that has been chosen on purpose by 
the Exynos design team.

I can convert this patch such that it uses a "quirk" flag instead of a 
kernel config option to enable support for Exynos encryption. However, 
doing that will add runtime overhead in the hot path for all UFS 
vendors, including those that are compliant with the UFSHCI 
specification. Is everyone OK with this?

Thanks,

Bart.

RE: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Avri Altman]

> On 11/15/22 00:56, Avri Altman wrote:
> > It is usually acceptable to temporarily use quirks while the hw get fixed.
> 
> The behavior supported by this patch is not a hardware bug - my understanding
> is that it is behavior that has been chosen on purpose by the Exynos design
> team.
> 
> I can convert this patch such that it uses a "quirk" flag instead of a kernel config
> option to enable support for Exynos encryption. However, doing that will add
> runtime overhead in the hot path for all UFS vendors, including those that are
> compliant with the UFSHCI specification. Is everyone OK with this?
No - I guess not.

Thanks,
Avri
> 
> Thanks,
> 
> Bart.

Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Bart Van Assche]

On 11/14/22 23:48, Christoph Hellwig wrote:
> On Wed, Nov 09, 2022 at 09:29:47AM -0800, Bart Van Assche wrote:
>> I'm not sure how to interpret your reply. Anyway, this patch is required to
>> use the encryption functionality of the UFS Exynos controller. The
>> "vendor-specific fields" text in the patch description refers to the
>> encryption fields since these follow the data buffer when using the Exynos
>> controller. Although it makes me unhappy that the UFS Exynos controller is
>> not compliant with the UFS specification, since it is being used widely I
>> think we need support for this controller in the upstream kernel.
> 
> The fact that in UFS no one sticks to the standard, and not one but
> us in the kernel being more strict and your employer sticking to that
> can fix it.

The above statement is too strong. The Exynos controller is the least 
compliant controller I'm aware of. Future Google Pixel devices will use 
a UFS controller that is compliant with the UFSHCI standard. 
Additionally, the Exynos UFS controller also occurs in devices produced 
by other companies than my employer.

> But that's not the point here - the point is that such fields are
> always implementation specific and never vendor specific.  Any
> particular vendor can, and often does, have various different
> implementation specific derivations from or extensions to a spec.
> 
> Just like there is no 'vendor' driver as there are plenty different
> drivers for the same device class from the same manufacturer.

Do you perhaps want me to change what Eric Biggers suggested into 
something more generic? I'm referring to the following suggestion from Eric:

config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
	bool
	default y if SCSI_UFS_EXYNOS && SCSI_UFS_CRYPTO

Thanks,

Bart.

Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Eric Biggers]

On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote:
> diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
> index 4cc2dbd79ed0..49017abdac92 100644
> --- a/drivers/ufs/host/Kconfig
> +++ b/drivers/ufs/host/Kconfig
> @@ -124,3 +124,13 @@ config SCSI_UFS_EXYNOS
>  
>  	  Select this if you have UFS host controller on Samsung Exynos SoC.
>  	  If unsure, say N.
> +
> +config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
> +	bool "Variable size UTP physical region descriptor"
> +	help
> +	  In the UFSHCI 3.0 standard the Physical Region Descriptor (PRD) is a
> +	  data structure used for transferring data between host and UFS
> +	  device. This data structure describes a single region in physical
> +	  memory. Although the standard requires that this data structure has a
> +	  size of 16 bytes, for some controllers this data structure has a
> +	  different size. Enable this option for UFS controllers that need it.

This shouldn't be a user-selectable option.  Just make it be enabled
automatically when it is needed.  Like this:

config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
	bool
	default y if SCSI_UFS_EXYNOS && SCSI_UFS_CRYPTO

Also, this patch doesn't make sense without the code that actually needs it, so
I hope you plan to send that upstream too.  I haven't been able to do so yet,
because no platform with this hardware actually can run the upstream kernel at
all, as far as I know.  Maybe commit 06874015327 ("arm64: dts: exynos: Add
initial device tree support for Exynos7885 SoC") changed that?  Any suggestions
would be greatly appreciated...  How are you testing this?

> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index bd45818bf0e8..c6854514e40e 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -754,6 +754,9 @@ struct ufs_hba_monitor {
>   * @vops: pointer to variant specific operations
>   * @vps: pointer to variant specific parameters
>   * @priv: pointer to variant specific private data
> +#ifdef CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
> + * @sg_entry_size: size of struct ufshcd_sg_entry (may include variant fields)
> +#endif
>   * @irq: Irq number of the controller

It doesn't make sense to have an #ifdef in the middle of a comment.

- Eric

Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

[Bart Van Assche]

On 11/9/22 10:24, Eric Biggers wrote:
> On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote:
>> +config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
>> +	bool "Variable size UTP physical region descriptor"
>> +	help
>> +	  In the UFSHCI 3.0 standard the Physical Region Descriptor (PRD) is a
>> +	  data structure used for transferring data between host and UFS
>> +	  device. This data structure describes a single region in physical
>> +	  memory. Although the standard requires that this data structure has a
>> +	  size of 16 bytes, for some controllers this data structure has a
>> +	  different size. Enable this option for UFS controllers that need it.
> 
> This shouldn't be a user-selectable option.  Just make it be enabled
> automatically when it is needed.  Like this:
> 
> config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
> 	bool
> 	default y if SCSI_UFS_EXYNOS && SCSI_UFS_CRYPTO

Thanks Eric for having taken a look. I will include the above change in 
this patch when I repost it.

> Also, this patch doesn't make sense without the code that actually needs it, so
> I hope you plan to send that upstream too.  I haven't been able to do so yet,
> because no platform with this hardware actually can run the upstream kernel at
> all, as far as I know.  Maybe commit 06874015327 ("arm64: dts: exynos: Add
> initial device tree support for Exynos7885 SoC") changed that?  Any suggestions
> would be greatly appreciated...  How are you testing this?

My approach for maintaining the UFS driver in the Android kernel tree is 
to keep the diffs between the upstream UFS driver and the driver in the 
Android kernel tree as small as possible. This patch has been tested in 
the same way as my other UFS driver patches, namely by applying it on 
the Android kernel tree and by testing the Android kernel tree. For this 
patch in particular "applying" came down to integrating the changes from 
this patch that are not yet in the Android kernel tree.

Thanks,

Bart.