diff mbox series

[v2,3/9] mmc: cqhci: initialize upper 64 bits of 128-bit task descriptors

Message ID 20201203020516.225701-4-ebiggers@kernel.org (mailing list archive)
State Superseded
Headers show
Series eMMC inline encryption support | expand

Commit Message

Eric Biggers Dec. 3, 2020, 2:05 a.m. UTC 
From: Eric Biggers <ebiggers@google.com>

Move the task descriptor initialization into cqhci_prep_task_desc(), and
make it initialize all 128 bits of the task descriptor if the host
controller is using 128-bit task descriptors.

This is needed to prepare for CQHCI inline encryption support, which
requires 128-bit task descriptors and uses the upper 64 bits.

Note: since some host controllers already enable 128-bit task
descriptors, it's unclear why the previous code worked when it wasn't
initializing the upper 64 bits.  One possibility is that the bits are
being ignored because the features that use them aren't enabled yet.
In any case, setting them to 0 won't hurt.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/mmc/host/cqhci-core.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

Comments

Adrian Hunter Dec. 3, 2020, 6:45 a.m. UTC | #1 
On 3/12/20 4:05 am, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Move the task descriptor initialization into cqhci_prep_task_desc(), and
> make it initialize all 128 bits of the task descriptor if the host
> controller is using 128-bit task descriptors.
> 
> This is needed to prepare for CQHCI inline encryption support, which
> requires 128-bit task descriptors and uses the upper 64 bits.
> 
> Note: since some host controllers already enable 128-bit task
> descriptors, it's unclear why the previous code worked when it wasn't
> initializing the upper 64 bits.  One possibility is that the bits are
> being ignored because the features that use them aren't enabled yet.
> In any case, setting them to 0 won't hurt.

Coherent allocations are zero-initialized.  So the upper 64-bits stay zero.
People set 128-bit anyway because the hardware needs it.

> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/cqhci-core.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
> index 697fe40756bf2..ad7c9acff1728 100644
> --- a/drivers/mmc/host/cqhci-core.c
> +++ b/drivers/mmc/host/cqhci-core.c
> @@ -408,13 +408,15 @@ static void cqhci_disable(struct mmc_host *mmc)
>  }
>  
>  static void cqhci_prep_task_desc(struct mmc_request *mrq,
> -					u64 *data, bool intr)
> +				 struct cqhci_host *cq_host, int tag)
>  {
> +	__le64 *task_desc = (__le64 __force *)get_desc(cq_host, tag);
>  	u32 req_flags = mrq->data->flags;
> +	u64 desc0;
>  
> -	*data = CQHCI_VALID(1) |
> +	desc0 = CQHCI_VALID(1) |
>  		CQHCI_END(1) |
> -		CQHCI_INT(intr) |
> +		CQHCI_INT(1) |
>  		CQHCI_ACT(0x5) |
>  		CQHCI_FORCED_PROG(!!(req_flags & MMC_DATA_FORCED_PRG)) |
>  		CQHCI_DATA_TAG(!!(req_flags & MMC_DATA_DAT_TAG)) |
> @@ -425,8 +427,19 @@ static void cqhci_prep_task_desc(struct mmc_request *mrq,
>  		CQHCI_BLK_COUNT(mrq->data->blocks) |
>  		CQHCI_BLK_ADDR((u64)mrq->data->blk_addr);
>  
> -	pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx\n",
> -		 mmc_hostname(mrq->host), mrq->tag, (unsigned long long)*data);
> +	task_desc[0] = cpu_to_le64(desc0);
> +
> +	if (cq_host->caps & CQHCI_TASK_DESC_SZ_128) {
> +		u64 desc1 = 0;
> +
> +		task_desc[1] = cpu_to_le64(desc1);
> +
> +		pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx%016llx\n",
> +			 mmc_hostname(mrq->host), mrq->tag, desc1, desc0);
> +	} else {
> +		pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx\n",
> +			 mmc_hostname(mrq->host), mrq->tag, desc0);
> +	}
>  }
>  
>  static int cqhci_dma_map(struct mmc_host *host, struct mmc_request *mrq)
> @@ -567,8 +580,6 @@ static inline int cqhci_tag(struct mmc_request *mrq)
>  static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  {
>  	int err = 0;
> -	u64 data = 0;
> -	u64 *task_desc = NULL;
>  	int tag = cqhci_tag(mrq);
>  	struct cqhci_host *cq_host = mmc->cqe_private;
>  	unsigned long flags;
> @@ -598,9 +609,8 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  	}
>  
>  	if (mrq->data) {
> -		task_desc = (__le64 __force *)get_desc(cq_host, tag);
> -		cqhci_prep_task_desc(mrq, &data, 1);
> -		*task_desc = cpu_to_le64(data);
> +		cqhci_prep_task_desc(mrq, cq_host, tag);
> +
>  		err = cqhci_prep_tran_desc(mrq, cq_host, tag);
>  		if (err) {
>  			pr_err("%s: cqhci: failed to setup tx desc: %d\n",
>
Eric Biggers Dec. 3, 2020, 7:23 p.m. UTC | #2 
On Thu, Dec 03, 2020 at 08:45:15AM +0200, Adrian Hunter wrote:
> On 3/12/20 4:05 am, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Move the task descriptor initialization into cqhci_prep_task_desc(), and
> > make it initialize all 128 bits of the task descriptor if the host
> > controller is using 128-bit task descriptors.
> > 
> > This is needed to prepare for CQHCI inline encryption support, which
> > requires 128-bit task descriptors and uses the upper 64 bits.
> > 
> > Note: since some host controllers already enable 128-bit task
> > descriptors, it's unclear why the previous code worked when it wasn't
> > initializing the upper 64 bits.  One possibility is that the bits are
> > being ignored because the features that use them aren't enabled yet.
> > In any case, setting them to 0 won't hurt.
> 
> Coherent allocations are zero-initialized.  So the upper 64-bits stay zero.
> People set 128-bit anyway because the hardware needs it.

Okay, that explains it then -- I didn't realize that dma_alloc_coherent() always
returns zeroed memory.  It isn't mentioned in
Documentation/core-api/dma-api.rst, and there's no kerneldoc comment, so it
wasn't clear.  But apparently it's intentional; see commit 518a2f1925c3
("dma-mapping: zero memory returned from dma_alloc_*").

I'll fix this commit message in the next version.

- Eric
Satya Tangirala Dec. 8, 2020, 11:45 p.m. UTC | #3 
On Wed, Dec 02, 2020 at 06:05:10PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Move the task descriptor initialization into cqhci_prep_task_desc(), and
> make it initialize all 128 bits of the task descriptor if the host
> controller is using 128-bit task descriptors.
> 
> This is needed to prepare for CQHCI inline encryption support, which
> requires 128-bit task descriptors and uses the upper 64 bits.
> 
> Note: since some host controllers already enable 128-bit task
> descriptors, it's unclear why the previous code worked when it wasn't
> initializing the upper 64 bits.  One possibility is that the bits are
> being ignored because the features that use them aren't enabled yet.
> In any case, setting them to 0 won't hurt.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  drivers/mmc/host/cqhci-core.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
> index 697fe40756bf2..ad7c9acff1728 100644
> --- a/drivers/mmc/host/cqhci-core.c
> +++ b/drivers/mmc/host/cqhci-core.c
> @@ -408,13 +408,15 @@ static void cqhci_disable(struct mmc_host *mmc)
>  }
>  
>  static void cqhci_prep_task_desc(struct mmc_request *mrq,
> -					u64 *data, bool intr)
> +				 struct cqhci_host *cq_host, int tag)
>  {
> +	__le64 *task_desc = (__le64 __force *)get_desc(cq_host, tag);
>  	u32 req_flags = mrq->data->flags;
> +	u64 desc0;
>  
> -	*data = CQHCI_VALID(1) |
> +	desc0 = CQHCI_VALID(1) |
>  		CQHCI_END(1) |
> -		CQHCI_INT(intr) |
> +		CQHCI_INT(1) |
>  		CQHCI_ACT(0x5) |
>  		CQHCI_FORCED_PROG(!!(req_flags & MMC_DATA_FORCED_PRG)) |
>  		CQHCI_DATA_TAG(!!(req_flags & MMC_DATA_DAT_TAG)) |
> @@ -425,8 +427,19 @@ static void cqhci_prep_task_desc(struct mmc_request *mrq,
>  		CQHCI_BLK_COUNT(mrq->data->blocks) |
>  		CQHCI_BLK_ADDR((u64)mrq->data->blk_addr);
>  
> -	pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx\n",
> -		 mmc_hostname(mrq->host), mrq->tag, (unsigned long long)*data);
> +	task_desc[0] = cpu_to_le64(desc0);
> +
> +	if (cq_host->caps & CQHCI_TASK_DESC_SZ_128) {
> +		u64 desc1 = 0;
> +
> +		task_desc[1] = cpu_to_le64(desc1);
> +
> +		pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx%016llx\n",
> +			 mmc_hostname(mrq->host), mrq->tag, desc1, desc0);
> +	} else {
> +		pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx\n",
> +			 mmc_hostname(mrq->host), mrq->tag, desc0);
> +	}
>  }
>  
>  static int cqhci_dma_map(struct mmc_host *host, struct mmc_request *mrq)
> @@ -567,8 +580,6 @@ static inline int cqhci_tag(struct mmc_request *mrq)
>  static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  {
>  	int err = 0;
> -	u64 data = 0;
> -	u64 *task_desc = NULL;
>  	int tag = cqhci_tag(mrq);
>  	struct cqhci_host *cq_host = mmc->cqe_private;
>  	unsigned long flags;
> @@ -598,9 +609,8 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  	}
>  
>  	if (mrq->data) {
> -		task_desc = (__le64 __force *)get_desc(cq_host, tag);
> -		cqhci_prep_task_desc(mrq, &data, 1);
> -		*task_desc = cpu_to_le64(data);
> +		cqhci_prep_task_desc(mrq, cq_host, tag);
> +
>  		err = cqhci_prep_tran_desc(mrq, cq_host, tag);
>  		if (err) {
>  			pr_err("%s: cqhci: failed to setup tx desc: %d\n",
> -- 
> 2.29.2
> 
Looks good to me. Please feel free to add
Reviewed-by: Satya Tangirala <satyat@google.com>
diff mbox series

Patch

diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
index 697fe40756bf2..ad7c9acff1728 100644
--- a/drivers/mmc/host/cqhci-core.c
+++ b/drivers/mmc/host/cqhci-core.c
@@ -408,13 +408,15 @@  static void cqhci_disable(struct mmc_host *mmc)
 }
 
 static void cqhci_prep_task_desc(struct mmc_request *mrq,
-					u64 *data, bool intr)
+				 struct cqhci_host *cq_host, int tag)
 {
+	__le64 *task_desc = (__le64 __force *)get_desc(cq_host, tag);
 	u32 req_flags = mrq->data->flags;
+	u64 desc0;
 
-	*data = CQHCI_VALID(1) |
+	desc0 = CQHCI_VALID(1) |
 		CQHCI_END(1) |
-		CQHCI_INT(intr) |
+		CQHCI_INT(1) |
 		CQHCI_ACT(0x5) |
 		CQHCI_FORCED_PROG(!!(req_flags & MMC_DATA_FORCED_PRG)) |
 		CQHCI_DATA_TAG(!!(req_flags & MMC_DATA_DAT_TAG)) |
@@ -425,8 +427,19 @@  static void cqhci_prep_task_desc(struct mmc_request *mrq,
 		CQHCI_BLK_COUNT(mrq->data->blocks) |
 		CQHCI_BLK_ADDR((u64)mrq->data->blk_addr);
 
-	pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx\n",
-		 mmc_hostname(mrq->host), mrq->tag, (unsigned long long)*data);
+	task_desc[0] = cpu_to_le64(desc0);
+
+	if (cq_host->caps & CQHCI_TASK_DESC_SZ_128) {
+		u64 desc1 = 0;
+
+		task_desc[1] = cpu_to_le64(desc1);
+
+		pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx%016llx\n",
+			 mmc_hostname(mrq->host), mrq->tag, desc1, desc0);
+	} else {
+		pr_debug("%s: cqhci: tag %d task descriptor 0x%016llx\n",
+			 mmc_hostname(mrq->host), mrq->tag, desc0);
+	}
 }
 
 static int cqhci_dma_map(struct mmc_host *host, struct mmc_request *mrq)
@@ -567,8 +580,6 @@  static inline int cqhci_tag(struct mmc_request *mrq)
 static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
 	int err = 0;
-	u64 data = 0;
-	u64 *task_desc = NULL;
 	int tag = cqhci_tag(mrq);
 	struct cqhci_host *cq_host = mmc->cqe_private;
 	unsigned long flags;
@@ -598,9 +609,8 @@  static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	}
 
 	if (mrq->data) {
-		task_desc = (__le64 __force *)get_desc(cq_host, tag);
-		cqhci_prep_task_desc(mrq, &data, 1);
-		*task_desc = cpu_to_le64(data);
+		cqhci_prep_task_desc(mrq, cq_host, tag);
+
 		err = cqhci_prep_tran_desc(mrq, cq_host, tag);
 		if (err) {
 			pr_err("%s: cqhci: failed to setup tx desc: %d\n",