Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]
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.c2009-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);
-
Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]
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 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
Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]
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 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.c2009-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
--
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
Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]
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 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
@@ -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
--
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
Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]
On Fri, 18 Dec 2009 22:57:22 +0100 Stefani Seibold stef...@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? -- 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
Re: [Fwd: [patch] media video cx23888 driver: ported to new kfifo API]
Am Freitag, den 18.12.2009, 14:00 -0800 schrieb Andrew Morton: On Fri, 18 Dec 2009 22:57:22 +0100 Stefani Seibold stef...@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 -- 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
