Re: [PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic

From: Lorenzo Pieralisi
Date: Tue Sep 13 2016 - 09:55:11 EST

Next message: Alex Shi: "Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu"
Previous message: Vivek Goyal: "Re: [PATCH] blk-throttle: fix infinite throttling caused by non-cascading timer wheel"
In reply to: Robin Murphy: "Re: [PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic"
Next in thread: Lorenzo Pieralisi: "[PATCH v5 03/14] drivers: acpi: iort: introduce linker section for IORT entries probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Sep 13, 2016 at 02:38:35PM +0100, Robin Murphy wrote:
> > static int arm_smmu_match_node(struct device *dev, void *data)
> > {
> > - return dev->of_node == data;
> > + struct fwnode_handle *fwnode;
> > +
> > + fwnode = dev->of_node ? &dev->of_node->fwnode : dev->fwnode;
> > +
> > + return fwnode == data;
> > }
>
> Maybe we should hoist the dev_fwnode() helper from property.c up to
> property.h so we can just have "return dev_fwnode(dev) == data;" here?

Yes, that's one way of doing it. The other would be initializing
dev->fwnode to &dev->of_node->fwnode in the DT probe path but first
I need to understand why that is not done in the first place.

[...]

> > +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> > +{
> > + struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > + size_t size;
> > +
> > + if (!fwspec)
> > + return -EINVAL;
> > +
> > + size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 1]);
> > + fwspec = krealloc(fwspec, size, GFP_KERNEL);
> > + if (!fwspec)
> > + return -ENOMEM;
> > +
> > + while (num_ids--)
> > + fwspec->ids[fwspec->num_ids++] = *ids++;
>
> You've still got the +1 bug and incomprehensible loop from the old code
> here, rather than the fixed version being removed below. Although now
> that I've taken the plunge and done it properly in core code from the
> outset, that should hopefully become moot.

Gah sorry, rebase mistake. I will wait for the dust to settle before
churning out a new series, it is hard to respin without a stable
base (hopefully your series will make this patch useless).

Thanks !
Lorenzo

