Message-ID: <20140914202323.GW12361@n2100.arm.linux.org.uk>
Date:	Sun, 14 Sep 2014 21:23:23 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Ankit Jindal <ankit.jindal@...aro.org>
Cc:	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	patches@....com, Rob Herring <robh+dt@...nel.org>,
	"Hans J. Koch" <hjk@...sjkoch.de>,
	Tushar Jagad <tushar.jagad@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 3/5] drivers: uio: Add Xgene QMTM UIO driver

On Tue, Sep 09, 2014 at 03:26:57PM +0530, Ankit Jindal wrote:
> diff --git a/drivers/uio/uio_xgene_qmtm.c b/drivers/uio/uio_xgene_qmtm.c
...
> +/* QMTM CSR read/write routine */
> +static inline void qmtm_csr_write(struct uio_qmtm_dev *qmtm_dev, u32 offset,
> +		u32 data)
> +{
> +	void __iomem *addr = (u8 *)qmtm_dev->info->mem[0].internal_addr +
> +		offset;
> +
> +	writel(data, addr);

Why not...
	void __iomem *base = qmtm_dev->info->mem[0].internal_addr;

	writel(data, addr + offset);

We permit void pointer arithmetic in the kernel.

> +static int qmtm_reset(struct uio_qmtm_dev *qmtm_dev)
> +{
...
> +	/* check whether device is out of reset or not */
> +	do {
> +		val = qmtm_csr_read(qmtm_dev, QMTM_CFG_MEM_RAM_SHUTDOWN);
> +
> +		if (!wait--)
> +			return -1;
> +		udelay(1);
> +	} while (val == 0xffffffff);

There's two points about the above:

1. The loop is buggy.  A correct implementation would:
	- test for the success condition
	- on success, break out of the loop
	- decrement the timeout, and break out of the loop if we have timed out
	- delay the required delay
	- repeat

   Note that this means that we will always check for success before
   deciding whether we've failed or not, /and/ without penalty of an
   unnecessary delay (as you have.)

2. returning -1 here is not on.  This value can (via your probe
   function) be propagated to userspace, where it will be interpreted
   as an -EPERM error.  Wouldn't a proper errno be better?

> +static void qmtm_cleanup(struct platform_device *pdev,
> +		struct uio_qmtm_dev *qmtm_dev)
> +{
> +	struct uio_info *info = qmtm_dev->info;
> +
> +	uio_unregister_device(info);
> +
> +	kfree(info->name);
> +
> +	if (!IS_ERR(info->mem[0].internal_addr))
> +		devm_iounmap(&pdev->dev, info->mem[0].internal_addr);

So what if we hit a failure at

	platform_get_resource(pdev, IORESOURCE_MEM, 0);

in the probe function below, and call this function.  The purpose of the
devm_* stuff is to avoid these kinds of error-cleanup errors by automating
that stuff.  devm_* APIs record the resource allocations against the
device, and when the probe function fails, or the driver is unbound, the
devm_* resources claimed in the probe function are freed.

> +
> +	kfree(info);
> +	clk_put(qmtm_dev->qmtm_clk);
> +	kfree(qmtm_dev);

All of the above, with the exception of uio_unregister_device() and the
missing clk_unprepare_disable() can be left to the devm_* stuff to deal
with, if you'd used the devm_* functions in the probe function.

> +}
> +
> +static int qmtm_probe(struct platform_device *pdev)
> +{
> +	struct uio_info *info;
> +	struct uio_qmtm_dev *qmtm_dev;
> +	struct resource *csr;
> +	struct resource *fabric;
> +	struct resource *qpool;
> +	unsigned int num_queues;
> +	unsigned int devid;
> +	int ret = -ENODEV;

Probably a bad idea to use a standard value.  You probably have some
error codes you've forgotten to propagate.

> +
> +	qmtm_dev = kzalloc(sizeof(struct uio_qmtm_dev), GFP_KERNEL);

devm_kzalloc

> +	if (!qmtm_dev)
> +		return -ENOMEM;
> +
> +	qmtm_dev->info = kzalloc(sizeof(*info), GFP_KERNEL);

devm_kzalloc

> +	if (!qmtm_dev->info) {
> +		kfree(qmtm_dev);

No need for this free.

> +		return -ENOMEM;
> +	}
> +
> +	/* Power on qmtm in case its not done as part of boot-loader */
> +	qmtm_dev->qmtm_clk = clk_get(&pdev->dev, NULL);

devm_clk_get

> +	if (IS_ERR(qmtm_dev->qmtm_clk)) {
> +		dev_err(&pdev->dev, "Failed to get clock\n");
> +		ret = PTR_ERR(qmtm_dev->qmtm_clk);
> +		kfree(qmtm_dev->info);
> +		kfree(qmtm_dev);

Both kfree's can be removed.

> +		return ret;
> +	} else {
> +		clk_prepare_enable(qmtm_dev->qmtm_clk);
> +	}
> +
> +	csr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!csr) {
> +		dev_err(&pdev->dev, "No QMTM CSR resource specified\n");
> +		goto out_free;
> +	}
> +
> +	if (!csr->start) {
> +		dev_err(&pdev->dev, "Invalid CSR resource\n");
> +		goto out_free;
> +	}
> +
> +	fabric = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!fabric) {
> +		dev_err(&pdev->dev, "No QMTM Fabric resource specified\n");
> +		goto out_free;
> +	}
> +
> +	if (!fabric->start) {
> +		dev_err(&pdev->dev, "Invalid Fabric resource\n");
> +		goto out_free;
> +	}
> +
> +	qpool = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	if (!qpool) {
> +		dev_err(&pdev->dev, "No QMTM Qpool resource specified\n");
> +		goto out_free;
> +	}
> +
> +	if (!qpool->start) {
> +		dev_err(&pdev->dev, "Invalid Qpool resource\n");
> +		goto out_free;
> +	}
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "num_queues",
> +			&num_queues);

