[PATCH] scsi: ufs: Consider device limitations for dma_mask

[PATCH] scsi: ufs: Consider device limitations for dma_mask

[Bjorn Andersson]

On Qualcomm SDM845 the capabilities of the UFS MEM controller states
that it's capable of dealing with 64 bit addresses, but DMA addresses
are truncated causing IOMMU faults when trying to issue operations.

Limit the DMA mask to that of the device, so that DMA allocations
is limited to the range supported by the bus and device and not just
following what the controller's capabilities states.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/scsi/ufs/ufshcd.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9ba7671b84f8..dc0eb59dd46f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8151,11 +8151,14 @@ EXPORT_SYMBOL_GPL(ufshcd_dealloc_host);
  */
 static int ufshcd_set_dma_mask(struct ufs_hba *hba)
 {
-	if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
-		if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
-			return 0;
-	}
-	return dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(32));
+	u64 dma_mask = dma_get_mask(hba->dev);
+
+	if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT)
+		dma_mask &= DMA_BIT_MASK(64);
+	else
+		dma_mask &= DMA_BIT_MASK(32);
+
+	return dma_set_mask_and_coherent(hba->dev, dma_mask);
 }
 
 /**
-- 
2.18.0

Re: [PATCH] scsi: ufs: Consider device limitations for dma_mask

[Doug Anderson]

Hi,

On Fri, Jan 11, 2019 at 2:54 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Qualcomm SDM845 the capabilities of the UFS MEM controller states
> that it's capable of dealing with 64 bit addresses, but DMA addresses
> are truncated causing IOMMU faults when trying to issue operations.
>
> Limit the DMA mask to that of the device, so that DMA allocations
> is limited to the range supported by the bus and device and not just
> following what the controller's capabilities states.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9ba7671b84f8..dc0eb59dd46f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8151,11 +8151,14 @@ EXPORT_SYMBOL_GPL(ufshcd_dealloc_host);
>   */
>  static int ufshcd_set_dma_mask(struct ufs_hba *hba)
>  {
> -       if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
> -               if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
> -                       return 0;
> -       }
> -       return dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(32));
> +       u64 dma_mask = dma_get_mask(hba->dev);
> +
> +       if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT)
> +               dma_mask &= DMA_BIT_MASK(64);
> +       else
> +               dma_mask &= DMA_BIT_MASK(32);

Just because I'm annoying like that, I'll point out  that the above is
a bit on the silly side.  Instead I'd do:

if (!(hba->capabilities & MASK_64_ADDRESSING_SUPPORT))
    dma_mask &= DMA_BIT_MASK(32);

AKA: your code is masking a 64-bit variable with a value that is known
to be 0xffffffffffffffff, which is kinda a no-op.


...other than the nit, this seems sane to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH] scsi: ufs: Consider device limitations for dma_mask

[Bjorn Andersson]

On Fri 11 Jan 15:33 PST 2019, Doug Anderson wrote:

> Hi,
> 
> On Fri, Jan 11, 2019 at 2:54 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Qualcomm SDM845 the capabilities of the UFS MEM controller states
> > that it's capable of dealing with 64 bit addresses, but DMA addresses
> > are truncated causing IOMMU faults when trying to issue operations.
> >
> > Limit the DMA mask to that of the device, so that DMA allocations
> > is limited to the range supported by the bus and device and not just
> > following what the controller's capabilities states.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 9ba7671b84f8..dc0eb59dd46f 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8151,11 +8151,14 @@ EXPORT_SYMBOL_GPL(ufshcd_dealloc_host);
> >   */
> >  static int ufshcd_set_dma_mask(struct ufs_hba *hba)
> >  {
> > -       if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
> > -               if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
> > -                       return 0;
> > -       }
> > -       return dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(32));
> > +       u64 dma_mask = dma_get_mask(hba->dev);
> > +
> > +       if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT)
> > +               dma_mask &= DMA_BIT_MASK(64);
> > +       else
> > +               dma_mask &= DMA_BIT_MASK(32);
> 
> Just because I'm annoying like that, I'll point out  that the above is
> a bit on the silly side.  Instead I'd do:
> 
> if (!(hba->capabilities & MASK_64_ADDRESSING_SUPPORT))
>     dma_mask &= DMA_BIT_MASK(32);
> 
> AKA: your code is masking a 64-bit variable with a value that is known
> to be 0xffffffffffffffff, which is kinda a no-op.
> 

You're right, so I took a stab at reworking the patch, but we end up
with something:

	u64 dma_mask;

	if (!(hba->capabilities & MASK_64_ADDRESSING_SUPPORT)) {
		dma_mask = dma_get_mask(hba->dev);
		dma_mash &= DMA_BIT_MASK(32);
		return dma_set_mask_and_coherent(hba->dev, dma_mask);
	}

	return 0;
}

Which makes me feel I need a comment here describing that what happens
in the 64-bit case (i.e. nothing). So I think the proposed form is
clearer, even though the compiler is expected to optimize away one of
the branches.

James, Martin, do you have a preference?

> 
> ...other than the nit, this seems sane to me.
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Douglas Anderson <dianders@chromium.org>

Thanks,
Bjorn

Re: [PATCH] scsi: ufs: Consider device limitations for dma_mask

