Re: [PATCH v11 10/10] iommu/arm-smmu-v3: Add stall support for platform devices

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

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);
> > +}