On Fri, Sep 30, 2011 at 9:06 PM, Roedel, Joerg <Joerg.Roedel@xxxxxxx> wrote:
> First comment: Pleas remove the 'inline' annotations in this patch. It
> is better to let the compiler decide what to inline and what not.
Ok. Thanks :)
>
> Hmm, may it make sense to store data directly in dev->arch.iommu? This
> will save you the list traversals to get the information.
>
That looks better.
However, adding features to dev_archdata of ARM is actively discussed
in Linaro-mm-sig and the features also include iommu_domain.
On the other hand,
I was not able to determine what will be accepted to the mainline kernel
between Linaro's one that is suggested by Marek and Ohad's one.
>> +
>> +static inline bool set_sysmmu_active(struct sysmmu_drvdata *data)
>> +{
>> + /* return true if the System MMU was not active previously
>> + and it needs to be initialized */
>> +
>> + data->activations++;
>
> Is this variable only accessed under a lock? If not it should be an
> atomic.
>
All 'sysmmu' functions must be called under a lock because a System MMU
(Samsung's IOMMU) is a shared resource.
I the first patch, I declared the 'data->activations' as atomic_t but
Russell King pointed
that atomic operations on data->activations is not helpful for
preventing to become
a System MMU enabled when its data->activations is 0.
>> + return data->activations == 1;
>
> Is that right? Shouldn't it be 'data->activations > 0'?
>
The value returned by set_sysmmu_active() means "The System MMU must be
initialized".
System MMU is initialized when its state is changed from
'inactive(disabled)' to active(enabled).
If data->activations++ becomes 2 or more, the System MMU is already enabled and
must not be initialized again.
>> +static inline void __set_fault_handler(struct sysmmu_drvdata *data,
>> + int (*handler)(enum S5P_SYSMMU_INTERRUPT_TYPE itype,
>> + unsigned long pgtable_base,
>> + unsigned long fault_addr))
>
> Please typedef the function signature.
>
Ok. thanks.
>> +static int exynos_iommu_domain_init(struct iommu_domain *domain)
>> +{
>> + struct exynos_iommu_domain *priv;
>> +
>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->pgtable = (unsigned long *)__get_free_pages(GFP_KERNEL,
>> + (S5P_LV1TABLE_ENTRIES * sizeof(unsigned long)) >> PAGE_SHIFT);
>
> For __get_free_pages you can't just pass in the number of pages, you
> need the order. Please use get_order.
>
Oh! Thank you.
I didn't notice the problem.
It allocated too large memory so far.
>> + if (!priv->pgtable) {
>> + kfree(priv);
>> + return -ENOMEM;
>> + }
>> +
>> + memset(priv->pgtable, 0, S5P_LV1TABLE_ENTRIES * sizeof(unsigned long));
>> + pgtable_flush(priv->pgtable, priv->pgtable + S5P_LV1TABLE_ENTRIES);
>> +
>> + spin_lock_init(&priv->lock);
>
> No init for the page-table lock?
>
Thank you. I forgot that.
>> +static void exynos_iommu_domain_destroy(struct iommu_domain *domain)
>> +{
>> + struct exynos_iommu_domain *priv = domain->priv;
>> +
>> + free_pages((unsigned long)priv->pgtable,
>> + (S5P_LV1TABLE_ENTRIES * sizeof(unsigned long)) >> PAGE_SHIFT);
>
> Same here, please use get_order.
>
True. I will fix that immediately.
>> +static int __init exynos_iommu_init(void)
>> +{
>> + l2table_cachep = kmem_cache_create("SysMMU Lv2 Tables",
>> + S5P_LV2TABLE_SIZE, S5P_LV2TABLE_SIZE, 0, NULL);
>
> Any reason you allocate a seperate slab-cache? It doesn't have a
> constructor so it would be merged into the kmalloc-slabs anyway (at
> least with SLUB).
>
I found that kmalloc(SZ_1K) returned an address that is not aligned by SZ_1K.
I did not understand why it happened and I changed it to the slab cache.
I will try it again with kmalloc().
Regards,
KyongHo
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
