Re: [RFC 3/3] iommu/arm-smmu: detach DMA domain if driver is managing iommu

[Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Wed, Feb 1, 2017 at 11:10 PM, Sricharan <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Hi Rob,
>
>>On Wed, Feb 1, 2017 at 10:23 AM, Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  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