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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ideasonboard.com>
> > ---
> > 
> >   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 ?

> >   	/* 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.

-- 
Regards,

Laurent Pinchart