Hi Bjorn, On Mon, Jul 27, 2015 at 05:54:53PM -0500, Bjorn Helgaas wrote: > > Hmm, this is a place where the relaxed error handling in > > pci_enable_ats() can get problematic. > > By "relaxed error handling," do you mean the fact that in v4.1, > pci_enable_ats() has a BUG_ON(is_enabled), while now it merely > returns -EINVAL? > > (BTW, I did change it to add a WARN_ON and return -EBUSY as you suggested.) Okay, great. > > If ATS is (by accident or a bug > > elsewhere) already enabled an the function returns -EINVAL, the IOMMU > > driver considers ATS disabled and doesn't flush the IO/TLBs of the > > device. This can cause data corruption later on, so we should make sure > > that info->ats.enabled is consistent with pdev->ats_enabled. > > I'm not quite sure I understand this. In the patch above, we only set > "info->ats.enabled = 1" if pci_enable_ats() has succeeded. The amd_iommu.c > code is similar. > > Are you concerned about the case where future code enables ATS before > intel-iommu, the pci_enable_ats() in intel-iommu fails, intel-iommu > believes ATS is disabled, intel-iommu calls iommu_flush_dev_iotlb(), but it > doesn't flush the IOTLB? > > I guess I could make intel-iommu handle -EBUSY differently from -EINVAL. > Would that help? It seems sort of clumsy, but ...? I was concerned that it was harder now to spot bugs in ATS enabling/disabling, when pci_enable_ats just returns -EINVAL when ATS is already enabled. But with the WARN_ON now we will notice these bugs early again, thanks for adding it. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
