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 <stef...@seibold.net>
> Para: linux-ker...@vger.kernel.org, Andrew Morton
> <a...@linux-foundation.org>, Mauro Carvalho Chehab <mche...@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 <stef...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