> Robin.
>
> > +
> > + arch_set_iommu_fwspec(dev, fwspec);
> > + return 0;
> > +}
> > +
> > +inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> > +{
> > + return arch_get_iommu_fwspec(dev);
> > +}
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 38669b8..ab3c069 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
> > }
> > EXPORT_SYMBOL_GPL(of_get_dma_window);
> >
> > -struct of_iommu_node {
> > - struct list_head list;
> > - struct device_node *np;
> > - const struct iommu_ops *ops;
> > -};
> > -static LIST_HEAD(of_iommu_list);
> > -static DEFINE_SPINLOCK(of_iommu_lock);
> > -
> > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
> > -{
> > - struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> > -
> > - if (WARN_ON(!iommu))
> > - return;
> > -
> > - of_node_get(np);
> > - INIT_LIST_HEAD(&iommu->list);
> > - iommu->np = np;
> > - iommu->ops = ops;
> > - spin_lock(&of_iommu_lock);
> > - list_add_tail(&iommu->list, &of_iommu_list);
> > - spin_unlock(&of_iommu_lock);
> > -}
> > -
> > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> > -{
> > - struct of_iommu_node *node;
> > - const struct iommu_ops *ops = NULL;
> > -
> > - spin_lock(&of_iommu_lock);
> > - list_for_each_entry(node, &of_iommu_list, list)
> > - if (node->np == np) {
> > - ops = node->ops;
> > - break;
> > - }
> > - spin_unlock(&of_iommu_lock);
> > - return ops;
> > -}
> > -
> > static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> > {
> > struct of_phandle_args *iommu_spec = data;
> > @@ -226,57 +187,3 @@ static int __init of_iommu_init(void)
> > return 0;
> > }
> > postcore_initcall_sync(of_iommu_init);
> > -
> > -int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)
> > -{
> > - struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -
> > - if (fwspec)
> > - return 0;
> > -
> > - fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> > - if (!fwspec)
> > - return -ENOMEM;
> > -
> > - fwspec->iommu_np = of_node_get(iommu_np);
> > - fwspec->iommu_ops = of_iommu_get_ops(iommu_np);
> > - arch_set_iommu_fwspec(dev, fwspec);
> > - return 0;
> > -}
> > -
> > -void iommu_fwspec_free(struct device *dev)
> > -{
> > - struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -
> > - if (fwspec) {
> > - of_node_put(fwspec->iommu_np);
> > - kfree(fwspec);
> > - }
> > -}
> > -
> > -int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> > -{
> > - struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > - size_t size;
> > - int i;
> > -
> > - if (!fwspec)
> > - return -EINVAL;
> > -
> > - size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> > - fwspec = krealloc(fwspec, size, GFP_KERNEL);
> > - if (!fwspec)
> > - return -ENOMEM;
> > -
> > - for (i = 0; i < num_ids; i++)
> > - fwspec->ids[fwspec->num_ids + i] = ids[i];
> > -
> > - fwspec->num_ids += num_ids;
> > - arch_set_iommu_fwspec(dev, fwspec);
> > - return 0;
> > -}
> > -
> > -inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> > -{
> > - return arch_get_iommu_fwspec(dev);
> > -}
> > diff --git a/include/linux/iommu-fwspec.h b/include/linux/iommu-fwspec.h
> > new file mode 100644
> > index 0000000..f88b635
> > --- /dev/null
> > +++ b/include/linux/iommu-fwspec.h
> > @@ -0,0 +1,70 @@
> > +#ifndef __IOMMU_FWSPEC_H
> > +#define __IOMMU_FWSPEC_H
> > +
> > +#include <linux/device.h>
> > +#include <linux/iommu.h>
> > +
> > +struct iommu_fwspec {
> > + const struct iommu_ops *iommu_ops;
> > + struct fwnode_handle *iommu_fwnode;
> > + void *iommu_priv;
> > + unsigned int num_ids;
> > + u32 ids[];
> > +};
> > +
> > +#ifdef CONFIG_IOMMU_FWSPEC
> > +int iommu_fwspec_init(struct device *dev,
> > + struct fwnode_handle *iommu_fwnode);
> > +void iommu_fwspec_free(struct device *dev);
> > +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> > +struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
> > +
> > +void fwspec_iommu_set_ops(struct fwnode_handle *fwnode,
> > + const struct iommu_ops *ops);
> > +const struct iommu_ops *fwspec_iommu_get_ops(struct fwnode_handle *fwnode);
> > +
> > +#ifdef CONFIG_HAVE_IOMMU_FWSPEC
> > +#include <asm/iommu-fwspec.h>
> > +#else /* !CONFIG_HAVE_IOMMU_FWSPEC */
> > +static inline void arch_set_iommu_fwspec(struct device *dev,
> > + struct iommu_fwspec *fwspec) {}
> > +
> > +static inline struct iommu_fwspec *
> > +arch_get_iommu_fwspec(struct device *dev) { return NULL; }
> > +#endif
> > +#else /* CONFIG_IOMMU_FWSPEC */
> > +static inline int iommu_fwspec_init(struct device *dev,
> > + struct fwnode_handle *iommu_fwnode)
> > +{
> > + return -ENODEV;
> > +}
> > +
> > +static inline void iommu_fwspec_free(struct device *dev)
> > +{
> > +}
> > +
> > +static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
> > + int num_ids)
> > +{
> > + return -ENODEV;
> > +}
> > +
> > +static inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> > +{
> > + return NULL;
> > +}
> > +
> > +static inline void fwspec_iommu_set_ops(struct fwnode_handle *fwnode,
> > + const struct iommu_ops *ops)
> > +{
> > +}
> > +
> > +static inline const struct iommu_ops *
> > +fwspec_iommu_get_ops(struct fwnode_handle *fwnode)
> > +{
> > + return NULL;
> > +}
> > +
> > +#endif /* CONFIG_IOMMU_FWSPEC */
> > +
> > +#endif /* __IOMMU_FWSPEC_H */
> > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> > index 358db49..4b02861 100644
> > --- a/include/linux/of_iommu.h
> > +++ b/include/linux/of_iommu.h
> > @@ -3,6 +3,7 @@
> >
> > #include <linux/device.h>
> > #include <linux/iommu.h>
> > +#include <linux/iommu-fwspec.h>
> > #include <linux/of.h>
> >
> > #ifdef CONFIG_OF_IOMMU
> > @@ -14,14 +15,6 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
> > extern const struct iommu_ops *of_iommu_configure(struct device *dev,
> > struct device_node *master_np);
> >
> > -struct iommu_fwspec {
> > - const struct iommu_ops *iommu_ops;
> > - struct device_node *iommu_np;
> > - void *iommu_priv;
> > - unsigned int num_ids;
> > - u32 ids[];
> > -};
> > -
> > #else
> >
> > static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
> > @@ -36,28 +29,19 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
> > {
> > return NULL;
> > }
> > -
> > -struct iommu_fwspec;
> > -
> > #endif /* CONFIG_OF_IOMMU */
> >
> > -int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
> > -void iommu_fwspec_free(struct device *dev);
> > -int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> > -struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
> > -
> > -#ifdef CONFIG_HAVE_IOMMU_FWSPEC
> > -#include <asm/iommu-fwspec.h>
> > -#else /* !CONFIG_HAVE_IOMMU_FWSPEC */
> > -static inline void arch_set_iommu_fwspec(struct device *dev,
> > - struct iommu_fwspec *fwspec) {}
> > -
> > -static inline struct iommu_fwspec *
> > -arch_get_iommu_fwspec(struct device *dev) { return NULL; }
> > -#endif
> > +static inline void of_iommu_set_ops(struct device_node *np,
> > + const struct iommu_ops *ops)
> > +{
> > + fwspec_iommu_set_ops(&np->fwnode, ops);
> > +}
> >
> > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
> > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
> > +static inline const struct iommu_ops *
> > +of_iommu_get_ops(struct device_node *np)
> > +{
> > + return fwspec_iommu_get_ops(&np->fwnode);
> > +}
> >
> > extern struct of_device_id __iommu_of_table;
> >
> >
>

Next message: Alex Shi: "Re: [PATCH 2/4] cpu: expose pm_qos_resume_latency for each cpu"
Previous message: Vivek Goyal: "Re: [PATCH] blk-throttle: fix infinite throttling caused by non-cascading timer wheel"
In reply to: Robin Murphy: "Re: [PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic"
Next in thread: Lorenzo Pieralisi: "[PATCH v5 03/14] drivers: acpi: iort: introduce linker section for IORT entries probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]