On Mon, Jan 25, 2021 at 01:50:09PM +0000, Jonathan Cameron wrote:
> > +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
>
> > + 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.
>
>
> > +
> > + iommu_unregister_device_fault_handler(dev);
> > + iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> > +}
