Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]

Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]

[Andy Walls]

On Fri, 2009-12-18 at 13:11 +0100, Stefani Seibold wrote:
> Am Freitag, den 18.12.2009, 07:00 -0500 schrieb Andy Walls:
> > On Fri, 2009-12-18 at 08:14 -0200, Mauro Carvalho Chehab wrote:

> > 
> > Stefani and Mauro,
> > 
> > My comments/concerns are in line:
> > 
> > > -------- Mensagem original --------
> > > Assunto: [patch] media video cx23888 driver: ported to new kfifo API
> > > Data: Fri, 18 Dec 2009 09:12:34 +0100
> > > De: Stefani Seibold <stefani@seibold.net>
> > > Para: linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>,  Mauro Carvalho Chehab <mchehab@infradead.org>
> > > 
> > > This patch will fix the cx23888 driver to use the new kfifo API.
> > > 
> > > The patch-set is against current mm tree from 11-Dec-2009
> > > 
> > > Greetings,
> > > Stefani
> > > 
> > > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > > ---
> > >  cx23888-ir.c |   35 ++++++++++-------------------------
> > >  1 file changed, 10 insertions(+), 25 deletions(-)
> > > 
> > > --- mmotm.orig/drivers/media/video/cx23885/cx23888-ir.c	2009-12-18 08:42:53.936778002 +0100
> > > +++ mmotm/drivers/media/video/cx23885/cx23888-ir.c	2009-12-18 09:03:04.808703259 +0100

