Re: [RFC][PATCH 2/2] Example to show use of uio pgprot

["Hans J. Koch" <hjk@hansjkoch.de>]

On Fri, Oct 28, 2011 at 10:50:30AM -0500, Kumar Gala wrote:

A few remarks below.

> +static void __init dpa_uio_portal_init(struct dpa_uio_portal *p,
> +				const struct dpa_uio_class *c)

This can't be "void". You have to return apropiate errors.

> +{
> +	struct dpa_uio_info *info;
> +	const struct resource *res;
> +	u32 index;
> +	int irq, ret;
> +
> +	/* allocate 'info' */
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return;

return -ENOMEM (more similar cases below)

> +	atomic_set(&info->ref, 1);
> +	if (p->type == dpa_uio_portal_bman) {
> +		res = &p->bm_cfg->addr_phys[0];
> +		index = p->bm_cfg->public_cfg.index;
> +		irq = p->bm_cfg->public_cfg.irq;
> +	} else {
> +		res = &p->qm_cfg->addr_phys[0];
> +		index = p->qm_cfg->public_cfg.index;
> +		irq = p->qm_cfg->public_cfg.irq;
> +	}
> +	/* We need to map the cache-inhibited region in the kernel for
> +	 * interrupt-handling purposes. */
> +	info->addr_ci = ioremap_prot(res[DPA_PORTAL_CI].start,
> +				resource_size(&res[DPA_PORTAL_CI]),
> +				_PAGE_GUARDED | _PAGE_NO_CACHE);
> +	/* Name the UIO device according to the cell-index. It's supposed to be
> +	 * unique for each device class (Qman/Bman), and is also a convenient
> +	 * way for user-space to find the UIO device that corresponds to a given
> +	 * portal device-tree node. */
> +	sprintf(info->name, "%s%x", c->dev_prefix, index);
> +	info->pdev = platform_device_alloc(info->name, -1);
> +	if (!info->pdev) {
> +		iounmap(info->addr_ci);
> +		kfree(info);
> +		pr_err("dpa_uio_portal: platform_device_alloc() failed\n");
> +		return;
> +	}
> +	ret = platform_device_add(info->pdev);
> +	if (ret) {
> +		platform_device_put(info->pdev);
> +		iounmap(info->addr_ci);
> +		kfree(info);
> +		pr_err("dpa_uio_portal: platform_device_add() failed\n");
> +		return;
> +	}
> +	info->uio.name = info->name;
> +	info->uio.version = dpa_uio_version;
> +	info->uio.mem[DPA_PORTAL_CE].name = "cena";
> +	info->uio.mem[DPA_PORTAL_CE].addr = res[DPA_PORTAL_CE].start;
> +	info->uio.mem[DPA_PORTAL_CE].size = resource_size(&res[DPA_PORTAL_CE]);
> +	info->uio.mem[DPA_PORTAL_CE].memtype = UIO_MEM_PHYS;
> +	info->uio.mem[DPA_PORTAL_CI].name = "cinh";
> +	info->uio.mem[DPA_PORTAL_CI].addr = res[DPA_PORTAL_CI].start;
> +	info->uio.mem[DPA_PORTAL_CI].size = resource_size(&res[DPA_PORTAL_CI]);
> +	info->uio.mem[DPA_PORTAL_CI].memtype = UIO_MEM_PHYS;
> +	info->uio.irq = irq;
> +	info->uio.handler = dpa_uio_irq_handler;
> +	info->uio.set_pgprot = dpa_uio_pgprot;
> +	info->uio.open = dpa_uio_open;
> +	info->uio.release = dpa_uio_release;
> +	ret = uio_register_device(&info->pdev->dev, &info->uio);

This should be the last thing you do before return. You can have
interrupts even before uio_register_device returns.

> +	if (ret) {
> +		platform_device_del(info->pdev);
> +		platform_device_put(info->pdev);
> +		iounmap(info->addr_ci);
> +		kfree(info);
> +		pr_err("dpa_uio_portal: UIO registration failed\n");
> +		return;
> +	}
> +	list_add_tail(&info->node, &uio_portal_list);

That should probably be done prior to uio_register_device.

> +	pr_info("USDPAA portal initialised, %s\n", info->name);
> +}
> +
> +static int __init dpa_uio_init(void)
> +{
> +	const struct dpa_uio_class *classes[3], **c = classes;
> +	classes[0] = dpa_uio_bman();
> +	classes[1] = dpa_uio_qman();
> +	classes[2] = NULL;
> +	while (*c) {
> +		struct dpa_uio_portal *p;
> +		list_for_each_entry(p, &(*c)->list, node)
> +			dpa_uio_portal_init(p, *c);
> +		c++;
> +	}
> +	pr_info("USDPAA portal layer loaded\n");
> +	return 0;

You can't just return OK here if dpa_uio_portal_init() fails.

> +}
> +
> +static void __exit dpa_uio_exit(void)
> +{
> +	struct dpa_uio_info *info, *tmp;
> +	list_for_each_entry_safe(info, tmp, &uio_portal_list, node) {
> +		list_del(&info->node);
> +		uio_unregister_device(&info->uio);
> +		platform_device_del(info->pdev);
> +		platform_device_put(info->pdev);
> +		iounmap(info->addr_ci);
> +		pr_info("USDPAA portal removed, %s\n", info->name);
> +		kfree(info);
> +	}
> +	pr_info("USDPAA portal layer unloaded\n");
> +}
> +
> +
> +module_init(dpa_uio_init)
> +module_exit(dpa_uio_exit)
> +MODULE_LICENSE("GPL");
> +
> -- 
> 1.7.3.4
> 
>