Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>]

Hi Santosh,

On 03/15/2014 12:54 PM, Santosh Shilimkar wrote:
> On Friday 14 March 2014 09:49 PM, Suman Anna wrote:
>> Hi Santosh, Laurent, Russell, Arnd,
>>
>> On 03/14/2014 12:51 PM, Santosh Shilimkar wrote:
>>> On Friday 14 March 2014 12:38 PM, Laurent Pinchart wrote:
>>>> Hi Santosh,
>>>>
>>>> On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
>>>>> + Russell, Arnd
>>>>>
>>>>> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
>>>>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>>>>>> The page table entries must be cleaned from the cache before being
>>>>>>> accessed by the IOMMU. Instead of implementing cache management manually
>>>>>>> (and ignoring L2 cache), use clean_dcache_area() to make sure the
>>>>>>> entries are visible to the device.
>>>>>>
>>>>>> Thanks for fixing this, this has been long pending. There have been some
>>>>>> previous attempts at this as well by Ramesh Gupta, with the last thread
>>>>>> (a long time now) being
>>>>>> http://marc.info/?t=134752035400001&r=1&w=2
>>>>>>
>>>>>> Santosh,
>>>>>> Can you please take a look at this patch and provide your comments?
>>>>>>
>>>>>> regards
>>>>>> Suman
>>>>>>
>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>>>>>> ---
>>>>>>>
>>>>>>>     drivers/iommu/omap-iommu.c | 41 ++++++++++-----------------------------
>>>>>>>     1 file changed, 10 insertions(+), 31 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>>>>>> index a893eca..bb605c9 100644
>>>>>>> --- a/drivers/iommu/omap-iommu.c
>>>>>>> +++ b/drivers/iommu/omap-iommu.c
>>>>>>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
>>>>>>>     /*
>>>>>>>      *    H/W pagetable operations
>>>>>>>      */
>>>>>>> -static void flush_iopgd_range(u32 *first, u32 *last)
>>>>>>> +static void flush_pgtable(void *addr, size_t size)
>>>>>>>     {
>>>>>>> -    /* FIXME: L2 cache should be taken care of if it exists */
>>>>>>> -    do {
>>>>>>> -        asm("mcr    p15, 0, %0, c7, c10, 1 @ flush_pgd"
>>>>>>> -            : : "r" (first));
>>>>>>> -        first += L1_CACHE_BYTES / sizeof(*first);
>>>>>>> -    } while (first <= last);
>>>>>>> -}
>>>>>>> -
>>>>>>> -static void flush_iopte_range(u32 *first, u32 *last)
>>>>>>> -{
>>>>>>> -    /* FIXME: L2 cache should be taken care of if it exists */
>>>>>>> -    do {
>>>>>>> -        asm("mcr    p15, 0, %0, c7, c10, 1 @ flush_pte"
>>>>>>> -            : : "r" (first));
>>>>>>> -        first += L1_CACHE_BYTES / sizeof(*first);
>>>>>>> -    } while (first <= last);
>>>>>>> +    clean_dcache_area(addr, size);
>>>>>
>>>>> I remember NAKing this approach in past and my stand remains same.
>>>>> The cache APIs which you are trying to use here are not suppose
>>>>> to be used outside.
>>
>> So this particular change has a long history (have to dig through to educate myself). I have not seen clean_dcache_area attempted before, and I wasn't sure myself it it can be used here. Ramesh and Fernando definitely did start out with dmac_flush_range and outer_flush_range which was definitely nacked [1].
>>
> OK. Please wrap the lines appropriately while replying.

Hmm, weird. I don't see this with my other responses. But sorry for
the trouble.

>
>>>>
>>>> Please note that the omap-iommu driver already uses clean_dcache_area().
>>>> That's where I got the idea :-)
>>>>
>>> So that fall through the cracks then ;-)
>>>
>>>>> I think the right way to fix this is to make use of streaming APIs.
>>>>> If needed, IOMMU can have its own dma_ops for special case
>>>>> handling if any.
>>>>
>>>> I can replace clean_dcache_area() with dma_map_page() as done by the arm-smmu
>>>> driver. If that's fine I'll modify this patch accordingly in v2.
>>>>
>>
>> Ramesh had also attempted to use dma_page_single() previously [2], and Russell has instead suggested to extend the cpu cache operations [3].
>> Ramesh had worked based on this suggestion and the series reached v6 [4] (same as link I mentioned above) and this is the last attempt on this, but the thread went silent.
>>
>> I am wondering if that is still valid and is the right way to go, as this seems to be a common problem. I do see dmac_flush_range being used for similar purposes in msm_iommu.c and exynos-iommu.c, so looks like they also fell through the cracks.
>>
> Thanks for the links. I think you should revive the v6 series unless and until
> RMK has some other suggestion. This can also help to remove the wrong usages
> from other IOMMU drivers as you pointed out.

OK, will do.

regards
Suman

>
>> Laurent,
>> Can you drop this patch out from v2 so that it is not clubbed with the small cleanup patches, and we can track this separately?
>>
> Agree
>
> Regards,
> Santosh
>
>>
>> [1] http://marc.info/?l=linux-omap&m=129907009019355&w=2
>> [2] http://marc.info/?t=130281804900005&r=1&w=2
>> [3] http://marc.info/?l=linux-omap&m=131310179423214&w=2
>> [4] http://marc.info/?t=134752035400001&r=1&w=2
>>
>>
>