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

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

[Mauro Carvalho Chehab]

Andy,

Please review. The lack of porting cx23885 to new kfifo is stopping the merge
of the redesigned kfifo upstream.

Cheers,
Mauro.

-------- 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
@@ -124,15 +124,12 @@ struct cx23888_ir_state {
 	atomic_t rxclk_divider;
 	atomic_t rx_invert;
 
-	struct kfifo *rx_kfifo;
+	struct kfifo rx_kfifo;
 	spinlock_t rx_kfifo_lock;
 
 	struct v4l2_subdev_ir_parameters tx_params;
 	struct mutex tx_params_lock;
 	atomic_t txclk_divider;
-
-	struct kfifo *tx_kfifo;
-	spinlock_t tx_kfifo_lock;
 };
 
 static inline struct cx23888_ir_state *to_state(struct v4l2_subdev *sd)
@@ -594,8 +591,9 @@ static int cx23888_ir_irq_handler(struct
 			if (i == 0)
 				break;
 			j = i * sizeof(u32);
-			k = kfifo_put(state->rx_kfifo,
-				      (unsigned char *) rx_data, j);
+			k = kfifo_in_locked(&state->rx_kfifo,
+				      (unsigned char *) rx_data, j,
+				      &state->rx_kfifo_lock);
 			if (k != j)
 				kror++; /* rx_kfifo over run */
 		}
