| Message ID | f52a07f7320da94afe575c9631340d0019a203a7.1666042873.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Define EINVAL as device/domain incompatibility | expand |
On Mon, Oct 17, 2022 at 04:02:21PM -0700, Nicolin Chen wrote: > Following the new rules in include/linux/iommu.h kdocs, update all drivers > ->attach_dev callback functions to return EINVAL in the failure paths that > are related to domain incompatibility. > > Also, drop adjacent error prints to prevent a kernel log spam. > > Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 +---------- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 3 --- > drivers/iommu/arm/arm-smmu/qcom_iommu.c | 7 +------ > drivers/iommu/intel/iommu.c | 10 +++------- > drivers/iommu/ipmmu-vmsa.c | 2 -- > drivers/iommu/omap-iommu.c | 2 +- > drivers/iommu/sprd-iommu.c | 4 +--- > drivers/iommu/tegra-gart.c | 2 +- > drivers/iommu/virtio-iommu.c | 3 +-- > 9 files changed, 9 insertions(+), 35 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index ba47c73f5b8c..01fd7df16cb9 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2430,23 +2430,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > goto out_unlock; > } > } else if (smmu_domain->smmu != smmu) { > - dev_err(dev, > - "cannot attach to SMMU %s (upstream of %s)\n", > - dev_name(smmu_domain->smmu->dev), > - dev_name(smmu->dev)); > - ret = -ENXIO; > + ret = -EINVAL; > goto out_unlock; > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) { > - dev_err(dev, > - "cannot attach to incompatible domain (%u SSID bits != %u)\n", > - smmu_domain->s1_cfg.s1cdmax, master->ssid_bits); > ret = -EINVAL; > goto out_unlock; > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > smmu_domain->stall_enabled != master->stall_enabled) { > - dev_err(dev, "cannot attach to stall-%s domain\n", > - smmu_domain->stall_enabled ? "enabled" : "disabled"); > ret = -EINVAL; > goto out_unlock; > } I think it would be helpful to preserve these messages using dev_err_ratelimited() so that attach failure can be diagnosed without having to hack the messages back into the driver. With that: Acked-by: Will Deacon <will@kernel.org> Will
On Mon, Nov 07, 2022 at 03:26:45PM +0000, Will Deacon wrote: > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > index ba47c73f5b8c..01fd7df16cb9 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -2430,23 +2430,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > goto out_unlock; > > } > > } else if (smmu_domain->smmu != smmu) { > > - dev_err(dev, > > - "cannot attach to SMMU %s (upstream of %s)\n", > > - dev_name(smmu_domain->smmu->dev), > > - dev_name(smmu->dev)); > > - ret = -ENXIO; > > + ret = -EINVAL; > > goto out_unlock; > > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > > master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) { > > - dev_err(dev, > > - "cannot attach to incompatible domain (%u SSID bits != %u)\n", > > - smmu_domain->s1_cfg.s1cdmax, master->ssid_bits); > > ret = -EINVAL; > > goto out_unlock; > > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > > smmu_domain->stall_enabled != master->stall_enabled) { > > - dev_err(dev, "cannot attach to stall-%s domain\n", > > - smmu_domain->stall_enabled ? "enabled" : "disabled"); > > ret = -EINVAL; > > goto out_unlock; > > } > I think it would be helpful to preserve these messages using > dev_err_ratelimited() so that attach failure can be diagnosed without > having to hack the messages back into the driver. Thank you for the review. The change is already picked up last week. Yet, I can add prints back with a followup patch, if no one has a problem with that. Also, I am not quite sure what the use case would be to have an error print. Perhaps dev_dbg() would be more fitting if it is just for diagnosis? Thanks Nic
On Mon, Nov 07, 2022 at 04:14:32PM -0800, Nicolin Chen wrote: > On Mon, Nov 07, 2022 at 03:26:45PM +0000, Will Deacon wrote: > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > index ba47c73f5b8c..01fd7df16cb9 100644 > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > @@ -2430,23 +2430,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > > goto out_unlock; > > > } > > > } else if (smmu_domain->smmu != smmu) { > > > - dev_err(dev, > > > - "cannot attach to SMMU %s (upstream of %s)\n", > > > - dev_name(smmu_domain->smmu->dev), > > > - dev_name(smmu->dev)); > > > - ret = -ENXIO; > > > + ret = -EINVAL; > > > goto out_unlock; > > > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > > > master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) { > > > - dev_err(dev, > > > - "cannot attach to incompatible domain (%u SSID bits != %u)\n", > > > - smmu_domain->s1_cfg.s1cdmax, master->ssid_bits); > > > ret = -EINVAL; > > > goto out_unlock; > > > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > > > smmu_domain->stall_enabled != master->stall_enabled) { > > > - dev_err(dev, "cannot attach to stall-%s domain\n", > > > - smmu_domain->stall_enabled ? "enabled" : "disabled"); > > > ret = -EINVAL; > > > goto out_unlock; > > > } > > > I think it would be helpful to preserve these messages using > > dev_err_ratelimited() so that attach failure can be diagnosed without > > having to hack the messages back into the driver. > > Thank you for the review. > > The change is already picked up last week. Yet, I can add prints > back with a followup patch, if no one has a problem with that. Sorry, I fell behind with upstream so I got to this late. A patch on top would be fantastic! > Also, I am not quite sure what the use case would be to have an > error print. Perhaps dev_dbg() would be more fitting if it is > just for diagnosis? Sure, that works for me. I think the messages are useful for folks triggering this path e.g. via sysfs but if they're limited to debug I think that's better than removing them altogether. Cheers, Will
On Tue, Nov 08, 2022 at 01:20:42PM +0000, Will Deacon wrote: > On Mon, Nov 07, 2022 at 04:14:32PM -0800, Nicolin Chen wrote: > > On Mon, Nov 07, 2022 at 03:26:45PM +0000, Will Deacon wrote: > > > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > > index ba47c73f5b8c..01fd7df16cb9 100644 > > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > > @@ -2430,23 +2430,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > > > goto out_unlock; > > > > } > > > > } else if (smmu_domain->smmu != smmu) { > > > > - dev_err(dev, > > > > - "cannot attach to SMMU %s (upstream of %s)\n", > > > > - dev_name(smmu_domain->smmu->dev), > > > > - dev_name(smmu->dev)); > > > > - ret = -ENXIO; > > > > + ret = -EINVAL; > > > > goto out_unlock; > > > > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > > > > master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) { > > > > - dev_err(dev, > > > > - "cannot attach to incompatible domain (%u SSID bits != %u)\n", > > > > - smmu_domain->s1_cfg.s1cdmax, master->ssid_bits); > > > > ret = -EINVAL; > > > > goto out_unlock; > > > > } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > > > > smmu_domain->stall_enabled != master->stall_enabled) { > > > > - dev_err(dev, "cannot attach to stall-%s domain\n", > > > > - smmu_domain->stall_enabled ? "enabled" : "disabled"); > > > > ret = -EINVAL; > > > > goto out_unlock; > > > > } > > > > > I think it would be helpful to preserve these messages using > > > dev_err_ratelimited() so that attach failure can be diagnosed without > > > having to hack the messages back into the driver. > > > > Thank you for the review. > > > > The change is already picked up last week. Yet, I can add prints > > back with a followup patch, if no one has a problem with that. > > Sorry, I fell behind with upstream so I got to this late. A patch on top > would be fantastic! > > > Also, I am not quite sure what the use case would be to have an > > error print. Perhaps dev_dbg() would be more fitting if it is > > just for diagnosis? > > Sure, that works for me. I think the messages are useful for folks > triggering this path e.g. via sysfs but if they're limited to debug I think > that's better than removing them altogether. I suspsect it has to be dbg - vfio/iommufd will probably trigger these messages as it probes for domains that are compatible - eg certainly the first one. Even if it is a "once" it would still emit a confusing message for a normal occurance. This is why they were removed in the first place.. Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index ba47c73f5b8c..01fd7df16cb9 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2430,23 +2430,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) goto out_unlock; } } else if (smmu_domain->smmu != smmu) { - dev_err(dev, - "cannot attach to SMMU %s (upstream of %s)\n", - dev_name(smmu_domain->smmu->dev), - dev_name(smmu->dev)); - ret = -ENXIO; + ret = -EINVAL; goto out_unlock; } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) { - dev_err(dev, - "cannot attach to incompatible domain (%u SSID bits != %u)\n", - smmu_domain->s1_cfg.s1cdmax, master->ssid_bits); ret = -EINVAL; goto out_unlock; } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && smmu_domain->stall_enabled != master->stall_enabled) { - dev_err(dev, "cannot attach to stall-%s domain\n", - smmu_domain->stall_enabled ? "enabled" : "disabled"); ret = -EINVAL; goto out_unlock; } diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index 6c1114a4d6cc..3bca3cc1ceae 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -1150,9 +1150,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) * different SMMUs. */ if (smmu_domain->smmu != smmu) { - dev_err(dev, - "cannot attach to SMMU %s whilst already attached to domain on SMMU %s\n", - dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev)); ret = -EINVAL; goto rpm_put; } diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c index 3869c3ecda8c..bfd7b51eb5db 100644 --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c @@ -381,13 +381,8 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev * Sanity check the domain. We don't support domains across * different IOMMUs. */ - if (qcom_domain->iommu != qcom_iommu) { - dev_err(dev, "cannot attach to IOMMU %s while already " - "attached to domain on IOMMU %s\n", - dev_name(qcom_domain->iommu->dev), - dev_name(qcom_iommu->dev)); + if (qcom_domain->iommu != qcom_iommu) return -EINVAL; - } return 0; } diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index a8b36c3fddf1..1ac94f7fc8be 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4189,19 +4189,15 @@ static int prepare_domain_attach_device(struct iommu_domain *domain, return -ENODEV; if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap)) - return -EOPNOTSUPP; + return -EINVAL; /* check if this iommu agaw is sufficient for max mapped address */ addr_width = agaw_to_width(iommu->agaw); if (addr_width > cap_mgaw(iommu->cap)) addr_width = cap_mgaw(iommu->cap); - if (dmar_domain->max_addr > (1LL << addr_width)) { - dev_err(dev, "%s: iommu width (%d) is not " - "sufficient for the mapped address (%llx)\n", - __func__, addr_width, dmar_domain->max_addr); - return -EFAULT; - } + if (dmar_domain->max_addr > (1LL << addr_width)) + return -EINVAL; dmar_domain->gaw = addr_width; /* diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c index 3b30c0752274..22230cc15dcd 100644 --- a/drivers/iommu/ipmmu-vmsa.c +++ b/drivers/iommu/ipmmu-vmsa.c @@ -628,8 +628,6 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain, * Something is wrong, we can't attach two devices using * different IOMMUs to the same domain. */ - dev_err(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n", - dev_name(mmu->dev), dev_name(domain->mmu->dev)); ret = -EINVAL; } else dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id); diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index 3f153f9e0ac5..2fd7702c6709 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -1472,7 +1472,7 @@ omap_iommu_attach_dev(struct iommu_domain *domain, struct device *dev) /* only a single client device can be attached to a domain */ if (omap_domain->dev) { dev_err(dev, "iommu domain is already attached\n"); - ret = -EBUSY; + ret = -EINVAL; goto out; } diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c index fadd2c907222..e02793375598 100644 --- a/drivers/iommu/sprd-iommu.c +++ b/drivers/iommu/sprd-iommu.c @@ -237,10 +237,8 @@ static int sprd_iommu_attach_device(struct iommu_domain *domain, struct sprd_iommu_domain *dom = to_sprd_domain(domain); size_t pgt_size = sprd_iommu_pgt_size(domain); - if (dom->sdev) { - pr_err("There's already a device attached to this domain.\n"); + if (dom->sdev) return -EINVAL; - } dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL); if (!dom->pgt_va) diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c index e5ca3cf1a949..ed53279d1106 100644 --- a/drivers/iommu/tegra-gart.c +++ b/drivers/iommu/tegra-gart.c @@ -112,7 +112,7 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain, spin_lock(&gart->dom_lock); if (gart->active_domain && gart->active_domain != domain) { - ret = -EBUSY; + ret = -EINVAL; } else if (dev_iommu_priv_get(dev) != domain) { dev_iommu_priv_set(dev, domain); gart->active_domain = domain; diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c index f0caf9a4f6f8..9e0ca757fa42 100644 --- a/drivers/iommu/virtio-iommu.c +++ b/drivers/iommu/virtio-iommu.c @@ -732,8 +732,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) */ ret = viommu_domain_finalise(vdev, domain); } else if (vdomain->viommu != vdev->viommu) { - dev_err(dev, "cannot attach to foreign vIOMMU\n"); - ret = -EXDEV; + ret = -EINVAL; } mutex_unlock(&vdomain->mutex);