[linux-dvb] [PATCH 1/3] Fix a problem during the access to the IER and ISR registers of the SA7146

Oliver Endriss o.endriss at gmx.de
Wed Nov 1 01:43:57 CET 2006


e9hack wrote:
> Oliver Endriss wrote:
> >   
> >> Protect the access to the IER/ISR register of the SAA7146 by the device spinlock.
> >>     
> >
> > Imho it is not necessary to protect write operations to the ISR because
> > it is a single write-only operation.
> >   
> You are right.
> 
> > SAA7146_IER_DISABLE/SAA7146_IER_ENABLE must be protected by a spinlock
> > because it is a read-modify-write operation.
> >
> > So your patch could be replaced by the attached one.
> >
> > What do you think?
> >
> >   
> It exist some pieces of code, where the spinlock is already locked and
> where the macros are used.

Argh, I should have checked that. :(

> It exist a second spinlock within the SAA7146 
> device structure. Currently, it is unused. This one can be used.

>--- a/linux/include/media/saa7146.h     Sun Oct 29 11:12:27 2006 -0300
>+++ b/linux/include/media/saa7146.h     Tue Oct 31 18:12:58 2006 +0100
>@@ -54,10 +54,20 @@ extern unsigned int saa7146_debug;
> #define DEB_INT(x)  if (0!=(DEBUG_VARIABLE&0x20)) { DEBUG_PROLOG; printk x; } /* interrupt debug messages */
> #define DEB_CAP(x)  if (0!=(DEBUG_VARIABLE&0x40)) { DEBUG_PROLOG; printk x; } /* capture debug messages */
> 
>-#define SAA7146_IER_DISABLE(x,y) \
>-       saa7146_write(x, IER, saa7146_read(x, IER) & ~(y));
>+#define SAA7146_IER_DISABLE(x,y)                                       \
>+       do {                                                            \
>+               unsigned int flags;                                     \
>+               spin_lock_irqsave(&x->int_slock, flags);                \
>+               saa7146_write(x, IER, saa7146_read(x, IER) & ~(y));     \
>+               spin_unlock_irqrestore(&x->int_slock, flags);           \
>+       } while(0)
> #define SAA7146_IER_ENABLE(x,y) \
>-       saa7146_write(x, IER, saa7146_read(x, IER) | (y));
>+       do {                                                            \
>+               unsigned int flags;                                     \
>+               spin_lock_irqsave(&x->int_slock, flags);                \
>+               saa7146_write(x, IER, saa7146_read(x, IER) | (y));      \
>+               spin_unlock_irqrestore(&x->int_slock, flags);           \
>+       } while(0)
> #define SAA7146_ISR_CLEAR(x,y) \
>        saa7146_write(x, ISR, (y));
>

Ack, looks good.

>diff -r a80a23f896b2 linux/drivers/media/common/saa7146_i2c.c
>--- a/linux/drivers/media/common/saa7146_i2c.c  Sun Oct 29 11:12:27 2006 -0300
>+++ b/linux/drivers/media/common/saa7146_i2c.c  Tue Oct 31 18:16:28 2006 +0100
>@@ -190,13 +190,24 @@ static int saa7146_i2c_writeout(struct s
>                saa7146_write(dev, I2C_TRANSFER, *dword);
> 
>                dev->i2c_op = 1;
>+               SAA7146_ISR_CLEAR(dev, MASK_16|MASK_17);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Good! [1]

>                SAA7146_IER_ENABLE(dev, MASK_16|MASK_17);
>                saa7146_write(dev, MC2, (MASK_00 | MASK_16));
> 
>-               wait_event_interruptible(dev->i2c_wq, dev->i2c_op == 0);
>-               if (signal_pending (current)) {
>-                       /* a signal arrived */
>-                       return -ERESTARTSYS;
>+               timeout = HZ/100 + 1; /* 10ms */
>+               timeout = wait_event_interruptible_timeout(dev->i2c_wq, dev->i2c_op == 0, timeout);
>+               if (timeout == -ERESTARTSYS || dev->i2c_op) {
>+                       SAA7146_IER_DISABLE(dev, MASK_16|MASK_17);
>+                       SAA7146_ISR_CLEAR(dev, MASK_16|MASK_17);
>+                       if (timeout == -ERESTARTSYS) {
>+                               /* a signal arrived */
>+                               return -ERESTARTSYS;
>+                       }

Please remove '{' and '}' - kernel coding style ;-)

>+                       /* this is normal when probing the bus
>+                        * (no answer from nonexisistant device...)
>+                        */
>+                       DEB_I2C(("saa7146_i2c_writeout: timed out waiting for end of xfer\n"));
>+                       return -EIO;

Are you able to trigger this timeout? Is yes, how?

Imho it cannot happen anymore (because of [1]) unless the hardware is
broken. Even with nonexistent devices there should be an I2C irq.

Anyway, it is a good idea to add this timeout protection.

If it cannot happen under normal circumstances you should replace
DEB_I2C by printk.

Oliver

-- 
--------------------------------------------------------
VDR Remote Plugin 0.3.8 available at
http://www.escape-edv.de/endriss/vdr/
--------------------------------------------------------


More information about the linux-dvb mailing list