| Message ID | 20190111225402.6133-1-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: ufs: Consider device limitations for dma_mask | expand |
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>
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
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.
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
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.
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
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); } /**
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(-)