On Wed, Feb 1, 2017 at 11:10 PM, Sricharan <sricharan@xxxxxxxxxxxxxx> wrote: > Hi Rob, > >>On Wed, Feb 1, 2017 at 10:23 AM, Rob Clark <robdclark@xxxxxxxxx> wrote: >>> Before the driver is probed, arm_smmu_add_device() helpfully attaches >>> an IOMMU_DOMAIN_DMA domain. Which ofc does not support stalling, and >>> when the driver later attaches a domain that can_stall to an smmu that >>> can stall, the default _DMA domain prevents stalling from being enabled. >>> (And will cause further problems later) >>> >>> One simple way to deal with this is simply toss the default _DMA domain >>> if the driver attaches it's own domain. >>> >>> TODO maybe the tracking of list of attached domains should be done in >>> iommu core, so the detach can happen outside of group->mutex. >>> >>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx> >>> --- >>> drivers/iommu/arm-smmu.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 96a1be6..50bf135 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -1323,6 +1323,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) >>> >>> smmu = fwspec_smmu(fwspec); >>> >>> + /* >>> + * If driver is explicitly managing the iommu, detatch any previously >>> + * attached _DMA domains. >>> + * >>> + * TODO maybe this logic should be in iommu_attach_device() so it can >>> + * happen outside of holding group->mutex?? >>> + */ >>> + if (domain->type != IOMMU_DOMAIN_DMA) { >>> + struct arm_smmu_domain *other_domain, *n; >>> + >>> + list_for_each_entry_safe(other_domain, n, &smmu->domain_list, domain_node) >>> + if (other_domain->domain.type == IOMMU_DOMAIN_DMA) >>> + arm_smmu_detach_dev(&other_domain->domain, dev); >> > > So the arm_smmu_detach_dev api is no more there and is removed now. > Also this will be a problem when multiple devices share the iommu, we end up > removing domains used by other devices. Should this not be done > per-device which does not want to use the DMA domain ? > >>hmm, we might want to unhook dev->archdata.dma_ops here too.. >> >>I'm thinking maybe on arm64 __generic_dma_ops() should fall back to >>swiotlb ops instead of dummy_ops if archdata.dma_ops is NULL, so that >>we could just set it to NULL here? >> > > hmm, both not attaching the default dma domain and not setting the dma_ops > is tried in this series as well [1] > > [1] https://www.spinics.net/lists/arm-kernel/msg556081.html > >>(Is there really any purpose for having the dummy-ops??) >> > > To enforce setting arch_setup_dma_ops for device so that the > devices can do cache coherent transactions, otherwise disable the dma > capability of the device. I see that this was introduced as a part of > making ACPI_CCA_REQUIRED to be set in arm64 and later > generalized. hmm, maybe fallback to swiotlb ops vs dummy ops should depend on whether it is an ACPI system or not.. BR, -R -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html