Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-14 Thread Evgeniy Polyakov 
On Tue, Oct 14, 2008 at 07:30:58AM -0700, Andrew Morton ([EMAIL PROTECTED]) 
wrote:
  Why not just skipping the waiting and returning error pretending user
  really sent a signal?
 
 Better than nothing, but because signal_pending() isn't actually true,
 upper layers wil behave differently.

If they check... 

For example omap_hdq_break() can be interrupted and omap_hdq_probe()
does not check its return value.

-- 
Evgeniy Polyakov
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-14 Thread Andrew Morton 
 On Tue, 14 Oct 2008 14:21:50 +0530 Madhusudhan Chikkature [EMAIL 
 PROTECTED] wrote:
 
 - Original Message - 
 From: Andrew Morton [EMAIL PROTECTED]
 To: Madhusudhan Chikkature [EMAIL PROTECTED]
 Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; 
 linux-omap@vger.kernel.org
 Sent: Monday, October 13, 2008 9:23 PM
 Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
 
 
  On Mon, 13 Oct 2008 18:55:43 +0530 Madhusudhan Chikkature [EMAIL 
  PROTECTED] wrote:
  
  - Original Message - 
  From: Andrew Morton [EMAIL PROTECTED]
  To: Gadiyar, Anand [EMAIL PROTECTED]
  Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; 
  linux-omap@vger.kernel.org; [EMAIL PROTECTED]
  Sent: Saturday, October 11, 2008 2:08 AM
  Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  
  
   +   /* 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?
  
  Yes.
  
 
 Yes. It is desired to return an error if the condition in the wait is not 
 met. I need to change the check for return value to check for zero and neg 
 value.
 
 I spent some time to test this perticular scenario.I could not really see any 
 impact of hitting ^C when an application is 
 transfering data in the background. When the h/w is programmed to transfer 
 data and the driver issues a wait, I see that 
 TXCOMPLETE interrupt comes immediately and wakes up as expected. 
 
 So guess I am unable to hit ^C exactly when the driver is waiting in 
 wait_event_interruptible_timeout(before the condition 
 is met) for it to catch the signal. Is it generally suggested to use 
 wait_event_timeout so that ^C signal is not caught?
 

I think it's reasonable to permit the driver's operations to be interrupted
in this manner.  It's done in quite a few other places.  But the problem is
actually *testing* it.

I guess one could do a whitebox-style test.  Add new code like:

{
static int x;
if (!(x++ % 1000)) {
printk(hit ^c now\n);
msleep(2000);
}
}

in the right place.

Tricky.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-14 Thread Andrew Morton 
 On Tue, 14 Oct 2008 17:42:49 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote:
 Hi.
 
 On Tue, Oct 14, 2008 at 05:50:02AM -0700, Andrew Morton ([EMAIL PROTECTED]) 
 wrote:
  I think it's reasonable to permit the driver's operations to be interrupted
  in this manner.  It's done in quite a few other places.  But the problem is
  actually *testing* it.
 
 Why not just skipping the waiting and returning error pretending user
 really sent a signal?

Better than nothing, but because signal_pending() isn't actually true,
upper layers wil behave differently.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-14 Thread Evgeniy Polyakov 
Hi.

On Tue, Oct 14, 2008 at 05:50:02AM -0700, Andrew Morton ([EMAIL PROTECTED]) 
wrote:
 I think it's reasonable to permit the driver's operations to be interrupted
 in this manner.  It's done in quite a few other places.  But the problem is
 actually *testing* it.

Why not just skipping the waiting and returning error pretending user
really sent a signal?

-- 
Evgeniy Polyakov
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-14 Thread Madhusudhan Chikkature 

- Original Message - 
From: Andrew Morton [EMAIL PROTECTED]
To: Madhusudhan Chikkature [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; 
linux-omap@vger.kernel.org
Sent: Monday, October 13, 2008 9:23 PM
Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430


 On Mon, 13 Oct 2008 18:55:43 +0530 Madhusudhan Chikkature [EMAIL 
 PROTECTED] wrote:
 
 - Original Message - 
 From: Andrew Morton [EMAIL PROTECTED]
 To: Gadiyar, Anand [EMAIL PROTECTED]
 Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; linux-omap@vger.kernel.org; 
 [EMAIL PROTECTED]
 Sent: Saturday, October 11, 2008 2:08 AM
 Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
 
 
  +   /* 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?
 
 Yes.
 

Yes. It is desired to return an error if the condition in the wait is not met. 
I need to change the check for return value to check for zero and neg value.

I spent some time to test this perticular scenario.I could not really see any 
impact of hitting ^C when an application is 
transfering data in the background. When the h/w is programmed to transfer data 
and the driver issues a wait, I see that 
TXCOMPLETE interrupt comes immediately and wakes up as expected. 

So guess I am unable to hit ^C exactly when the driver is waiting in 
wait_event_interruptible_timeout(before the condition 
is met) for it to catch the signal. Is it generally suggested to use 
wait_event_timeout so that ^C signal is not caught?

Regards,
Madhu
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-13 Thread Madhusudhan Chikkature 

- Original Message - 
From: Andrew Morton [EMAIL PROTECTED]
To: Gadiyar, Anand [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; linux-omap@vger.kernel.org; 
[EMAIL PROTECTED]
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 [EMAIL PROTECTED] wrote:
 
 From: Madhusudhan Chikkature [EMAIL PROTECTED]
 
 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.0 +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

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-13 Thread Evgeniy Polyakov 
Hi.

On Mon, Oct 13, 2008 at 06:55:43PM +0530, Madhusudhan Chikkature ([EMAIL 
PROTECTED]) wrote:
  +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?

Just plain return looks suspicious, is there some kind of a race between
calling code (which for example frees hdq_data) and the path, which sets
the bit and wakes up this thread?

-- 
Evgeniy Polyakov
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-13 Thread Andrew Morton 
 On Mon, 13 Oct 2008 18:55:43 +0530 Madhusudhan Chikkature [EMAIL 
 PROTECTED] wrote:
 
 - Original Message - 
 From: Andrew Morton [EMAIL PROTECTED]
 To: Gadiyar, Anand [EMAIL PROTECTED]
 Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; linux-omap@vger.kernel.org; 
 [EMAIL PROTECTED]
 Sent: Saturday, October 11, 2008 2:08 AM
 Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
 
 
  +   /* 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?

Yes.


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430

2008-10-10 Thread Andrew Morton 
On Wed, 8 Oct 2008 12:46:25 +0530
Gadiyar, Anand [EMAIL PROTECTED] wrote:

 From: Madhusudhan Chikkature [EMAIL PROTECTED]
 
 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.0 
 +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?

 +   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);
 +