> > > @@ -631,7 +629,7 @@ static int cx23888_ir_irq_handler(struct
> > >  		cx23888_ir_write4(dev, CX23888_IR_CNTRL_REG, cntrl);
> > >  		*handled = true;
> > >  	}
> > > -	if (kfifo_len(state->rx_kfifo) >= CX23888_IR_RX_KFIFO_SIZE / 2)
> > > +	if (kfifo_len(&state->rx_kfifo) >= CX23888_IR_RX_KFIFO_SIZE / 2)
> > >  		events |= V4L2_SUBDEV_IR_RX_FIFO_SERVICE_REQ;
> > >  
> > >  	if (events)
> > 
> > I am concerned about reading the kfifo_len() without taking the lock,
> > since another thread on another CPU may be reading from the kfifo at the
> > same time.
> > 
> > If the new kfifo implementation has an atomic_read() or something behind
> > the kfifo_len() call, then OK.
> > 
> > 
> > > @@ -657,7 +655,7 @@ static int cx23888_ir_rx_read(struct v4l
> > >  		return 0;
> > >  	}
> > >  
> > > -	n = kfifo_get(state->rx_kfifo, buf, n);
> > > +	n = kfifo_out_locked(&state->rx_kfifo, buf, n, &state->rx_kfifo_lock);
> > >  
> > >  	n /= sizeof(u32);
> > >  	*num = n * sizeof(u32);
> > > @@ -785,7 +783,7 @@ static int cx23888_ir_rx_s_parameters(st
> > >  	o->interrupt_enable = p->interrupt_enable;
> > >  	o->enable = p->enable;
> > >  	if (p->enable) {
> > > -		kfifo_reset(state->rx_kfifo);
> > > +		kfifo_reset(&state->rx_kfifo);
> > >  		if (p->interrupt_enable)
> > >  			irqenable_rx(dev, IRQEN_RSE | IRQEN_RTE | IRQEN_ROE);
> > >  		control_rx_enable(dev, p->enable);
> > 
> > Same concern about kfifo_reset() not taking the lock, and another thread
> > reading data from the kfifo at the same time.  In the cx23885 module,
> > this would mostly likely happen only during module unload as things are
> > being shut down.
> > 

> Sorry, i ported it only to the new API. I did not touch the
> functionality.

Stefani,

Huh?  The new kfifo implementation you wrote removed the locks from
kfifo_len() and kfifo_reset().  By changing those two functions to not
provide locking, you changed the functionality.


>  Feel free to fix it and post it.

Please point me to the mmotm tree and I will try to get to this
tonight. 

I have a blizzard coming tonight threatening to leave 16 to 20 inches of
snow and I will likely lose power.  I have to purchase gas for my
generator and go split some wood for the fire.  I might not get the
change done before I lose power.

If you can't wait for me to provide a patch, just add spin locks and
unlocks around the calls to kfifo_len() and kfifo_reset() in addition to
the changes in your current patch.


Regards,
Andy

> Stefani

Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]

[Stefani Seibold]

Am Freitag, den 18.12.2009, 16:39 -0500 schrieb Andy Walls:
> On Fri, 2009-12-18 at 13:11 +0100, Stefani Seibold wrote:
> > Am Freitag, den 18.12.2009, 07:00 -0500 schrieb Andy Walls:
> > > On Fri, 2009-12-18 at 08:14 -0200, Mauro Carvalho Chehab wrote:
> 
> > > 
> > > Stefani and Mauro,
> > > 
> > > My comments/concerns are in line:
> > > 
> > > > -------- Mensagem original --------
> > > > Assunto: [patch] media video cx23888 driver: ported to new kfifo API
> > > > Data: Fri, 18 Dec 2009 09:12:34 +0100
> > > > De: Stefani Seibold <stefani@seibold.net>
> > > > Para: linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>,  Mauro Carvalho Chehab <mchehab@infradead.org>
> > > > 
> > > > This patch will fix the cx23888 driver to use the new kfifo API.
> > > > 
> > > > The patch-set is against current mm tree from 11-Dec-2009
> > > > 
> > > > Greetings,
> > > > Stefani
> > > > 
> > > > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > > > ---
> > > >  cx23888-ir.c |   35 ++++++++++-------------------------
> > > >  1 file changed, 10 insertions(+), 25 deletions(-)
> > > > 
> > > > --- mmotm.orig/drivers/media/video/cx23885/cx23888-ir.c	2009-12-18 08:42:53.936778002 +0100
> > > > +++ mmotm/drivers/media/video/cx23885/cx23888-ir.c	2009-12-18 09:03:04.808703259 +0100
> 
> > > > @@ -631,7 +629,7 @@ static int cx23888_ir_irq_handler(struct
> > > >  		cx23888_ir_write4(dev, CX23888_IR_CNTRL_REG, cntrl);
> > > >  		*handled = true;
> > > >  	}
> > > > -	if (kfifo_len(state->rx_kfifo) >= CX23888_IR_RX_KFIFO_SIZE / 2)
> > > > +	if (kfifo_len(&state->rx_kfifo) >= CX23888_IR_RX_KFIFO_SIZE / 2)
> > > >  		events |= V4L2_SUBDEV_IR_RX_FIFO_SERVICE_REQ;
> > > >  
> > > >  	if (events)
> > > 
> > > I am concerned about reading the kfifo_len() without taking the lock,
> > > since another thread on another CPU may be reading from the kfifo at the
> > > same time.
> > > 
> > > If the new kfifo implementation has an atomic_read() or something behind
> > > the kfifo_len() call, then OK.
> > > 
> > > 
> > > > @@ -657,7 +655,7 @@ static int cx23888_ir_rx_read(struct v4l
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > -	n = kfifo_get(state->rx_kfifo, buf, n);
> > > > +	n = kfifo_out_locked(&state->rx_kfifo, buf, n, &state->rx_kfifo_lock);
> > > >  
> > > >  	n /= sizeof(u32);
> > > >  	*num = n * sizeof(u32);
> > > > @@ -785,7 +783,7 @@ static int cx23888_ir_rx_s_parameters(st
> > > >  	o->interrupt_enable = p->interrupt_enable;
> > > >  	o->enable = p->enable;
> > > >  	if (p->enable) {
> > > > -		kfifo_reset(state->rx_kfifo);
> > > > +		kfifo_reset(&state->rx_kfifo);
> > > >  		if (p->interrupt_enable)
> > > >  			irqenable_rx(dev, IRQEN_RSE | IRQEN_RTE | IRQEN_ROE);
> > > >  		control_rx_enable(dev, p->enable);
> > > 
> > > Same concern about kfifo_reset() not taking the lock, and another thread
> > > reading data from the kfifo at the same time.  In the cx23885 module,
> > > this would mostly likely happen only during module unload as things are
> > > being shut down.
> > > 
> 
> > Sorry, i ported it only to the new API. I did not touch the
> > functionality.
> 
> Stefani,
> 
> Huh?  The new kfifo implementation you wrote removed the locks from
> kfifo_len() and kfifo_reset().  By changing those two functions to not
> provide locking, you changed the functionality.

You are right. Brain was temporary switch off. But kfifo_len() did not
requiere a lock in my opinion. It is save to use without a look. 

But the use of kfifo_reset() is without locking dangerous. 

I will write a path to lock your operations and send it to andrew.

Stefani

Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]

[Andrew Morton]

On Fri, 18 Dec 2009 22:57:22 +0100
Stefani Seibold <stefani@seibold.net> wrote:

> But kfifo_len() did not
> requiere a lock in my opinion. It is save to use without a look. 

What do you mean by this?  Safe in general, or safe in this particular driver?

In either case: why?

Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]

[Stefani Seibold]

Am Freitag, den 18.12.2009, 14:00 -0800 schrieb Andrew Morton:
> On Fri, 18 Dec 2009 22:57:22 +0100
> Stefani Seibold <stefani@seibold.net> wrote:
> 
> > But kfifo_len() did not
> > requiere a lock in my opinion. It is save to use without a look. 
> 
> What do you mean by this?  Safe in general, or safe in this particular driver?
> 

Safe until you don't call kfifo_reset(). kfifo_reset() is evil, because
it it the only function which breaks the single read/writer concept. 

This function modifies the in and the out fifo counters. Thats why i
introduced the kfifo_reset_out() function, which set the out to the
value of in, which means the fifo is empty. This function can be call
from the receiver of a fifo without locking.  

Stefani