| 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
| ||
|
Date: Mon, 13 Oct 2008 18:55:43 +0530
From: "Madhusudhan Chikkature" <madhu.cr@...com>
To: "Andrew Morton" <akpm@...ux-foundation.org>,
"Gadiyar, Anand" <gadiyar@...com>
Cc: <johnpol@....mipt.ru>, <linux-kernel@...r.kernel.org>,
<linux-omap@...r.kernel.org>
Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
----- Original Message -----
From: "Andrew Morton" <akpm@...ux-foundation.org>
To: "Gadiyar, Anand" <gadiyar@...com>
Cc: <johnpol@....mipt.ru>; <linux-kernel@...r.kernel.org>; <linux-omap@...r.kernel.org>; <madhu.cr@...com>
Sent: Saturday, October 11, 2008 2:08 AM
Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
> On Wed, 8 Oct 2008 12:46:25 +0530
> "Gadiyar, Anand" <gadiyar@...com> wrote:
>
>> From: Madhusudhan Chikkature <madhu.cr@...com>
>>
>> The HDQ/1-Wire module of TI OMAP2430/3430 platforms implement the hardware
>> protocol of the master functions of the Benchmark HDQ and the Dallas
>> Semiconductor 1-Wire protocols. These protocols use a single wire for
>> communication between the master (HDQ/1-Wire controller) and the slave
>> (HDQ/1-Wire external compliant device).
>>
>> This patch provides the HDQ driver to suppport TI OMAP2430/3430 platforms.
>
> Every tab character in all five patches was converted to eight-spaces by
> your email client. Please fix the mailer and resend everything.
>
>> +++ linux-2.6/drivers/w1/masters/omap_hdq.c 2008-09-26 14:28:36.000000000 +0530
>> @@ -0,0 +1,730 @@
>> +/*
>> + * drivers/w1/masters/omap_hdq.c
>> + *
>> + * Copyright (C) 2007 Texas Instruments, Inc.
>> + *
>> + * This file is licensed under the terms of the GNU General Public License
>> + * version 2. This program is licensed "as is" without any warranty of any
>> + * kind, whether express or implied.
>> + *
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <asm/irq.h>
>> +#include <mach/hardware.h>
>
> We conventionally put a blank line between the linux/ includes and the
> asm/ includes.
>
>> +static int omap_hdq_get(struct hdq_data *hdq_data);
>> +static int omap_hdq_put(struct hdq_data *hdq_data);
>> +static int omap_hdq_break(struct hdq_data *hdq_data);
>
> These three aren't strictly needed, because these functions are defined
> before first use. I think it's best to not declare them.
>
>> +static int hdq_wait_for_flag(struct hdq_data *hdq_data, u32 offset,
>> + u8 flag, u8 flag_set, u8 *status)
>> +{
>> + int ret = 0;
>> + unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT;
>> +
>> + if (flag_set == OMAP_HDQ_FLAG_CLEAR) {
>> + /* wait for the flag clear */
>> + while (((*status = hdq_reg_in(hdq_data, offset)) & flag)
>> + && time_before(jiffies, timeout)) {
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(1);
>
> Use schedule_timeout_uninterruptible(1)
>
>> + }
>> + if (*status & flag)
>> + ret = -ETIMEDOUT;
>> + } else if (flag_set == OMAP_HDQ_FLAG_SET) {
>> + /* wait for the flag set */
>> + while (!((*status = hdq_reg_in(hdq_data, offset)) & flag)
>> + && time_before(jiffies, timeout)) {
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(1);
>
> elsewhere..
>
>> + }
>> + if (!(*status & flag))
>> + ret = -ETIMEDOUT;
>> + } else
>> + return -EINVAL;
>> +
>> + return ret;
>> +}
>> +
>> +/* write out a byte and fill *status with HDQ_INT_STATUS */
>> +static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
>> +{
>> + int ret;
>> + u8 tmp_status;
>> + unsigned long irqflags;
>> +
>> + *status = 0;
>> +
>> + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
>> + /* clear interrupt flags via a dummy read */
>> + hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
>> + /* ISR loads it with new INT_STATUS */
>> + hdq_data->hdq_irqstatus = 0;
>> + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
>> +
>> + hdq_reg_out(hdq_data, OMAP_HDQ_TX_DATA, val);
>> +
>> + /* set the GO bit */
>> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, OMAP_HDQ_CTRL_STATUS_GO,
>> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO);
>> + /* wait for the TXCOMPLETE bit */
>> + ret = wait_event_interruptible_timeout(hdq_wait_queue,
>> + hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT);
>> + if (ret < 0) {
>> + dev_dbg(hdq_data->dev, "wait interrupted");
>> + return -EINTR;
>> + }
>
> Is this desirable? The user hits ^C and the driver bails out?
>
> I assume so, but was this tested?
Andrew, What is the test scenario you mean here? A user hitting ^C when the driver is waiting for the TXCOMPLETE bit?
>
>> + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
>> + *status = hdq_data->hdq_irqstatus;
>> + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
>
> It's unusual to put a lock around a single atomic move instruction.
>
>> + /* check irqstatus */
>> + if (!(*status & OMAP_HDQ_INT_STATUS_TXCOMPLETE)) {
>> + dev_dbg(hdq_data->dev, "timeout waiting for"
>> + "TXCOMPLETE/RXCOMPLETE, %x", *status);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + /* wait for the GO bit return to zero */
>> + ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS,
>> + OMAP_HDQ_CTRL_STATUS_GO,
>> + OMAP_HDQ_FLAG_CLEAR, &tmp_status);
>> + if (ret) {
>> + dev_dbg(hdq_data->dev, "timeout waiting GO bit"
>> + "return to zero, %x", tmp_status);
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>>
>> ...
>>
>> +/* Issue break pulse to the device */
>> +static int omap_hdq_break(struct hdq_data *hdq_data)
>> +{
>> + int ret;
>> + u8 tmp_status;
>> + unsigned long irqflags;
>> +
>> + ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
>> + if (ret < 0)
>> + return -EINTR;
>> +
>> + if (!hdq_data->hdq_usecount) {
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> + return -EINVAL;
>> + }
>> +
>> + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
>> + /* clear interrupt flags via a dummy read */
>> + hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
>> + /* ISR loads it with new INT_STATUS */
>> + hdq_data->hdq_irqstatus = 0;
>> + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
>> +
>> + /* set the INIT and GO bit */
>> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
>> + OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
>> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
>> + OMAP_HDQ_CTRL_STATUS_GO);
>> +
>> + /* wait for the TIMEOUT bit */
>> + ret = wait_event_interruptible_timeout(hdq_wait_queue,
>> + hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT);
>> + if (ret < 0) {
>> + dev_dbg(hdq_data->dev, "wait interrupted");
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> + return -EINTR;
>
> Multiple-return-statements-per-function are a common source of
> maintenance problems: locking errors, resource leaks. This is why
> kernel code usually does the `goto out' way of avoiding this.
>
> So.. please consider. In this case
>
> ret = -EINTR;
> goto out;
>
> would fit nicely.
>
>> + }
>> +
>> + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
>> + tmp_status = hdq_data->hdq_irqstatus;
>> + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
>> + /* check irqstatus */
>> + if (!(tmp_status & OMAP_HDQ_INT_STATUS_TIMEOUT)) {
>> + dev_dbg(hdq_data->dev, "timeout waiting for TIMEOUT, %x",
>> + tmp_status);
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> + return -ETIMEDOUT;
>
> Here too.
>
>> + }
>> + /*
>> + * wait for both INIT and GO bits rerurn to zero.
>> + * zero wait time expected for interrupt mode.
>> + */
>> + ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS,
>> + OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
>> + OMAP_HDQ_CTRL_STATUS_GO, OMAP_HDQ_FLAG_CLEAR,
>> + &tmp_status);
>> + if (ret)
>> + dev_dbg(hdq_data->dev, "timeout waiting INIT&GO bits"
>> + "return to zero, %x", tmp_status);
>> +
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
>> +{
>> + int ret;
>> + u8 status;
>> + unsigned long irqflags;
>> + unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT;
>> +
>> + ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
>> + if (ret < 0)
>> + return -EINTR;
>> +
>> + if (!hdq_data->hdq_usecount) {
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> + return -EINVAL;
>> + }
>> +
>> + if (!(hdq_data->hdq_irqstatus & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
>> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
>> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO,
>> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO);
>> + /*
>> + * The RX comes immediately after TX. It
>> + * triggers another interrupt before we
>> + * sleep. So we have to wait for RXCOMPLETE bit.
>> + */
>> + while (!(hdq_data->hdq_irqstatus
>> + & OMAP_HDQ_INT_STATUS_RXCOMPLETE)
>> + && time_before(jiffies, timeout)) {
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> + schedule_timeout(1);
>
> schedule_timeout_uninterruptible(1)
>
> If we were to implement the presently-missing
> wait_event_uninterruptible_timeout() (like
> wait_event_interruptible_timeout), could we use it here?
>
>> + }
>> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, 0,
>> + OMAP_HDQ_CTRL_STATUS_DIR);
>> + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
>> + status = hdq_data->hdq_irqstatus;
>> + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
>> + /* check irqstatus */
>> + if (!(status & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
>> + dev_dbg(hdq_data->dev, "timeout waiting for"
>> + "RXCOMPLETE, %x", status);
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> + return -ETIMEDOUT;
>> + }
>> + }
>> + /* the data is ready. Read it in! */
>> + *val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> +
>> + return 0;
>> +
>> +}
>> +
>>
>> ...
>>
>> +static int __init omap_hdq_probe(struct platform_device *pdev)
>> +{
>> + struct hdq_data *hdq_data;
>> + struct resource *res;
>> + int ret, irq;
>> + u8 rev;
>> +
>> + if (!pdev)
>> + return -ENODEV;
>
> Can this happen?
>
>> + hdq_data = kmalloc(sizeof(*hdq_data), GFP_KERNEL);
>> + if (!hdq_data) {
>> + dev_dbg(&pdev->dev, "unable to allocate memory\n");
>> + ret = -ENODEV;
>
> -ENOMEM
>
>> + goto err_kmalloc;
>> + }
>> +
>> + hdq_data->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, hdq_data);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_dbg(&pdev->dev, "unable to get resource\n");
>> + ret = ENXIO;
>
> bzzt, that should have been -ENXIO.
>
>> + goto err_resource;
>> + }
>> +
>> + hdq_data->hdq_base = ioremap(res->start, SZ_4K);
>> + if (!hdq_data->hdq_base) {
>> + dev_dbg(&pdev->dev, "ioremap failed\n");
>> + ret = -EINVAL;
>> + goto err_ioremap;
>> + }
>> +
>> + /* get interface & functional clock objects */
>> + hdq_data->hdq_ick = clk_get(&pdev->dev, "hdq_ick");
>> + hdq_data->hdq_fck = clk_get(&pdev->dev, "hdq_fck");
>> +
>> + if (IS_ERR(hdq_data->hdq_ick) || IS_ERR(hdq_data->hdq_fck)) {
>> + dev_dbg(&pdev->dev, "Can't get HDQ clock objects\n");
>> + if (IS_ERR(hdq_data->hdq_ick)) {
>> + ret = PTR_ERR(hdq_data->hdq_ick);
>> + goto err_clk;
>> + }
>> + if (IS_ERR(hdq_data->hdq_fck)) {
>> + ret = PTR_ERR(hdq_data->hdq_fck);
>> + clk_put(hdq_data->hdq_ick);
>> + goto err_clk;
>> + }
>> + }
>> +
>> + hdq_data->hdq_usecount = 0;
>> + mutex_init(&hdq_data->hdq_mutex);
>> +
>> + if (clk_enable(hdq_data->hdq_ick)) {
>> + dev_dbg(&pdev->dev, "Can not enable ick\n");
>> + ret = -ENODEV;
>> + goto err_ick;
>> + }
>> +
>> + if (clk_enable(hdq_data->hdq_fck)) {
>> + dev_dbg(&pdev->dev, "Can not enable fck\n");
>> + ret = -ENODEV;
>> + goto err_fck;
>> + }
>
> ooh, I like err_ick and err_fck a lot. They sound like akpm review
> comments at the end of a long day.
>
>> + rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
>> + dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
>> + (rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
>> +
>> + spin_lock_init(&hdq_data->hdq_spinlock);
>> + omap_hdq_break(hdq_data);
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + ret = -ENXIO;
>> + goto err_irq;
>> + }
>> +
>> + ret = request_irq(irq, hdq_isr, IRQF_DISABLED, "omap_hdq", hdq_data);
>> + if (ret < 0) {
>> + dev_dbg(&pdev->dev, "could not request irq\n");
>> + goto err_irq;
>> + }
>> +
>> + /* don't clock the HDQ until it is needed */
>> + clk_disable(hdq_data->hdq_ick);
>> + clk_disable(hdq_data->hdq_fck);
>> +
>> + omap_w1_master.data = hdq_data;
>> +
>> + ret = w1_add_master_device(&omap_w1_master);
>> + if (ret) {
>> + dev_dbg(&pdev->dev, "Failure in registering w1 master\n");
>> + goto err_w1;
>> + }
>> +
>> + return 0;
>> +
>> +err_w1:
>> +err_irq:
>> + clk_disable(hdq_data->hdq_fck);
>> +
>> +err_fck:
>> + clk_disable(hdq_data->hdq_ick);
>> +
>> +err_ick:
>> + clk_put(hdq_data->hdq_ick);
>> + clk_put(hdq_data->hdq_fck);
>> +
>> +err_clk:
>> + iounmap(hdq_data->hdq_base);
>> +
>> +err_ioremap:
>> +err_resource:
>> + platform_set_drvdata(pdev, NULL);
>> + kfree(hdq_data);
>> +
>> +err_kmalloc:
>> + return ret;
>> +
>> +}
>> +
>> +static int omap_hdq_remove(struct platform_device *pdev)
>> +{
>> + struct hdq_data *hdq_data = platform_get_drvdata(pdev);
>> +
>> + mutex_lock(&hdq_data->hdq_mutex);
>> +
>> + if (0 != hdq_data->hdq_usecount) {
>
> Well. Lots of people dislike that trick. It's used to catch
> accidental use of = where == was intended, but the compiler will warn
> anyway. There's less point in using it for !=.
>
>> + dev_dbg(&pdev->dev, "removed when use count is not zero\n");
>> + return -EBUSY;
>> + }
>> +
>> + mutex_unlock(&hdq_data->hdq_mutex);
>> +
>> + /* remove module dependency */
>> + clk_put(hdq_data->hdq_ick);
>> + clk_put(hdq_data->hdq_fck);
>> + free_irq(INT_24XX_HDQ_IRQ, hdq_data);
>> + platform_set_drvdata(pdev, NULL);
>> + iounmap(hdq_data->hdq_base);
>> + kfree(hdq_data);
>> +
>> + return 0;
>> +}
>> +
>> +static int __init
>> +omap_hdq_init(void)
>> +{
>> + return platform_driver_register(&omap_hdq_driver);
>> +}
>> +module_init(omap_hdq_init);
>> +
>> +static void __exit
>> +omap_hdq_exit(void)
>> +{
>> + platform_driver_unregister(&omap_hdq_driver);
>> +}
>> +module_exit(omap_hdq_exit);
>> +
>> +module_param(w1_id, int, S_IRUSR);
>> +MODULE_PARM_DESC(w1_id, "1-wire id for the slave detection");
>> +
>> +MODULE_AUTHOR("Texas Instruments");
>> +MODULE_DESCRIPTION("HDQ driver Library");
>> +MODULE_LICENSE("GPL");
>
> The code looks pretty good.
>
>
>
--
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