[RFC 1/6] omap: iommu: generic iommu api migration

Hi Ohad,

On Tuesday 07 June 2011 13:19:05 Ohad Ben-Cohen wrote:

On Tue, Jun 7, 2011 at 12:22 PM, Laurent Pinchart wrote:

quoted

quoted

+     BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));

Either __get_free_pages() guarantees that the allocated memory will be
aligned on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is
unnecessary, or doesn't offer such guarantee, in which case the BUG_ON()
will oops randomly.

Curious, does it oops randomly today ?
(i just copied this from omap_iommu_probe, where it always existed).

No that I know of :-)

It is a bit ugly though, and thinking on it again, 16KB is not that
big. We can just use kmalloc here, which does ensure the alignment
(or, better yet, kzalloc, and then ditch the memset).

quoted

In both cases BUG_ON() should probably be avoided.

I disagree; we must check this so user data won't be harmed (hardware
requirement), and if a memory allocation API fails to meet its
requirements - that's really bad and user data is again at stake (much
more will break, not only the iommu driver).

My point is that if the allocator guarantees the alignment (not as a side 
effect of the implementation, but per its API) there's no need to check it 
again. As the alignement is required, we need an allocator that guarantees it 
anyway.

quoted

This leaks omap_domain->pgtable.

The free_pages() call in omap_iommu_remove() should be removed, as
omap_iommu_probe() doesn't allocate the pages table anymore.

thanks !

quoted

You can also remove the the struct iommu::iopgd field.

No, I can't; it's used when the device is attached to an address space
domain.

Right, my bad.

quoted

You return 0 in the bogus pte/pgd cases. Is that intentional ?

Yes, that's probably the most (if any) reasonable value to return here
(all other iommu implementations are doing so too).

-- 
Regards,

Laurent Pinchart