On Thu, Jul 13, 2017 at 5:50 AM, Robin Murphy [off-list ref] wrote:

On 13/07/17 07:48, Stephen Boyd wrote:

quoted

On 07/13, Vivek Gautam wrote:

quoted

Hi Stephen,


On 07/13/2017 04:24 AM, Stephen Boyd wrote:

quoted

On 07/06, Vivek Gautam wrote:

quoted

@@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
 static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
                        size_t size)
 {
-  struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
+  struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+  struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+  size_t ret;
   if (!ops)
           return 0;
-  return ops->unmap(ops, iova, size);
+  pm_runtime_get_sync(smmu_domain->smmu->dev);

Can these map/unmap ops be called from an atomic context? I seem
to recall that being a problem before.

That's something which was dropped in the following patch merged in master:
523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock

Looks like we don't  need locks here anymore?

While removing the spinlock around the map/unmap path may be one
thing, I'm not sure that's all of them. Is there a path from an
atomic DMA allocation (GFP_ATOMIC sort of thing) mapped into an
IOMMU for a device that can eventually get down to here and
attempt to turn a clk on?

Yes, in the DMA path map/unmap will frequently be called from IRQ
handlers (think e.g. network packets). The whole point of removing the
lock was to allow multiple maps/unmaps to execute in parallel (since we
know they will be safely operating on different areas of the pagetable).
AFAICS this change is going to largely reintroduce that bottleneck via
dev->power_lock, which is anything but what we want :(

Maybe __pm_runtime_resume() needs some sort of fast-path if already
enabled?  Or otherwise we need some sort of flag to tell the iommu
that it cannot rely on the unmapping device to be resumed?

BR,
-R