Re: [PATCH v6 2/2] iommu/exynos: Add iommu driver for Exynos Platforms

[KyongHo Cho]

On Tue, Nov 15, 2011 at 3:05 PM, Kyungmin Park <kmpark@xxxxxxxxxxxxx> wrote:
>> +static 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++;
>> +     return data->activations == 1;
>> +}
> If it calls the twice, then caller get the active failed return value.
> Are there no case to call the multiple activation?

Return value of set_sysmmu_active() does not mean that activation is successful
but that System MMU H/W is needed to be initialized.

System MMU driver only initializes System MMU H/W when data->activation
becomes 1 from 0 and deinitializes when it becomes 0 from 1.

>> +void exynos_sysmmu_set_tablebase_pgd(struct device *owner, unsigned long
>> pgd)
>> +{
>> +     struct sysmmu_drvdata *data = NULL;
>> +
>> +     while ((data = get_sysmmu_data(owner, data))) {
>> +             unsigned long flags;
>> +
>> +             read_lock_irqsave(&data->lock, flags);
> It should be 'write_lock_irqsave'

data->lock just protects contents of 'data' from race condition.
It does not care about control registers of System MMU.
Since no contents in 'data' is modified in this function,
I think read_lock is more proper.

However, suddenly, I think exynos_sysmmu_set_tablebase_pgd()
must be removed because it modifies page table base
that is already set with exynos_sysmmu_enable().

>> +
>> +             if (is_sysmmu_active(data)) {
>> +                     sysmmu_block(data->sfrbase);
>> +                     __sysmmu_set_ptbase(data->sfrbase, pgd);
>> +                     sysmmu_unblock(data->sfrbase);
>> +                     dev_dbg(data->dev, "New page table base is %p\n",
>> +                                                             (void *)pgd);
>> +             } else {
>> +                     dev_dbg(data->dev,
>> +                     "Disabled: Skipping setting page table base.\n");
>> +             }
>> +
>> +             read_unlock_irqrestore(&data->lock, flags);
> It should be 'write_unlock_irqrestore'

Ditto.

>> +void exynos_sysmmu_tlb_invalidate(struct device *owner)
>> +{
>> +     struct sysmmu_drvdata *data = NULL;
>> +
>> +     while ((data = get_sysmmu_data(owner, data))) {
>> +             unsigned long flags;
>> +
>> +             read_lock_irqsave(&data->lock, flags);
> doesn't use the write_lock_irqsave here?

Ditto.

>> +static void exynos_iommu_domain_destroy(struct iommu_domain *domain)
>> +{
>> +     struct exynos_iommu_domain *priv = domain->priv;
>> +     struct list_head *pos, *n;
>> +
>> +     WARN_ON(!list_empty(&priv->clients));
>> +
>> +     spin_lock(&priv->lock);
>> +
>> +     list_for_each_safe(pos, n, &priv->clients) {
>> +             struct iommu_client *client;
>> +
>> +             client = list_entry(pos, struct iommu_client, node);
>> +             exynos_sysmmu_disable(client->dev);
> I think It occurs sleeping lock error message since it calls the
> write_lock within spin_lock.

Sorry, I don't understand why it makes problem.

I also tested it with various devices.
It is also not a condition of neither deadlock nor livelock.

Thank you.

KyongHo.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/