On Thu, Dec 4, 2014 at 1:43 PM, Robin Murphy <robin.mur...@arm.com> wrote:
> Hi Grant, thanks for the advice - silly micro-optimisations removed, and
> I'll make a note to do so from my in-development code, too ;)
>
> I didn't much like the casting either, so rather than push it elsewhere or
> out to the caller I've just changed the prototype to obviate it completely.
> Since we're also expecting to churn this again to use something more
> suitable than iommu_ops as the private data, I think keeping things simple
> wins over const-correctness for now.
>
> Thanks,
> Robin
>
> --->8---
> From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001
> Message-Id:
> <b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.mur...@arm.com>
> From: Robin Murphy <robin.mur...@arm.com>
> Date: Thu, 4 Dec 2014 11:53:13 +0000
> Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately
>
> Since the data pointer in the DT node is public and may be overwritten
> by conflicting code, move the DT-probed IOMMU ops to a private list
> where they will be safe.
>
> Signed-off-by: Robin Murphy <robin.mur...@arm.com>
Acked-by: Grant Likely <grant.lik...@linaro.org>
> ---
> drivers/iommu/of_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
> include/linux/of_iommu.h | 12 ++----------
> 2 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 73236d3..c7078f6 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -94,6 +94,46 @@ 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;
> + 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, struct iommu_ops *ops)
> +{
> + struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> +
> + if (!iommu) {
> + __WARN();
> + return;
> + }
> +
> + 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);
> +}
> +
> +struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> +{
> + struct of_iommu_node *node;
> + 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;
> +}
> +
> struct iommu_ops *of_iommu_configure(struct device *dev)
> {
> struct of_phandle_args iommu_spec;
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index d03abbb..16c7554 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
> device *dev)
>
> #endif /* CONFIG_OF_IOMMU */
>
> -static inline void of_iommu_set_ops(struct device_node *np,
> - const struct iommu_ops *ops)
> -{
> - np->data = (struct iommu_ops *)ops;
> -}
> -
> -static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> -{
> - return np->data;
> -}
> +void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops);
> +struct iommu_ops *of_iommu_get_ops(struct device_node *np);
>
> extern struct of_device_id __iommu_of_table;
>
> --
> 1.9.1
>
> On 04/12/14 12:42, Grant Likely wrote:
>>
>> On Thu, Dec 4, 2014 at 12:26 PM, Robin Murphy <robin.mur...@arm.com>
>> wrote:
>>>
>>> Hi Arnd,
>>>
>>> On 03/12/14 19:57, Arnd Bergmann wrote:
>>> [...]
>>>>>
>>>>>
>>>>> Good catch. This is not good. The data pointer should be avoided since
>>>>> there are no controls over its use. Until a better solution can be
>>>>> implemented, probably the safest thing to do is add a struct iommu_ops
>>>>> pointer to struct device_node. However, assuming that only a small
>>>>> portion of nodes will actually have iommu_ops set, I'd rather see a
>>>>> separate registry that matches device_nodes to iommu_ops.
>>>>
>>>>
>>>>
>>>> Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
>>>> adapt it as needed? It should be exactly what we need to start
>>>> out and can be extended and generalized later.
>>>
>>>
>>>
>>> I'm quite keen to see this series go in, since I'm depending on it to
>>> make
>>> arm64 IOMMU DMA ops "just work". Will and I came to the conclusion the
>>> other
>>> day that we pretty much need to build up some kind of bus abstraction
>>> based
>>> on the probe data in order to be able to assign IOMMU groups correctly,
>>> which can also subsume this particular problem in the long run. Since
>>> I've
>>> made a start on that already, I've hacked the following short-term fix
>>> out
>>> of it. Tested on my Juno - admittedly with only two SMMUs and one master
>>> (EHCI) being probed, but it didn't blow up or regress anything.
>>>
>>> Regards,
>>> Robin.
>>>
>>> --->8---
>>> From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001
>>> Message-Id:
>>>
>>> <1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.mur...@arm.com>
>>> From: Robin Murphy <robin.mur...@arm.com>
>>> Date: Thu, 4 Dec 2014 11:53:13 +0000
>>> Subject: [PATCH] iommu: store DT-probed IOMMU data privately
>>>
>>> Since the data pointer in the DT node is public and may be overwritten
>>> by conflicting code, move the DT-probed IOMMU ops to a private list
>>> where they will be safe.
>>>
>>> Signed-off-by: Robin Murphy <robin.mur...@arm.com>
>>
>>
>> Looks reasonable to me. Comments below...
>>
>>> ---
>>> drivers/iommu/of_iommu.c | 38 ++++++++++++++++++++++++++++++++++++++
>>> include/linux/of_iommu.h | 12 ++----------
>>> 2 files changed, 40 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>>> index 73236d3..5cd451c 100644
>>> --- a/drivers/iommu/of_iommu.c
>>> +++ b/drivers/iommu/of_iommu.c
>>> @@ -94,6 +94,44 @@ 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 hlist_node list;
>>> + struct device_node *np;
>>> + const struct iommu_ops *ops;
>>> +};
>>> +static HLIST_HEAD(of_iommu_list);
>>
>>
>> Just use a list_head. hlist_head merely saves one pointer in this case.
>>
>>> +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 = kmalloc(sizeof(*iommu),
>>> GFP_KERNEL);
>>
>>
>> kzalloc()
>>
>>> +
>>> + if (!iommu)
>>> + return;
>>
>>
>> Shouldn't there be a WARN() here on failure? I don't think failing
>> silently is desired.
>>
>>> +
>>> + INIT_HLIST_NODE(&iommu->list);
>>> + iommu->np = np;
>>> + iommu->ops = ops;
>>> + spin_lock(&of_iommu_lock);
>>> + hlist_add_head(&iommu->list, &of_iommu_list);
>>> + spin_unlock(&of_iommu_lock);
>>> +}
>>> +
>>> +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);
>>> + hlist_for_each_entry(node, &of_iommu_list, list)
>>> + if (node->np == np) {
>>> + ops = node->ops;
>>> + break;
>>> + }
>>> + spin_unlock(&of_iommu_lock);
>>> + return (struct iommu_ops *)ops;
>>
>>
>> The cast looks fishy. If you need to use a cast, then the data types
>> are probably wrong. If you drop the const from *ops here and in the
>> structure then it should probably work fine. Due to the way it is
>> being used, there isn't any advantage to using const (unless you
>> changes of_iommu_get_ops() to return a const pointer, then const would
>> make sense).
>>
>>> +}
>>> +
>>> struct iommu_ops *of_iommu_configure(struct device *dev)
>>> {
>>> struct of_phandle_args iommu_spec;
>>> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
>>> index d03abbb..e27c53a 100644
>>> --- a/include/linux/of_iommu.h
>>> +++ b/include/linux/of_iommu.h
>>> @@ -31,16 +31,8 @@ static inline struct iommu_ops
>>> *of_iommu_configure(struct
>>> device *dev)
>>>
>>> #endif /* CONFIG_OF_IOMMU */
>>>
>>> -static inline void of_iommu_set_ops(struct device_node *np,
>>> - const struct iommu_ops *ops)
>>> -{
>>> - np->data = (struct iommu_ops *)ops;
>>> -}
>>> -
>>> -static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>>> -{
>>> - return np->data;
>>> -}
>>> +void of_iommu_set_ops(struct device_node *np, const struct iommu_ops
>>> *ops);
>>> +struct iommu_ops *of_iommu_get_ops(struct device_node *np);
>>>
>>> extern struct of_device_id __iommu_of_table;
>>>
>>> --
>>> 1.9.1
>>>
>>>
>>
>
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
