Re: [PATCH 1/6] omap iommu: tlb and pagetable primitives

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

A few more simple comments...

On Fri, Jan 16, 2009 at 10:37:09AM +0200, Hiroshi DOYU wrote:
> +struct iommu_platform_data {
> +	char	*name;

const?

> +	char	*clk_name;

const?

> +	int	nr_tlb_entries;
> +};

...

> +int install_iommu_arch(const struct iommu_functions *ops)
> +{
> +	if (arch_iommu)
> +		return -EBUSY;
> +
> +	arch_iommu = ops;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(install_iommu_arch);

Exported functions should have some documentation in the standard kernel
documentation format.  See Documentation/kernel-doc-nano-HOWTO.txt

> +static int iommu_enable(struct iommu *obj)
> +{
> +	int err;
> +
> +	BUG_ON(!arch_iommu || !arch_iommu->enable);

Are these run-time BUG_ON checks really worth it?  BUG_ON is effectively:

	if (condition_is_true)
		cause_a_null_pointer_dereference;

So, if you're checking for pointers you're about dereference being NULL,
there's little point - dereferencing the when they're NULL will cause an
oops, and you won't have the overhead of the runtime tests checking them
for NULL.  Ditto elsewhere in this file.

> +static u32 *iopte_alloc(struct iommu *obj, u32 *iopgd, u32 da)
> +{
> +	u32 *iopte;
> +
> +	/* a table has already existed */
> +	if (*iopgd)
> +		goto pte_ready;
> +
> +	/*
> +	 * do the allocation outside the page table lock
> +	 */
> +	spin_unlock(&obj->page_table_lock);
> +	iopte = kmem_cache_zalloc(iopte_cachep, GFP_KERNEL);
> +	spin_lock(&obj->page_table_lock);
> +
> +	if (!*iopgd) {
> +		if (!iopte)
> +			return ERR_PTR(-ENOMEM);
> +
> +		*iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
> +		flush_iopgd_range(iopgd, iopgd);
> +
> +#ifdef DEBUG_VERBOSE
> +		dev_dbg(obj->dev, "%s:\ta new pte:%p\n", __func__, iopte);
> +#endif

Not sure wrapping these in DEBUG_VERBOSE is necessary.  dev_dbg() is a
no-op unless DEBUG is defined.

> +#ifdef DEBUG
> +static void dump_tlb_entries(struct iommu *obj)
> +{
> +	int i;
> +	struct iotlb_lock l;
> +
> +	clk_enable(obj->clk);
> +
> +	pr_info("%8s %8s\n", "cam:", "ram:");
> +	pr_info("-----------------------------------------\n");
> +
> +	for (i = 0; i < obj->nr_tlb_entries; i++) {
> +		struct cr_regs cr;
> +		static char buf[4096];
> +
> +		iotlb_lock_get(obj, &l);
> +		l.vict = i;
> +		iotlb_lock_set(obj, &l);
> +		iotlb_read_cr(obj, &cr);
> +		if (!iotlb_cr_valid(&cr))
> +			continue;
> +
> +		memset(buf, 0, 4096);
> +		iotlb_dump_cr(obj, &cr, buf);
> +		pr_info("%s", buf);

Hmm.  You call this in relation to an error, but you print everything at
'info' level.  Are you sure that's correct?

> +/*
> + *	OMAP Device MMU(IOMMU) detection
> + */
> +static int __devinit omap_iommu_probe(struct platform_device *pdev)
> +{
> +	int err = -ENODEV;
> +	void *p;
> +	int irq;
> +	struct iommu *obj;
> +	struct resource *res;
> +	struct iommu_platform_data *pdata = pdev->dev.platform_data;
> +
> +	if (pdev->num_resources != 2)
> +		return -EINVAL;
> +
> +	obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
> +	if (!obj)
> +		return -ENOMEM;
> +
> +	obj->clk = clk_get(NULL, pdata->clk_name);

Avoid passing a NULL struct device except when you have absolutely no other
choice.  &pdev->dev looks sensible.  By doing this, it also provides us with
a path to fixing the OMAP clk API implementation without having to fix lots
of drivers.