[PATCH] scsi: ufs: core: Limit DMA alignment check

[PATCH] scsi: ufs: core: Limit DMA alignment check

[Bjorn Andersson]

The three DMA memory regions allocated for the host memory space is
documented to require alignment of 128, 1024 and 1024 respectively, but
the returned address is checked for PAGE_SIZE alignment.

In the case these allocations are serviced by e.g. the Arm SMMU, the
size and alignment will be determined by its supported page sizes. In
most cases SZ_4K and a few larger sizes are available.

In the typical configuration this does not cause problems, but in the
event that the system PAGE_SIZE is increased beyond 4k, it's no longer
reasonable to expect that the allocation will be PAGE_SIZE aligned.

Limit the DMA alignment check to the actual alignment requirements
written in the comments in the code, to avoid the UFS core refusing to
initialize with such configuration.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/ufs/core/ufshcd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ec732e4bbbf4..d7f3f1ba9d12 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3724,12 +3724,9 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
 
 	/*
 	 * UFSHCI requires UTP command descriptor to be 128 byte aligned.
-	 * make sure hba->ucdl_dma_addr is aligned to PAGE_SIZE
-	 * if hba->ucdl_dma_addr is aligned to PAGE_SIZE, then it will
-	 * be aligned to 128 bytes as well
 	 */
 	if (!hba->ucdl_base_addr ||
-	    WARN_ON(hba->ucdl_dma_addr & (PAGE_SIZE - 1))) {
+	    WARN_ON(hba->ucdl_dma_addr & (128 - 1))) {
 		dev_err(hba->dev,
 			"Command Descriptor Memory allocation failed\n");
 		goto out;
@@ -3745,7 +3742,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
 						   &hba->utrdl_dma_addr,
 						   GFP_KERNEL);
 	if (!hba->utrdl_base_addr ||
-	    WARN_ON(hba->utrdl_dma_addr & (PAGE_SIZE - 1))) {
+	    WARN_ON(hba->utrdl_dma_addr & (1024 - 1))) {
 		dev_err(hba->dev,
 			"Transfer Descriptor Memory allocation failed\n");
 		goto out;
@@ -3769,7 +3766,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
 						    &hba->utmrdl_dma_addr,
 						    GFP_KERNEL);
 	if (!hba->utmrdl_base_addr ||
-	    WARN_ON(hba->utmrdl_dma_addr & (PAGE_SIZE - 1))) {
+	    WARN_ON(hba->utmrdl_dma_addr & (1024 - 1))) {
 		dev_err(hba->dev,
 		"Task Management Descriptor Memory allocation failed\n");
 		goto out;
-- 
2.25.1

Re: [PATCH] scsi: ufs: core: Limit DMA alignment check

[Manivannan Sadhasivam]

On Tue, Jan 31, 2023 at 07:49:17PM -0800, Bjorn Andersson wrote:
> The three DMA memory regions allocated for the host memory space is
> documented to require alignment of 128, 1024 and 1024 respectively, but
> the returned address is checked for PAGE_SIZE alignment.
> 
> In the case these allocations are serviced by e.g. the Arm SMMU, the
> size and alignment will be determined by its supported page sizes. In
> most cases SZ_4K and a few larger sizes are available.
> 
> In the typical configuration this does not cause problems, but in the
> event that the system PAGE_SIZE is increased beyond 4k, it's no longer
> reasonable to expect that the allocation will be PAGE_SIZE aligned.
> 
> Limit the DMA alignment check to the actual alignment requirements
> written in the comments in the code, to avoid the UFS core refusing to
> initialize with such configuration.

Isn't dma_alloc_coherent() supposed to return PAGE_SIZE aligned dma and cpu
addresses? I suppose that could be reason for checking against PAGE_SIZE.

> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

But it doesn't hurt to check for the actual alignment.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/ufs/core/ufshcd.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ec732e4bbbf4..d7f3f1ba9d12 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3724,12 +3724,9 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
>  
>  	/*
>  	 * UFSHCI requires UTP command descriptor to be 128 byte aligned.
> -	 * make sure hba->ucdl_dma_addr is aligned to PAGE_SIZE
> -	 * if hba->ucdl_dma_addr is aligned to PAGE_SIZE, then it will
> -	 * be aligned to 128 bytes as well
>  	 */
>  	if (!hba->ucdl_base_addr ||
> -	    WARN_ON(hba->ucdl_dma_addr & (PAGE_SIZE - 1))) {
> +	    WARN_ON(hba->ucdl_dma_addr & (128 - 1))) {
>  		dev_err(hba->dev,
>  			"Command Descriptor Memory allocation failed\n");
>  		goto out;
> @@ -3745,7 +3742,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
>  						   &hba->utrdl_dma_addr,
>  						   GFP_KERNEL);
>  	if (!hba->utrdl_base_addr ||
> -	    WARN_ON(hba->utrdl_dma_addr & (PAGE_SIZE - 1))) {
> +	    WARN_ON(hba->utrdl_dma_addr & (1024 - 1))) {
>  		dev_err(hba->dev,
>  			"Transfer Descriptor Memory allocation failed\n");
>  		goto out;
> @@ -3769,7 +3766,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
>  						    &hba->utmrdl_dma_addr,
>  						    GFP_KERNEL);
>  	if (!hba->utmrdl_base_addr ||
> -	    WARN_ON(hba->utmrdl_dma_addr & (PAGE_SIZE - 1))) {
> +	    WARN_ON(hba->utmrdl_dma_addr & (1024 - 1))) {
>  		dev_err(hba->dev,
>  		"Task Management Descriptor Memory allocation failed\n");
>  		goto out;
> -- 
> 2.25.1
> 

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

Re: [PATCH] scsi: ufs: core: Limit DMA alignment check

[Bjorn Andersson]

On Wed, Feb 01, 2023 at 03:57:48PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jan 31, 2023 at 07:49:17PM -0800, Bjorn Andersson wrote:
> > The three DMA memory regions allocated for the host memory space is
> > documented to require alignment of 128, 1024 and 1024 respectively, but
> > the returned address is checked for PAGE_SIZE alignment.
> > 
> > In the case these allocations are serviced by e.g. the Arm SMMU, the
> > size and alignment will be determined by its supported page sizes. In
> > most cases SZ_4K and a few larger sizes are available.
> > 
> > In the typical configuration this does not cause problems, but in the
> > event that the system PAGE_SIZE is increased beyond 4k, it's no longer
> > reasonable to expect that the allocation will be PAGE_SIZE aligned.
> > 
> > Limit the DMA alignment check to the actual alignment requirements
> > written in the comments in the code, to avoid the UFS core refusing to
> > initialize with such configuration.
> 
> Isn't dma_alloc_coherent() supposed to return PAGE_SIZE aligned dma and cpu
> addresses? I suppose that could be reason for checking against PAGE_SIZE.
> 

If the allocating device has associated dma_mem, the requested size will
be rounded up to the next order and you will get an allocation which is
sized and aligned to that.
Given the three allocations on my device is 98304, 1024 and 640. The
latter two would only be aligned to 1kb if this code path is taken...


But in our case, the comment for __iommu_dma_alloc_noncontiguous() seems
to give us the reason for the issue.

/*
 * If size is less than PAGE_SIZE, then a full CPU page will be allocated,
 * but an IOMMU which supports smaller pages might not map the whole thing.
 */

Our iommu reports supporting mapping of 4kb pages.

> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> 
> But it doesn't hurt to check for the actual alignment.
> 
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> 

Thanks,
Bjorn

> Thanks,
> Mani
> 
> > ---
> >  drivers/ufs/core/ufshcd.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index ec732e4bbbf4..d7f3f1ba9d12 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -3724,12 +3724,9 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> >  
> >  	/*
> >  	 * UFSHCI requires UTP command descriptor to be 128 byte aligned.
> > -	 * make sure hba->ucdl_dma_addr is aligned to PAGE_SIZE
> > -	 * if hba->ucdl_dma_addr is aligned to PAGE_SIZE, then it will
> > -	 * be aligned to 128 bytes as well
> >  	 */
> >  	if (!hba->ucdl_base_addr ||
> > -	    WARN_ON(hba->ucdl_dma_addr & (PAGE_SIZE - 1))) {
> > +	    WARN_ON(hba->ucdl_dma_addr & (128 - 1))) {
> >  		dev_err(hba->dev,
> >  			"Command Descriptor Memory allocation failed\n");
> >  		goto out;
> > @@ -3745,7 +3742,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> >  						   &hba->utrdl_dma_addr,
> >  						   GFP_KERNEL);
> >  	if (!hba->utrdl_base_addr ||
> > -	    WARN_ON(hba->utrdl_dma_addr & (PAGE_SIZE - 1))) {
> > +	    WARN_ON(hba->utrdl_dma_addr & (1024 - 1))) {
> >  		dev_err(hba->dev,
> >  			"Transfer Descriptor Memory allocation failed\n");
> >  		goto out;
> > @@ -3769,7 +3766,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> >  						    &hba->utmrdl_dma_addr,
> >  						    GFP_KERNEL);
> >  	if (!hba->utmrdl_base_addr ||
> > -	    WARN_ON(hba->utmrdl_dma_addr & (PAGE_SIZE - 1))) {
> > +	    WARN_ON(hba->utmrdl_dma_addr & (1024 - 1))) {
> >  		dev_err(hba->dev,
> >  		"Task Management Descriptor Memory allocation failed\n");
> >  		goto out;
> > -- 
> > 2.25.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] scsi: ufs: core: Limit DMA alignment check

[Bart Van Assche]

On 1/31/23 19:49, Bjorn Andersson wrote:
> The three DMA memory regions allocated for the host memory space is
> documented to require alignment of 128, 1024 and 1024 respectively, but
> the returned address is checked for PAGE_SIZE alignment.
> 
> In the case these allocations are serviced by e.g. the Arm SMMU, the
> size and alignment will be determined by its supported page sizes. In
> most cases SZ_4K and a few larger sizes are available.
> 
> In the typical configuration this does not cause problems, but in the
> event that the system PAGE_SIZE is increased beyond 4k, it's no longer
> reasonable to expect that the allocation will be PAGE_SIZE aligned.
> 
> Limit the DMA alignment check to the actual alignment requirements
> written in the comments in the code, to avoid the UFS core refusing to
> initialize with such configuration.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] scsi: ufs: core: Limit DMA alignment check

[Asutosh Das]

On Tue, Jan 31 2023 at 19:49 -0800, Bjorn Andersson wrote:
>The three DMA memory regions allocated for the host memory space is
>documented to require alignment of 128, 1024 and 1024 respectively, but
>the returned address is checked for PAGE_SIZE alignment.
>
>In the case these allocations are serviced by e.g. the Arm SMMU, the
>size and alignment will be determined by its supported page sizes. In
>most cases SZ_4K and a few larger sizes are available.
>
>In the typical configuration this does not cause problems, but in the
>event that the system PAGE_SIZE is increased beyond 4k, it's no longer
>reasonable to expect that the allocation will be PAGE_SIZE aligned.
>
>Limit the DMA alignment check to the actual alignment requirements
>written in the comments in the code, to avoid the UFS core refusing to
>initialize with such configuration.
>
>Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

Reviewed-by: Asutosh Das <quic_asutoshd@quicinc.com>

>---
> drivers/ufs/core/ufshcd.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>index ec732e4bbbf4..d7f3f1ba9d12 100644
>--- a/drivers/ufs/core/ufshcd.c
>+++ b/drivers/ufs/core/ufshcd.c
>@@ -3724,12 +3724,9 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
>
> 	/*
> 	 * UFSHCI requires UTP command descriptor to be 128 byte aligned.
>-	 * make sure hba->ucdl_dma_addr is aligned to PAGE_SIZE
>-	 * if hba->ucdl_dma_addr is aligned to PAGE_SIZE, then it will
>-	 * be aligned to 128 bytes as well
> 	 */
> 	if (!hba->ucdl_base_addr ||
>-	    WARN_ON(hba->ucdl_dma_addr & (PAGE_SIZE - 1))) {
>+	    WARN_ON(hba->ucdl_dma_addr & (128 - 1))) {
> 		dev_err(hba->dev,
> 			"Command Descriptor Memory allocation failed\n");
> 		goto out;
>@@ -3745,7 +3742,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> 						   &hba->utrdl_dma_addr,
> 						   GFP_KERNEL);
> 	if (!hba->utrdl_base_addr ||
>-	    WARN_ON(hba->utrdl_dma_addr & (PAGE_SIZE - 1))) {
>+	    WARN_ON(hba->utrdl_dma_addr & (1024 - 1))) {
> 		dev_err(hba->dev,
> 			"Transfer Descriptor Memory allocation failed\n");
> 		goto out;
>@@ -3769,7 +3766,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> 						    &hba->utmrdl_dma_addr,
> 						    GFP_KERNEL);
> 	if (!hba->utmrdl_base_addr ||
>-	    WARN_ON(hba->utmrdl_dma_addr & (PAGE_SIZE - 1))) {
>+	    WARN_ON(hba->utmrdl_dma_addr & (1024 - 1))) {
> 		dev_err(hba->dev,
> 		"Task Management Descriptor Memory allocation failed\n");
> 		goto out;
>-- 
>2.25.1
>

Re: [PATCH] scsi: ufs: core: Limit DMA alignment check

[Manivannan Sadhasivam]

On Wed, Feb 01, 2023 at 08:28:41AM -0800, Bjorn Andersson wrote:
> On Wed, Feb 01, 2023 at 03:57:48PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jan 31, 2023 at 07:49:17PM -0800, Bjorn Andersson wrote:
> > > The three DMA memory regions allocated for the host memory space is
> > > documented to require alignment of 128, 1024 and 1024 respectively, but
> > > the returned address is checked for PAGE_SIZE alignment.
> > > 
> > > In the case these allocations are serviced by e.g. the Arm SMMU, the
> > > size and alignment will be determined by its supported page sizes. In
> > > most cases SZ_4K and a few larger sizes are available.
> > > 
> > > In the typical configuration this does not cause problems, but in the
> > > event that the system PAGE_SIZE is increased beyond 4k, it's no longer
> > > reasonable to expect that the allocation will be PAGE_SIZE aligned.
> > > 
> > > Limit the DMA alignment check to the actual alignment requirements
> > > written in the comments in the code, to avoid the UFS core refusing to
> > > initialize with such configuration.
> > 
> > Isn't dma_alloc_coherent() supposed to return PAGE_SIZE aligned dma and cpu
> > addresses? I suppose that could be reason for checking against PAGE_SIZE.
> > 
> 
> If the allocating device has associated dma_mem, the requested size will
> be rounded up to the next order and you will get an allocation which is
> sized and aligned to that.
> Given the three allocations on my device is 98304, 1024 and 640. The
> latter two would only be aligned to 1kb if this code path is taken...
> 
> 
> But in our case, the comment for __iommu_dma_alloc_noncontiguous() seems
> to give us the reason for the issue.
> 
> /*
>  * If size is less than PAGE_SIZE, then a full CPU page will be allocated,
>  * but an IOMMU which supports smaller pages might not map the whole thing.
>  */
> 

Ah, missed this part. Thanks for enlightening me :)

- Mani

> Our iommu reports supporting mapping of 4kb pages.
> 
> > > 
> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > 
> > But it doesn't hurt to check for the actual alignment.
> > 
> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> > 
> 
> Thanks,
> Bjorn
> 
> > Thanks,
> > Mani
> > 
> > > ---
> > >  drivers/ufs/core/ufshcd.c | 9 +++------
> > >  1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > > index ec732e4bbbf4..d7f3f1ba9d12 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -3724,12 +3724,9 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> > >  
> > >  	/*
> > >  	 * UFSHCI requires UTP command descriptor to be 128 byte aligned.
> > > -	 * make sure hba->ucdl_dma_addr is aligned to PAGE_SIZE
> > > -	 * if hba->ucdl_dma_addr is aligned to PAGE_SIZE, then it will
> > > -	 * be aligned to 128 bytes as well
> > >  	 */
> > >  	if (!hba->ucdl_base_addr ||
> > > -	    WARN_ON(hba->ucdl_dma_addr & (PAGE_SIZE - 1))) {
> > > +	    WARN_ON(hba->ucdl_dma_addr & (128 - 1))) {
> > >  		dev_err(hba->dev,
> > >  			"Command Descriptor Memory allocation failed\n");
> > >  		goto out;
> > > @@ -3745,7 +3742,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> > >  						   &hba->utrdl_dma_addr,
> > >  						   GFP_KERNEL);
> > >  	if (!hba->utrdl_base_addr ||
> > > -	    WARN_ON(hba->utrdl_dma_addr & (PAGE_SIZE - 1))) {
> > > +	    WARN_ON(hba->utrdl_dma_addr & (1024 - 1))) {
> > >  		dev_err(hba->dev,
> > >  			"Transfer Descriptor Memory allocation failed\n");
> > >  		goto out;
> > > @@ -3769,7 +3766,7 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
> > >  						    &hba->utmrdl_dma_addr,
> > >  						    GFP_KERNEL);
> > >  	if (!hba->utmrdl_base_addr ||
> > > -	    WARN_ON(hba->utmrdl_dma_addr & (PAGE_SIZE - 1))) {
> > > +	    WARN_ON(hba->utmrdl_dma_addr & (1024 - 1))) {
> > >  		dev_err(hba->dev,
> > >  		"Task Management Descriptor Memory allocation failed\n");
> > >  		goto out;
> > > -- 
> > > 2.25.1
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

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

Re: [PATCH] scsi: ufs: core: Limit DMA alignment check

[Martin K. Petersen]

Bjorn,

> The three DMA memory regions allocated for the host memory space is
> documented to require alignment of 128, 1024 and 1024 respectively,
> but the returned address is checked for PAGE_SIZE alignment.

Applied to 6.3/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering