On 08/03/18 16:17, Jonathan Cameron wrote:

quoted

+	arm_smmu_enable_ats(master);

It's a bit nasty not to handle the errors that this could output (other than
the ENOSYS for when it's not available). Seems that it would be nice to at
least add a note to the log if people are expecting it to work and it won't
because some condition or other isn't met.

I agree it's not ideal. Last time this came up the problem was that
checking if ATS is supported requires an ugly ifdef. A proper
implementation requires more support in the PCI core, e.g. a
pci_ats_supported() function.

https://www.spinics.net/lists/kvm/msg145932.html

quoted

+
 	group = iommu_group_get_for_dev(dev);
-	if (!IS_ERR(group)) {
-		arm_smmu_insert_master(smmu, master);
-		iommu_group_put(group);
-		iommu_device_link(&smmu->iommu, dev);
+	if (IS_ERR(group)) {
+		ret = PTR_ERR(group);
+		goto err_disable_ats;
 	}
 
-	return PTR_ERR_OR_ZERO(group);
+	iommu_group_put(group);
+	arm_smmu_insert_master(smmu, master);
+	iommu_device_link(&smmu->iommu, dev);
+
+	return 0;
+
+err_disable_ats:
+	arm_smmu_disable_ats(master);

master is leaked here I think...
Possibly other things as this doesn't line up with the
remove which I'd have mostly expected it to do.

There are some slightly fishy bits of ordering in the original code
anyway that I'm not seeing justification for (why is
the iommu_device_unlink later than one might expect for
example).

Yeah, knowing the rest of the probing code, there may exist subtle legacy
reasons for not freeing the master here and the strange orderings. I try
to keep existing behaviors where possible since I barely even have the
bandwidth to fix my own code.

Thanks,
Jean