[Christoph Hellwig]

On Fri, Jan 11, 2019 at 02:54:02PM -0800, Bjorn Andersson wrote:
>   */
>  static int ufshcd_set_dma_mask(struct ufs_hba *hba)
>  {
> -	if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
> -		if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
> -			return 0;
> -	}
> -	return dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(32));
> +	u64 dma_mask = dma_get_mask(hba->dev);
> +
> +	if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT)
> +		dma_mask &= DMA_BIT_MASK(64);
> +	else
> +		dma_mask &= DMA_BIT_MASK(32);
> +
> +	return dma_set_mask_and_coherent(hba->dev, dma_mask);

NAK.  ufshcd clearly is in charge of setting the dma mask, so reading
it back from someone else who might have set it is completely bogus.

You either need to introduce a quirk or a way to communicate the
different limit so that it can be set by the core.

Re: [PATCH] scsi: ufs: Consider device limitations for dma_mask

[Bjorn Andersson]

On Mon 14 Jan 03:11 PST 2019, Christoph Hellwig wrote:

> On Fri, Jan 11, 2019 at 02:54:02PM -0800, Bjorn Andersson wrote:
> >   */
> >  static int ufshcd_set_dma_mask(struct ufs_hba *hba)
> >  {
> > -	if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
> > -		if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
> > -			return 0;
> > -	}
> > -	return dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(32));
> > +	u64 dma_mask = dma_get_mask(hba->dev);
> > +
> > +	if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT)
> > +		dma_mask &= DMA_BIT_MASK(64);
> > +	else
> > +		dma_mask &= DMA_BIT_MASK(32);
> > +
> > +	return dma_set_mask_and_coherent(hba->dev, dma_mask);
> 
> NAK.  ufshcd clearly is in charge of setting the dma mask, so reading
> it back from someone else who might have set it is completely bogus.
> 

The problem here is that the capability bit states that the controller
itself claim to be able to deal with 64-bit addresses, which is probably
true. The thing that the struct device represents (the integrated
controller, on a bus in this SoC) doesn't.

The device model accurately handles this and carries a dma_mask that's
appropriate for the device in this system - the capability is not.

> You either need to introduce a quirk or a way to communicate the
> different limit so that it can be set by the core.

The system's limit is already communicated in hba->dev->dma_mask, but
the ufshcd driver overwrites this. I expect that this would make sense
if the device model claims we can do e.g. 40 bit addressing, but the
64-bit capability is not set in the controller - in which case ufshcd
would accurately lower this to 32-bits.


I'm not sure what to quirk here, but will look into this...

Regards,
Bjorn

Re: [PATCH] scsi: ufs: Consider device limitations for dma_mask

[Christoph Hellwig]

On Mon, Jan 14, 2019 at 09:30:51AM -0800, Bjorn Andersson wrote:
> The problem here is that the capability bit states that the controller
> itself claim to be able to deal with 64-bit addresses, which is probably
> true. The thing that the struct device represents (the integrated
> controller, on a bus in this SoC) doesn't.
> 
> The device model accurately handles this and carries a dma_mask that's
> appropriate for the device in this system - the capability is not.
> 
> > You either need to introduce a quirk or a way to communicate the
> > different limit so that it can be set by the core.
> 
> The system's limit is already communicated in hba->dev->dma_mask, but
> the ufshcd driver overwrites this. I expect that this would make sense
> if the device model claims we can do e.g. 40 bit addressing, but the
> 64-bit capability is not set in the controller - in which case ufshcd
> would accurately lower this to 32-bits.

No, that is absolutely not true.  dev->dma_mask is set by the driver
to what the driver based on the device specsheet/register claims to
support.  dev->bus_dma_mask contains any additional limits imposed
by the bus/system, but that is handled transparently by the dma mapping
code.

Re: [PATCH] scsi: ufs: Consider device limitations for dma_mask

[Bjorn Andersson]

On Mon 14 Jan 09:36 PST 2019, Christoph Hellwig wrote:

> On Mon, Jan 14, 2019 at 09:30:51AM -0800, Bjorn Andersson wrote:
> > The problem here is that the capability bit states that the controller
> > itself claim to be able to deal with 64-bit addresses, which is probably
> > true. The thing that the struct device represents (the integrated
> > controller, on a bus in this SoC) doesn't.
> > 
> > The device model accurately handles this and carries a dma_mask that's
> > appropriate for the device in this system - the capability is not.
> > 
> > > You either need to introduce a quirk or a way to communicate the
> > > different limit so that it can be set by the core.
> > 
> > The system's limit is already communicated in hba->dev->dma_mask, but
> > the ufshcd driver overwrites this. I expect that this would make sense
> > if the device model claims we can do e.g. 40 bit addressing, but the
> > 64-bit capability is not set in the controller - in which case ufshcd
> > would accurately lower this to 32-bits.
> 
> No, that is absolutely not true.  dev->dma_mask is set by the driver
> to what the driver based on the device specsheet/register claims to
> support.  dev->bus_dma_mask contains any additional limits imposed
> by the bus/system, but that is handled transparently by the dma mapping
> code.

You're right and I see now that my bus_dma_mask is not set properly and
is the cause of the problem.

Thanks for correcting me, I fully agree with your NACK now.

Regards,
Bjorn