On Mon, Jan 25, 2021 at 01:50:09PM +0000, Jonathan Cameron wrote:

quoted

+static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
+{
+	int ret;
+	struct device *dev = master->dev;
+
+	/*
+	 * Drivers for devices supporting PRI or stall should enable IOPF first.
+	 * Others have device-specific fault handlers and don't need IOPF.
+	 */
+	if (!arm_smmu_master_iopf_supported(master))

So if we have master->iopf_enabled and this happens. Then I'm not totally sure
what prevents the disable below running its cleanup on stuff that was never
configured.

Since arm_smmu_dev_enable_feature() checks that the feature is supported,
iopf_enabled can only be true if arm_smmu_master_iopf_supported() is true.

What's missing is checking that drivers don't disable IOPF while SVA is
enabled - or else the disable below can leak. Another thing I broke in v10 :/

Thanks,
Jean

quoted

+		return 0;
+
+	if (!master->iopf_enabled)
+		return -EINVAL;
+
+	ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev);
+	if (ret)
+		return ret;
+
+	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
+	if (ret) {
+		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+		return ret;
+	}
+	return 0;
+}
+
+static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master)
+{
+	struct device *dev = master->dev;
+
+	if (!master->iopf_enabled)
+		return;

As above, I think you need a sanity check on

!arm_smmu_master_iopf_supported(master) before clearing the following.

I may well be missing something that stops us getting here though.

Alternative is probably to sanity check iopf_enabled = true is supported
before letting a driver set it.

quoted

+
+	iommu_unregister_device_fault_handler(dev);
+	iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+}