@@ -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)
@@ -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);
@@ -892,7 +890,6 @@ static int cx23888_ir_tx_s_parameters(st
 	o->interrupt_enable = p->interrupt_enable;
 	o->enable = p->enable;
 	if (p->enable) {
-		kfifo_reset(state->tx_kfifo);
 		if (p->interrupt_enable)
 			irqenable_tx(dev, IRQEN_TSE);
 		control_tx_enable(dev, p->enable);
@@ -1168,19 +1165,9 @@ int cx23888_ir_probe(struct cx23885_dev 
 		return -ENOMEM;
 
 	spin_lock_init(&state->rx_kfifo_lock);
-	state->rx_kfifo = kfifo_alloc(CX23888_IR_RX_KFIFO_SIZE, GFP_KERNEL,
-				      &state->rx_kfifo_lock);
-	if (state->rx_kfifo == NULL)
+	if (kfifo_alloc(&state->rx_kfifo, CX23888_IR_RX_KFIFO_SIZE, GFP_KERNEL))
 		return -ENOMEM;
 
-	spin_lock_init(&state->tx_kfifo_lock);
-	state->tx_kfifo = kfifo_alloc(CX23888_IR_TX_KFIFO_SIZE, GFP_KERNEL,
-				      &state->tx_kfifo_lock);
-	if (state->tx_kfifo == NULL) {
-		kfifo_free(state->rx_kfifo);
-		return -ENOMEM;
-	}
-
 	state->dev = dev;
 	state->id = V4L2_IDENT_CX23888_IR;
 	state->rev = 0;
@@ -1211,8 +1198,7 @@ int cx23888_ir_probe(struct cx23885_dev 
 		       sizeof(struct v4l2_subdev_ir_parameters));
 		v4l2_subdev_call(sd, ir, tx_s_parameters, &default_params);
 	} else {
-		kfifo_free(state->rx_kfifo);
-		kfifo_free(state->tx_kfifo);
+		kfifo_free(&state->rx_kfifo);
 	}
 	return ret;
 }
@@ -1231,8 +1217,7 @@ int cx23888_ir_remove(struct cx23885_dev
 
 	state = to_state(sd);
 	v4l2_device_unregister_subdev(sd);
-	kfifo_free(state->rx_kfifo);
-	kfifo_free(state->tx_kfifo);
+	kfifo_free(&state->rx_kfifo);
 	kfree(state);
 	/* Nothing more to free() as state held the actual v4l2_subdev object */
 	return 0;

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

[Andy Walls]

On Fri, 2009-12-18 at 08:14 -0200, Mauro Carvalho Chehab wrote:
> Andy,
> 
> Please review. The lack of porting cx23885 to new kfifo is stopping the merge
> of the redesigned kfifo upstream.


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
> @@ -124,15 +124,12 @@ struct cx23888_ir_state {
>  	atomic_t rxclk_divider;
>  	atomic_t rx_invert;
>  
> -	struct kfifo *rx_kfifo;
> +	struct kfifo rx_kfifo;
>  	spinlock_t rx_kfifo_lock;
>  
>  	struct v4l2_subdev_ir_parameters tx_params;
>  	struct mutex tx_params_lock;
>  	atomic_t txclk_divider;
> -
> -	struct kfifo *tx_kfifo;
> -	spinlock_t tx_kfifo_lock;
>  };
>  
>  static inline struct cx23888_ir_state *to_state(struct v4l2_subdev *sd)
> @@ -594,8 +591,9 @@ static int cx23888_ir_irq_handler(struct
>  			if (i == 0)
>  				break;
>  			j = i * sizeof(u32);
> -			k = kfifo_put(state->rx_kfifo,
> -				      (unsigned char *) rx_data, j);
> +			k = kfifo_in_locked(&state->rx_kfifo,
> +				      (unsigned char *) rx_data, j,
> +				      &state->rx_kfifo_lock);
>  			if (k != j)
>  				kror++; /* rx_kfifo over run */
>  		}
> @@ -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.


> @@ -892,7 +890,6 @@ static int cx23888_ir_tx_s_parameters(st
>  	o->interrupt_enable = p->interrupt_enable;
>  	o->enable = p->enable;
>  	if (p->enable) {
> -		kfifo_reset(state->tx_kfifo);
>  		if (p->interrupt_enable)
>  			irqenable_tx(dev, IRQEN_TSE);
>  		control_tx_enable(dev, p->enable);

I don't mind the currently unused tx_kfifo being removed from the
current implementation.  However, could you leave a comment at this line
about reseting a tx_kfifo?  Otherwise, I may forget this one when I go
to implement transmit.


> @@ -1168,19 +1165,9 @@ int cx23888_ir_probe(struct cx23885_dev 
>  		return -ENOMEM;
>  
>  	spin_lock_init(&state->rx_kfifo_lock);
> -	state->rx_kfifo = kfifo_alloc(CX23888_IR_RX_KFIFO_SIZE, GFP_KERNEL,
> -				      &state->rx_kfifo_lock);
> -	if (state->rx_kfifo == NULL)
> +	if (kfifo_alloc(&state->rx_kfifo, CX23888_IR_RX_KFIFO_SIZE, GFP_KERNEL))
>  		return -ENOMEM;
>  
> -	spin_lock_init(&state->tx_kfifo_lock);
> -	state->tx_kfifo = kfifo_alloc(CX23888_IR_TX_KFIFO_SIZE, GFP_KERNEL,
> -				      &state->tx_kfifo_lock);
> -	if (state->tx_kfifo == NULL) {
> -		kfifo_free(state->rx_kfifo);
> -		return -ENOMEM;
> -	}
> -
>  	state->dev = dev;
>  	state->id = V4L2_IDENT_CX23888_IR;
>  	state->rev = 0;
> @@ -1211,8 +1198,7 @@ int cx23888_ir_probe(struct cx23885_dev 
>  		       sizeof(struct v4l2_subdev_ir_parameters));
>  		v4l2_subdev_call(sd, ir, tx_s_parameters, &default_params);
>  	} else {
> -		kfifo_free(state->rx_kfifo);
> -		kfifo_free(state->tx_kfifo);
> +		kfifo_free(&state->rx_kfifo);
>  	}
>  	return ret;
>  }
> @@ -1231,8 +1217,7 @@ int cx23888_ir_remove(struct cx23885_dev
>  
>  	state = to_state(sd);
>  	v4l2_device_unregister_subdev(sd);
> -	kfifo_free(state->rx_kfifo);
> -	kfifo_free(state->tx_kfifo);
> +	kfifo_free(&state->rx_kfifo);
>  	kfree(state);
>  	/* Nothing more to free() as state held the actual v4l2_subdev object */
>  	return 0;
> 
> 


That's it.  Thanks for taking the time to work up a patch.

Regards,
Andy

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

[Stefani Seibold]

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:
> > Andy,
> > 
> > Please review. The lack of porting cx23885 to new kfifo is stopping the merge
> > of the redesigned kfifo upstream.
> 
> 
> 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
> > @@ -124,15 +124,12 @@ struct cx23888_ir_state {
> >  	atomic_t rxclk_divider;
> >  	atomic_t rx_invert;
> >  
> > -	struct kfifo *rx_kfifo;
> > +	struct kfifo rx_kfifo;
> >  	spinlock_t rx_kfifo_lock;
> >  
> >  	struct v4l2_subdev_ir_parameters tx_params;
> >  	struct mutex tx_params_lock;
> >  	atomic_t txclk_divider;
> > -
> > -	struct kfifo *tx_kfifo;
> > -	spinlock_t tx_kfifo_lock;
> >  };
> >  
> >  static inline struct cx23888_ir_state *to_state(struct v4l2_subdev *sd)
> > @@ -594,8 +591,9 @@ static int cx23888_ir_irq_handler(struct
> >  			if (i == 0)
> >  				break;
> >  			j = i * sizeof(u32);
> > -			k = kfifo_put(state->rx_kfifo,
> > -				      (unsigned char *) rx_data, j);
> > +			k = kfifo_in_locked(&state->rx_kfifo,
> > +				      (unsigned char *) rx_data, j,
> > +				      &state->rx_kfifo_lock);
> >  			if (k != j)
> >  				kror++; /* rx_kfifo over run */
> >  		}
> > @@ -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.
> 
> 
> > @@ -892,7 +890,6 @@ static int cx23888_ir_tx_s_parameters(st
> >  	o->interrupt_enable = p->interrupt_enable;
> >  	o->enable = p->enable;
> >  	if (p->enable) {
> > -		kfifo_reset(state->tx_kfifo);
> >  		if (p->interrupt_enable)
> >  			irqenable_tx(dev, IRQEN_TSE);
> >  		control_tx_enable(dev, p->enable);
> 
> I don't mind the currently unused tx_kfifo being removed from the
> current implementation.  However, could you leave a comment at this line
> about reseting a tx_kfifo?  Otherwise, I may forget this one when I go
> to implement transmit.
> 
> 
> > @@ -1168,19 +1165,9 @@ int cx23888_ir_probe(struct cx23885_dev 
> >  		return -ENOMEM;
> >  
> >  	spin_lock_init(&state->rx_kfifo_lock);
> > -	state->rx_kfifo = kfifo_alloc(CX23888_IR_RX_KFIFO_SIZE, GFP_KERNEL,
> > -				      &state->rx_kfifo_lock);
> > -	if (state->rx_kfifo == NULL)
> > +	if (kfifo_alloc(&state->rx_kfifo, CX23888_IR_RX_KFIFO_SIZE, GFP_KERNEL))
> >  		return -ENOMEM;
> >  
> > -	spin_lock_init(&state->tx_kfifo_lock);
> > -	state->tx_kfifo = kfifo_alloc(CX23888_IR_TX_KFIFO_SIZE, GFP_KERNEL,
> > -				      &state->tx_kfifo_lock);
> > -	if (state->tx_kfifo == NULL) {
> > -		kfifo_free(state->rx_kfifo);
> > -		return -ENOMEM;
> > -	}
> > -
> >  	state->dev = dev;
> >  	state->id = V4L2_IDENT_CX23888_IR;
> >  	state->rev = 0;
> > @@ -1211,8 +1198,7 @@ int cx23888_ir_probe(struct cx23885_dev 
> >  		       sizeof(struct v4l2_subdev_ir_parameters));
> >  		v4l2_subdev_call(sd, ir, tx_s_parameters, &default_params);
> >  	} else {
> > -		kfifo_free(state->rx_kfifo);
> > -		kfifo_free(state->tx_kfifo);
> > +		kfifo_free(&state->rx_kfifo);
> >  	}
> >  	return ret;
> >  }
> > @@ -1231,8 +1217,7 @@ int cx23888_ir_remove(struct cx23885_dev
> >  
> >  	state = to_state(sd);
> >  	v4l2_device_unregister_subdev(sd);
> > -	kfifo_free(state->rx_kfifo);
> > -	kfifo_free(state->tx_kfifo);
> > +	kfifo_free(&state->rx_kfifo);
> >  	kfree(state);
> >  	/* Nothing more to free() as state held the actual v4l2_subdev object */
> >  	return 0;
> > 
> > 
> 
> 
> That's it.  Thanks for taking the time to work up a patch.

Sorry, i ported it only to the new API. I did not touch the
functionality. Feel free to fix it and post it.

Stefani

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