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/
--------------------------------------------------------
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
