Re: [PATCH 3/5] drivers: uio: Add Xgene QMTM UIO driver

[Ankit Jindal]

On 15 September 2014 01:53, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> 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.
>
Thanks, will incorporate all the suggested changes in next version.

Thanks,
Ankit
--
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/