Re: [PATCH 5/5] iommu/omap: Fix map protection value handling

From: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
To: Laurent Pinchart
	<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
	linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	sakari.ailus-X3B1VOXEql0@public.gmane.org,
	Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
Subject: Re: [PATCH 5/5] iommu/omap: Fix map protection value handling
Date: Fri, 14 Mar 2014 19:16:53 -0500	[thread overview]
Message-ID: <53239BF5.1000006@ti.com> (raw)
In-Reply-To: <13414141.7HHPA8i3Vs@avalon>

Hi Laurent,

On 03/14/2014 04:46 AM, Laurent Pinchart wrote:
> Hi Suman,
>
> Thank you for the review.
>
> On Thursday 13 March 2014 19:07:33 Suman Anna wrote:
>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>> The prot flags passed to the IOMMU map handler are defined in
>>> include/linux/iommu.h as IOMMU_(READ|WRITE|CACHE|EXEC). However, the
>>> driver expects to receive MMU_RAM_* OMAP-specific flags. This causes
>>> IOMMU flags being interpreted as page sizes, leading to failures.
>>>
>>> Hardcode the OMAP mapping parameters to little-endian, 8-bits and
>>> non-mixed page attributes. Furthermore, as the OMAP IOMMU doesn't
>>> support read-only or write-only mappings, ignore the prot value.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>> ---
>>>
>>>    drivers/iommu/omap-iommu.c | 17 +++++++----------
>>>    1 file changed, 7 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>> index 59de3fc..9f7f1d4 100644
>>> --- a/drivers/iommu/omap-iommu.c
>>> +++ b/drivers/iommu/omap-iommu.c
>>> @@ -1016,8 +1016,7 @@ static void iopte_cachep_ctor(void *iopte)
>>>    	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
>>>    }
>>>
>>> -static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
>>> -				   u32 flags)
>>> +static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa, int
>>> pgsz)
>>>    {
>>>    	memset(e, 0, sizeof(*e));
>>>
>>> @@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e,
>>> u32 da, u32 pa,
>>>    	e->pa		= pa;
>>>    	e->valid	= 1;
>>
>> As I was looking through this, I found a small bug here. This is unrelated
>> to this patch, but you can fix it in the same patch. The e->valid value is
>> directly used in omap2_alloc_cr, so it should be initialized to MMU_CAM_V.
>> It is not a problem currently as PREFETCH_IOTLB is not used, if used, this
>> is overriding the size bit-fields when it prefetches the entry, which may
>> lead to an MMU fault.
>
> Good catch. As it's a separate issue, do you mind if I fix that in a separate
> patch in v2 ?

I actually have couple of minor cleanup patches as well. I will send 
them out and you can include them with your series when you post v2.

>
>>>    	/* FIXME: add OMAP1 support */
>>>
>>> -	e->pgsz		= flags & MMU_CAM_PGSZ_MASK;
>>> -	e->endian	= flags & MMU_RAM_ENDIAN_MASK;
>>> -	e->elsz		= flags & MMU_RAM_ELSZ_MASK;
>>> -	e->mixed	= flags & MMU_RAM_MIXED_MASK;
>>> +	e->pgsz		= pgsz;
>>> +	e->endian	= MMU_RAM_ENDIAN_LITTLE;
>>> +	e->elsz		= MMU_RAM_ELSZ_8;
>>> +	e->mixed	= 0;
>>
>> Thanks, the change of these settings looks good overall. The only ones for
>> which it may apply anyway is just the Camera in OMAP2/OMAP3 (as per TRM,
>> endianness conversion supported only on writes anyway, so I am assuming that
>> we never use the combination), and OMAP2420 DSP, for which there is no
>> driver currently nor is the MMU being instantiated. All other processors are
>> pure little endian, and they even ignore the values written in these fields.
>>
>> Please recheck the Camera subsystem once, and see if the e->elsz should be
>> set as MMU_RAM_ELSZ_NONE (I am not too familiar with the Camera usage, so
>> you are probably in a better place than me).
>
> I've tested MMU_RAM_ELSZ_NONE and it works fine. I'll update the patch in v2.
>
Thanks for checking and confirming this.

regards
Suman