Re: [PATCH 4/6] omap iommu: simple virtual address space management

[Russell King - ARM Linux]

On Fri, Jan 16, 2009 at 10:37:25AM +0200, Hiroshi DOYU wrote:
> +static struct iovm_struct *__find_iovm_area(struct iommu *obj, const void *_da)
> +{
> +	struct iovm_struct *tmp;
> +	u32 da = (u32)_da;
...

> +static struct iovm_struct *alloc_iovm_area(struct iommu *obj, void *da,
> +					   size_t bytes, u32 flags)
> +{
> +	struct iovm_struct *new, *tmp;
> +	u32 start, prev_end, alignement;
> +
> +	if (!obj || !bytes)
> +		return ERR_PTR(-EINVAL);
> +
> +	start = (u32)da;
...

> +/* template function for all unmapping */
> +static struct sg_table *unmap_vm_area(struct iommu *obj, const void *da,
> +				      void (*fn)(const void *), u32 flags)
> +{
> +	struct sg_table *sgt = NULL;
> +	struct iovm_struct *area;
> +
> +	BUG_ON(in_interrupt());
> +
> +	if (!IS_ALIGNED((u32)da, PAGE_SIZE)) {
...etc

I've been wondering whether 'void *' is the right type for the device
address, and I've pretty much come to the conclusion that it isn't.
'void *' is a pointer, so one might expect it to point at memory.  It
may point at memory, just not on the host CPU.  So using a pointer
seems wrong to me.

I know that screws up error handling in the code somewhat since you're
using IS_ERR and friends - but have you also considered whether a
valid device address falls within the space used for error values
(iow, small negative numbers)?

Lastly, I'm not entirely sold on the four different interfaces and
whether they're the right approach.  I've not yet seen users of each
of these interfaces so I don't think I can really comment yet - but
I think it's important to have some examples before acking this patch.
--
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/