| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fa686aa41003261433x312453dfgca325cae3bbaf91@mail.gmail.com>
Date: Fri, 26 Mar 2010 15:33:41 -0600
From: Grant Likely <grant.likely@...retlab.ca>
To: "Steven J. Magnani" <steve@...idescorp.com>
Cc: Dan Williams <dan.j.williams@...el.com>,
linux-kernel@...r.kernel.org, microblaze-uclinux@...e.uq.edu.au,
monstr@...str.eu
Subject: Re: [PATCH 3/4] dmaengine: xlldma platform bus driver
On Wed, Mar 17, 2010 at 10:26 AM, Steven J. Magnani
<steve@...idescorp.com> wrote:
> Platform bus attachment for the Xilinx MPMC Soft Direct Memory
> Access Controller DMA engine.
>
> Signed-off-by: Steven J. Magnani <steve@...idescorp.com>
> ---
> diff -uprN a/drivers/dma/xlldma_plat.c b/drivers/dma/xlldma_plat.c
> --- a/drivers/dma/xlldma_plat.c 1969-12-31 18:00:00.000000000 -0600
> +++ b/drivers/dma/xlldma_plat.c 2010-03-17 11:11:16.000000000 -0500
Not really worth putting into a separate file. Just put this hunk in
the xlldma.c file to keep a few more symbols out of the global
namespace.
> @@ -0,0 +1,222 @@
> +/*
> + * Xilinx Multi-Port Memory Controller (MPMC) DMA Engine support
> + *
> + * Copyright (C) 2010 Digital Design Corporation. All rights reserved.
> + *
> + * Author:
> + * Steve Magnani <steve@...idescorp.com>, Mar 2010
> + *
> + * Description:
> + * Platform bus attachment for the Xilinx MPMC Soft Direct Memory
> + * Access Controller DMA engine.
> + *
> + * This is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +
> +#include "xlldma.h"
> +
> +static int __devinit xlldma_plat_chan_probe(struct xlldma_device *xdev)
> +{
> + struct xlldma_chan *chan;
> + struct resource *plat_rsrc;
> + int err;
> + struct platform_device *pdev = to_platform_device(xdev->dev);
> + char irq_name[9];
> +
> + plat_rsrc = platform_get_resource(pdev, IORESOURCE_MEM,
> + xdev->common.chancnt);
> + if (!plat_rsrc)
> + return -ENXIO;
> +
> + /* alloc channel */
> + chan = kzalloc(sizeof(struct xlldma_chan), GFP_KERNEL);
> + if (!chan) {
> + dev_err(xdev->dev,
> + "No free memory for allocating dma channels!\n");
> + return -ENOMEM;
> + }
> +
> + /* get dma channel register base */
> + chan->reg_base = NULL;
Superfluous statement.
> +
> + /* map device registers */
> + chan->reg_base = ioremap(plat_rsrc->start,
> + plat_rsrc->end - plat_rsrc->start + 1);
> + if (chan->reg_base) {
> + dev_dbg(xdev->dev, "MEM base: %p\n", chan->reg_base);
> + } else {
> + dev_err(xdev->dev, "unable to map DMA registers\n");
> + err = -ENXIO;
> + goto err_no_reg;
> + }
General convention is to do this the other way around. No else clause required:
if (!chan->reg_base) {
/* bail out here */
}
dev_dbg(...)
> +
> + chan->xdev = xdev;
> +
> + chan->id = xdev->common.chancnt;
> + if (chan->id >= XLLDMA_MAX_CHANS_PER_DEVICE) {
> + dev_err(xdev->dev, "There is no %d channel!\n", chan->id);
> + err = -EINVAL;
> + goto err_no_chan;
> + }
> + xdev->chan[chan->id] = chan;
> + tasklet_init(&chan->tasklet, xlldma_cleanup_tasklet,
> + (unsigned long)chan);
> +
> + spin_lock_init(&chan->desc_lock);
> + INIT_LIST_HEAD(&chan->desc_queue);
> + INIT_LIST_HEAD(&chan->pending_free);
> +
> + chan->common.device = &xdev->common;
> +
> + /* Add the channel to DMA device channel list */
> + list_add_tail(&chan->common.device_node, &xdev->common.channels);
> + xdev->common.chancnt++;
Don't you need a spinlock protecting the list access?
> +
> + /* RX interrupt is required */
> + sprintf(irq_name, "chan%d-rx", chan->id);
Be defensive. Make irq_name at least 20 bytes long in case chan->id
somehow becomes huge.
> + plat_rsrc = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> + irq_name);
> + if (!plat_rsrc) {
> + dev_err(xdev->dev, "DMA channel %d - missing %s IRQ\n",
> + chan->id, irq_name);
> + err = -ENXIO;
> + goto err_no_irq;
> + }
> +
> + chan->rx_irq = plat_rsrc->start;
> + BUG_ON(plat_rsrc->end != plat_rsrc->start);
> +
> + err = request_irq(chan->rx_irq, &xlldma_chan_rx_interrupt,
> + IRQF_SHARED, "xlldma-rx", chan);
> + if (err) {
> + dev_err(xdev->dev, "request_irq error on %s, return %d\n",
> + irq_name, err);
> + err = -ENXIO;
> + goto err_no_irq;
> + }
> +
> + /* TX interrupt is optional - only needed for DMA_INTERRUPT */
> + irq_name[6] = 't';
And if irq # is in the double digits? This is clever, but unclear and
potentially risky. Just do another sprintf()
> + plat_rsrc = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> + irq_name);
> + if (plat_rsrc) {
> + chan->tx_irq = plat_rsrc->start;
> + BUG_ON(plat_rsrc->end != plat_rsrc->start);
> + } else {
> + chan->tx_irq = NO_IRQ;
> + }
put chan->tx_irq = NO_IRQ before the if(), and you can drop the else
statement (a bit simpler).
> +
> + if (chan->tx_irq != NO_IRQ) {
> + err = request_irq(chan->tx_irq, &xlldma_chan_tx_interrupt,
> + IRQF_SHARED, "xlldma-tx", chan);
> + if (err) {
> + dev_err(xdev->dev,
> + "request_irq error on %s, return %d\n",
> + irq_name, err);
> + goto err_rx_irq;
> + }
> + }
> +
> + dev_info(xdev->dev, "#%d, tx irq %d, rx irq %d\n", chan->id,
> + chan->tx_irq, chan->rx_irq);
> +
> + return 0;
> +err_rx_irq:
> + free_irq(chan->rx_irq, chan);
> +err_no_irq:
> + list_del(&chan->common.device_node);
> +err_no_chan:
> + iounmap(chan->reg_base);
> +err_no_reg:
> + kfree(chan);
> + return err;
> +}
> +
> +static int __devinit xlldma_plat_probe(struct platform_device *pdev)
> +{
> + struct xlldma_device *xdev;
> + int i, rc;
> +
> + dev_info(&pdev->dev, "Xilinx DMA engine...\n");
> +
> + xdev = kzalloc(sizeof(struct xlldma_device), GFP_KERNEL);
> + if (!xdev) {
> + dev_err(&pdev->dev, "Not enough memory for 'priv'\n");
> + rc = -ENOMEM;
> + goto fail_alloc;
> + }
> + xdev->dev = &pdev->dev;
> + INIT_LIST_HEAD(&xdev->common.channels);
> +
> + xdev->common.dev = &pdev->dev;
> + platform_set_drvdata(pdev, xdev);
> +
> + do {
> + rc = xlldma_plat_chan_probe(xdev);
> + } while (rc == 0);
> +
> + if (xdev->common.chancnt == 0)
> + goto fail_nochan;
> +
> + rc = xlldma_device_setup(xdev->dev);
> + if (rc == 0)
> + return 0;
> +
> +/* fail: */
> + for (i = 0; i < XLLDMA_MAX_CHANS_PER_DEVICE; i++) {
> + if (xdev->chan[i])
> + xlldma_chan_remove(xdev->chan[i]);
> + }
> +fail_nochan:
> + platform_set_drvdata(pdev, NULL);
> + kfree(xdev);
> +
> +fail_alloc:
> + return rc;
> +}
> +
> +static int __devexit xlldma_plat_remove(struct platform_device *pdev)
> +{
> + return xlldma_device_teardown(&pdev->dev);
> +}
> +
> +static struct platform_driver xlldma_driver = {
> + .probe = xlldma_plat_probe,
> + .remove = __devexit_p(xlldma_plat_remove),
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "xlldma",
> + },
> +};
> +
> +static int __init xlldma_plat_init(void)
> +{
> + int ret;
> +
> + pr_info("Xilinx LocalLink DMA driver\n");
> +
> + ret = platform_driver_register(&xlldma_driver);
> + if (ret)
> + pr_err("xlldma: failed to register platform driver\n");
> +
> + return ret;
> +}
Don't bother with all this. sysfs will tell you whether or not the
driver got registered. Do this instead:
+static int __init xlldma_plat_init(void)
+{
+ return platform_driver_register(&xlldma_driver);
+}
> +
> +static void __exit xlldma_plat_exit(void)
> +{
> + platform_driver_unregister(&xlldma_driver);
> +}
> +
> +module_init(xlldma_plat_init);
Put the module_init directly after the xlldma_plat_init() function definition.
> +module_exit(xlldma_plat_exit);
> +
> +MODULE_AUTHOR("Digital Design Corporation");
> +MODULE_DESCRIPTION("Xilinx LocalLink DMA Platform driver");
> +MODULE_LICENSE("GPL");
>
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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