On Mon, Sep 23, 2019 at 09:41:34AM +0100, Russell King - ARM Linux admin wrote: > On Mon, Sep 23, 2019 at 02:51:15AM +0000, Y.b. Lu wrote: > > Hi Russell, > > > > > -----Original Message----- > > > From: Russell King <rmk@xxxxxxxxxxxxxxx> On Behalf Of Russell King > > > Sent: Sunday, September 22, 2019 6:27 PM > > > To: Robin Murphy <robin.murphy@xxxxxxx>; dann frazier > > > <dann.frazier@xxxxxxxxxxxxx>; Will Deacon <will.deacon@xxxxxxx>; Nicolin > > > Chen <nicoleotsuka@xxxxxxxxx>; Y.b. Lu <yangbo.lu@xxxxxxx>; Christoph > > > Hellwig <hch@xxxxxx> > > > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>; Ulf Hansson > > > <ulf.hansson@xxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx > > > Subject: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA > > > coherence > > > > > > We must not unconditionally set the DMA snoop bit; if the DMA API is > > > assuming that the device is not DMA coherent, and the device snoops the CPU > > > caches, the device can see stale cache lines brought in by speculative prefetch. > > > > > > This leads to the device seeing stale data, potentially resulting in corrupted > > > data transfers. Commonly, this results in a descriptor fetch error such as: > > > > > > mmc0: ADMA error > > > mmc0: sdhci: ============ SDHCI REGISTER DUMP =========== > > > mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202 > > > mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001 > > > mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013 > > > mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038 > > > mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000 > > > mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8 > > > mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001 > > > mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b > > > mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202 > > > mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00 > > > mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000 > > > mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33 > > > mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00 > > > mmc0: sdhci: Host ctl2: 0x00000000 > > > mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c > > > mmc0: sdhci: ============================================ > > > mmc0: error -5 whilst initialising SD card > > > > > > but can lead to other errors, and potentially direct the SDHCI controller to > > > read/write data to other memory locations (e.g. if a valid descriptor is visible > > > to the device in a stale cache line.) > > > > > > Fix this by ensuring that the DMA snoop bit corresponds with the behaviour of > > > the DMA API. Since the driver currently only supports DT, use > > > of_dma_is_coherent(). Note that device_get_dma_attr() can not be used as > > > that risks re-introducing this bug if/when the driver is converted to ACPI. > > > > [Y.b. Lu] May I have your suggestion how to check this condition with ACPI? > > Although we didn’t support APCI on upstream for this driver now, as I know we had already had ACPI used internally and would be upstreamed. > > Thanks. > > The short answer is, I don't believe there is an equivalent for ACPI. > However, it's a question for ACPI people, not me, as I have little > knowedge about ACPI. > > From what I've looked at so far, for ACPI, the decision used for > dev->dma_coherent() / dev_is_dma_coherent() (which controls the DMA > API behaviour) is not available - this is a decision by the SMMU code. > > See: > drivers/acpi/arm64/iort.c:arm_smmu_dma_configure() > drivers/acpi/scan.c:acpi_dma_configure() > arch/arm64/mm/dma-mapping.c:arch_setup_dma_ops() > > The decision is made whether the SMMU is in coherent walk mode. > > The proposed interface to use is device_get_dma_attr(). For OF, > this works as expected - it mirrors of_dma_is_coherent(). However, > for ACPI it does not - under ACPI, it uses acpi_get_dma_attr(). > > See: > drivers/acpi/scan.c:acpi_get_dma_attr() > > This can return one of three states. Let's concentrate on the > coherent/non-coherent states. This depends on > adev->flags.coherent_dma. This gets set by acpi_init_coherency(), > which looks up the _CCA property in the device handle. > > It looks to me like it is entirely possible for ACPI to say that, for > example, the SMMU is coherent, which will cause dev->dma_coherent to > be set, but the device (and it's parents) not to have _CCA indicating > that the device is coherent. > > It seems that the only way this could be guaranteed is if the ESDHC > was a child of the SMMU - but I don't know whether that is the case > or not. If it isn't, using device_get_dma_attr() will result in a > coherency disagreement between the DMA API and the device, which > will lead to ADMA errors and potential data corruption. > > There may be some subtle reason this can't happen, but from merely > looking at the code, I'd say using device_get_dma_attr() here would > be dangerous. The best I can come up with is something like: enum dev_dma_attr attr, expected; attr = device_get_dma_attr(dev); expected = dev_is_dma_coherent(dev) ? DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT; WARN_ON(attr != expected); Maybe even aborting the probe if they don't agree. However, it seems dev_is_dma_coherent() is an IOMMU/DMA API implementation detail that only IOMMU drivers and arch code are supposed to use. What I'm hearing at the moment from Jon Nettleton is that the NXP ACPI description does not include an IORT table and doesn't mention the SMMU. If I'm reading the code correctly, that means the Linux DMA API will assume all devices are DMA non-coherent - and if we use device_get_dma_attr() for this, and the ACPI device description has _CCA=1, we'll end up with potentially data corrupting mismatched DMA coherency expectations. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
