On Wed, May 31, 2017 at 06:52:30PM +0100, Robin Murphy wrote: > When a PCI device has DMA quirks, we need to ensure that an upstream > IOMMU knows about all possible aliases, since the presence of a DMA > quirk does not preclude the device still also emitting transactions > (e.g. MSIs) on its 'real' RID. Similarly, the rules for bridge aliasing > are relatively complex, and some bridges may only take ownership of > transactions under particular transient circumstances, leading again to > multiple RIDs potentially being seen at the IOMMU for the given device. > > Take all this into account in the IORT code by mapping every RID > produced by the alias walk, not just whichever one comes out last. Since > adding any more internal PTR_ERR() juggling would have confused me no > end, a bit of refactoring happens in the process - we know where to find > the ops if everything succeeded, so we're free to just pass regular error > codes around up until then. > > Signed-off-by: Robin Murphy <[email protected]> > --- > > This applies on top of the fixes currently queued in the IOMMU tree: > "ACPI/IORT: Move the check to get iommu_ops from translated fwspec" > "ACPI/IORT: Ignore all errors except EPROBE_DEFER" > > drivers/acpi/arm64/iort.c | 113 > ++++++++++++++++++++++++----------------------
It is a bit hard to read (and it is certainly not your fault) but the patch makes sense to me, I wonder whether it is not better to split it into two patches, one refactoring the return value and second one managing the aliases, _properly_ :), anyway: Reviewed-by: Lorenzo Pieralisi <[email protected]> > 1 file changed, 59 insertions(+), 54 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 797b28dc7b34..50e2749eafdc 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -598,14 +598,6 @@ void acpi_configure_pmsi_domain(struct device *dev) > dev_set_msi_domain(dev, msi_domain); > } > > -static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data) > -{ > - u32 *rid = data; > - > - *rid = alias; > - return 0; > -} > - > static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, > struct fwnode_handle *fwnode, > const struct iommu_ops *ops) > @@ -643,8 +635,7 @@ int iort_add_device_replay(const struct iommu_ops *ops, > struct device *dev) > { > int err = 0; > > - if (!IS_ERR_OR_NULL(ops) && ops->add_device && dev->bus && > - !dev->iommu_group) > + if (ops->add_device && dev->bus && !dev->iommu_group) > err = ops->add_device(dev); > > return err; > @@ -658,36 +649,49 @@ int iort_add_device_replay(const struct iommu_ops *ops, > struct device *dev) > { return 0; } > #endif > > -static const struct iommu_ops *iort_iommu_xlate(struct device *dev, > - struct acpi_iort_node *node, > - u32 streamid) > +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node, > + u32 streamid) > { > - const struct iommu_ops *ops = NULL; > - int ret = -ENODEV; > + const struct iommu_ops *ops; > struct fwnode_handle *iort_fwnode; > > - if (node) { > - iort_fwnode = iort_get_fwnode(node); > - if (!iort_fwnode) > - return NULL; > + if (!node) > + return -ENODEV; > > - ops = iommu_ops_from_fwnode(iort_fwnode); > - /* > - * If the ops look-up fails, this means that either > - * the SMMU drivers have not been probed yet or that > - * the SMMU drivers are not built in the kernel; > - * Depending on whether the SMMU drivers are built-in > - * in the kernel or not, defer the IOMMU configuration > - * or just abort it. > - */ > - if (!ops) > - return iort_iommu_driver_enabled(node->type) ? > - ERR_PTR(-EPROBE_DEFER) : NULL; > + iort_fwnode = iort_get_fwnode(node); > + if (!iort_fwnode) > + return -ENODEV; > > - ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops); > - } > + ops = iommu_ops_from_fwnode(iort_fwnode); > + /* > + * If the ops look-up fails, this means that either > + * the SMMU drivers have not been probed yet or that > + * the SMMU drivers are not built in the kernel; > + * Depending on whether the SMMU drivers are built-in > + * in the kernel or not, defer the IOMMU configuration > + * or just abort it. > + */ > + if (!ops) > + return iort_iommu_driver_enabled(node->type) ? > + -EPROBE_DEFER : -ENODEV; > > - return ret ? NULL : ops; > + return arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops); > +} > + > +struct iort_pci_alias_info { > + struct device *dev; > + struct acpi_iort_node *node; > +}; > + > +static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data) > +{ > + struct iort_pci_alias_info *info = data; > + struct acpi_iort_node *parent; > + u32 streamid; > + > + parent = iort_node_map_id(info->node, alias, &streamid, > + IORT_IOMMU_TYPE); > + return iort_iommu_xlate(info->dev, parent, streamid); > } > > /** > @@ -723,7 +727,7 @@ void iort_set_dma_mask(struct device *dev) > const struct iommu_ops *iort_iommu_configure(struct device *dev) > { > struct acpi_iort_node *node, *parent; > - const struct iommu_ops *ops = NULL; > + const struct iommu_ops *ops; > u32 streamid = 0; > int err; > > @@ -737,21 +741,18 @@ const struct iommu_ops *iort_iommu_configure(struct > device *dev) > > if (dev_is_pci(dev)) { > struct pci_bus *bus = to_pci_dev(dev)->bus; > - u32 rid; > - > - pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, > - &rid); > + struct iort_pci_alias_info info = { .dev = dev }; > > node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, > iort_match_node_callback, &bus->dev); > if (!node) > return NULL; > > - parent = iort_node_map_id(node, rid, &streamid, > - IORT_IOMMU_TYPE); > - > - ops = iort_iommu_xlate(dev, parent, streamid); > - > + info.node = node; > + err = pci_for_each_dma_alias(to_pci_dev(dev), > + iort_pci_iommu_init, &info); > + if (err) > + goto out_err; > } else { > int i = 0; > > @@ -764,9 +765,9 @@ const struct iommu_ops *iort_iommu_configure(struct > device *dev) > IORT_IOMMU_TYPE, i++); > > while (parent) { > - ops = iort_iommu_xlate(dev, parent, streamid); > - if (IS_ERR_OR_NULL(ops)) > - return ops; > + err = iort_iommu_xlate(dev, parent, streamid); > + if (err) > + goto out_err; > > parent = iort_node_map_platform_id(node, &streamid, > IORT_IOMMU_TYPE, > @@ -774,21 +775,25 @@ const struct iommu_ops *iort_iommu_configure(struct > device *dev) > } > } > > + ops = dev->iommu_fwspec->ops; > + > /* > * If we have reason to believe the IOMMU driver missed the initial > * add_device callback for dev, replay it to get things in order. > */ > err = iort_add_device_replay(ops, dev); > if (err) > - ops = ERR_PTR(err); > - > - /* Ignore all other errors apart from EPROBE_DEFER */ > - if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) { > - dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops)); > - ops = NULL; > - } > + goto out_err; > > return ops; > + > + /* Ignore all other errors apart from EPROBE_DEFER */ > +out_err: > + if (err == -EPROBE_DEFER) > + return ERR_PTR(err); > + > + dev_dbg(dev, "Adding to IOMMU failed: %d\n", err); > + return NULL; > } > > static void __init acpi_iort_register_irq(int hwirq, const char *name, > -- > 2.12.2.dirty >