You may wish to consider checking that you have a pdev->dev.of_node and
cleanly error if you don't.

> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "No num_queues resource specified\n");
> +		goto out_free;
> +	}
> +
> +	/* check whether sufficient memory is provided for the given queues */
> +	if (!((num_queues * QMTM_DEFAULT_QSIZE) <= resource_size(qpool))) {
> +		dev_err(&pdev->dev, "Insufficient Qpool for the given queues\n");
> +		goto out_free;
> +	}
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "devid", &devid);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "No devid resource specified\n");
> +		goto out_free;
> +	}
> +
> +	info = qmtm_dev->info;
> +	info->mem[0].name = "csr";
> +	info->mem[0].addr = csr->start;
> +	info->mem[0].size = resource_size(csr);
> +	info->mem[0].memtype = UIO_MEM_PHYS;
> +	info->mem[0].internal_addr = devm_ioremap_resource(&pdev->dev, csr);
> +
> +	if (IS_ERR(info->mem[0].internal_addr)) {
> +		dev_err(&pdev->dev, "Failed to ioremap CSR region\n");

How about printing the error code, and propagating that error code to your
caller?

> +		goto out_free;
> +	}
> +
> +	info->mem[1].name = "fabric";
> +	info->mem[1].addr = fabric->start;
> +	info->mem[1].size = resource_size(fabric);
> +	info->mem[1].memtype = UIO_MEM_PHYS;
> +
> +	info->mem[2].name = "qpool";
> +	info->mem[2].addr = qpool->start;
> +	info->mem[2].size = resource_size(qpool);
> +	info->mem[2].memtype = UIO_MEM_PHYS_CACHE;
> +
> +	info->name = kasprintf(GFP_KERNEL, "qmtm%d", devid);

devm_kasprintf

> +	info->version = DRV_VERSION;
> +
> +	info->priv = qmtm_dev;
> +	info->open = qmtm_open;
> +	info->release = qmtm_release;
> +
> +	/* get the qmtm out of reset */
> +	ret = qmtm_reset(qmtm_dev);
> +	if (ret < 0)
> +		goto out_free;

Here you propagate that -1 value out of your probe function to userspace.
If you want to do this, please choose a reasonable error code, rather
than -EPERM.

> +
> +	/* register with uio framework */
> +	ret = uio_register_device(&pdev->dev, info);
> +	if (ret < 0)
> +		goto out_free;
> +
> +	dev_info(&pdev->dev, "%s registered as UIO device.\n", info->name);

Does this printk serve a useful purpose?  Most other UIO drivers don't
bother printing anything, though if you do print something, it may be
useful to mention which uio device it is associated with.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists