[linux-dvb] [PATCH] budget_av / dvb_ca_en50221: fixes ci/cam handling especially on SMP machines

[linux-dvb] [PATCH] budget_av / dvb_ca_en50221: fixes ci/cam handling especially on SMP machines

[Matthias Dahl]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

Hi Oliver.

I can happily report that with the following two patches applied, I haven't 
seen a single case where the cam stopped working due to i/o errors or 
anything like it.

The budget_av patch is basically your patch just a bit extended which I 
thought was necessary to cover all relevant cases. Works just fine.

The dvb_ca_en50221 patch introduces the concept of slot lock that means, you 
can either read or write to a slot but concurrent i/o on a slot is no longer 
allowed. This case was already thought of and partly taken care of but 
unfortunately due to the missing locking mechanism, it just made the race 
condition harder to trigger but not impossible... especially on SMP systems 
where this is easier to hit. That's way I introduced a mutex. I left the 
original check in there but it actually never should get triggered anymore. 
Right now actually, if it gets triggered, one could assume the ci/cam is in 
an undefined state and trigger a reinit, like it's done on a few other 
places.

Could you please apply those patches to the dvb tree and maybe get into the 
official 2.6.27? Those bugs haven been around for quite some time now and 
without the patches, they are not so hard to trigger.

Thanks a lot for your help by the way!

So long,
matthias.

[-- Attachment #2: budget-av_camlock_2.diff --]
[-- Type: text/x-diff, Size: 6759 bytes --]

--- a/drivers/media/dvb/ttpci/budget-av.c	2008-07-13 23:51:29.000000000 +0200
+++ b/drivers/media/dvb/ttpci/budget-av.c	2008-08-18 21:53:10.000000000 +0200
@@ -65,10 +65,11 @@ struct budget_av {
 	struct tasklet_struct ciintf_irq_tasklet;
 	int slot_status;
 	struct dvb_ca_en50221 ca;
+	struct mutex camlock;
 	u8 reinitialise_demod:1;
 };
 
-static int ciintf_slot_shutdown(struct dvb_ca_en50221 *ca, int slot);
+static int ciintf_unlocked_slot_shutdown(struct dvb_ca_en50221 *ca, int slot);
 
 
 /* GPIO Connections:
@@ -128,7 +129,7 @@ static int i2c_writereg(struct i2c_adapt
 	return i2c_transfer(i2c, &msgs, 1);
 }
 
-static int ciintf_read_attribute_mem(struct dvb_ca_en50221 *ca, int slot, int address)
+static int ciintf_unlocked_read_attribute_mem(struct dvb_ca_en50221 *ca, int slot, int address)
 {
 	struct budget_av *budget_av = (struct budget_av *) ca->data;
 	int result;
@@ -141,9 +142,22 @@ static int ciintf_read_attribute_mem(str
 
 	result = ttpci_budget_debiread(&budget_av->budget, DEBICICAM, address & 0xfff, 1, 0, 1);
 	if (result == -ETIMEDOUT) {
-		ciintf_slot_shutdown(ca, slot);
+		ciintf_unlocked_slot_shutdown(ca, slot);
 		printk(KERN_INFO "budget-av: cam ejected 1\n");
 	}
+
+	return result;
+}
+
+static int ciintf_read_attribute_mem(struct dvb_ca_en50221 *ca, int slot, int address)
+{
+	struct budget_av *budget_av = (struct budget_av *) ca->data;
+	int result;
+
+	mutex_lock(&budget_av->camlock);
+	result = ciintf_unlocked_read_attribute_mem(ca, slot, address);
+	mutex_unlock(&budget_av->camlock);
+
 	return result;
 }
 
@@ -155,14 +169,19 @@ static int ciintf_write_attribute_mem(st
 	if (slot != 0)
 		return -EINVAL;
 
+	mutex_lock(&budget_av->camlock);
+
 	saa7146_setgpio(budget_av->budget.dev, 1, SAA7146_GPIO_OUTHI);
 	udelay(1);
 
 	result = ttpci_budget_debiwrite(&budget_av->budget, DEBICICAM, address & 0xfff, 1, value, 0, 1);
 	if (result == -ETIMEDOUT) {
-		ciintf_slot_shutdown(ca, slot);
+		ciintf_unlocked_slot_shutdown(ca, slot);
 		printk(KERN_INFO "budget-av: cam ejected 2\n");
 	}
+
+	mutex_unlock(&budget_av->camlock);
+
 	return result;
 }
 
@@ -174,15 +193,19 @@ static int ciintf_read_cam_control(struc
 	if (slot != 0)
 		return -EINVAL;
 
+	mutex_lock(&budget_av->camlock);
+
 	saa7146_setgpio(budget_av->budget.dev, 1, SAA7146_GPIO_OUTLO);
 	udelay(1);
 
 	result = ttpci_budget_debiread(&budget_av->budget, DEBICICAM, address & 3, 1, 0, 0);
 	if (result == -ETIMEDOUT) {
-		ciintf_slot_shutdown(ca, slot);
+		ciintf_unlocked_slot_shutdown(ca, slot);
 		printk(KERN_INFO "budget-av: cam ejected 3\n");
-		return -ETIMEDOUT;
 	}
+
+	mutex_unlock(&budget_av->camlock);
+
 	return result;
 }
 
@@ -194,14 +217,19 @@ static int ciintf_write_cam_control(stru
 	if (slot != 0)
 		return -EINVAL;
 
+	mutex_lock(&budget_av->camlock);
+
 	saa7146_setgpio(budget_av->budget.dev, 1, SAA7146_GPIO_OUTLO);
 	udelay(1);
 
 	result = ttpci_budget_debiwrite(&budget_av->budget, DEBICICAM, address & 3, 1, value, 0, 0);
 	if (result == -ETIMEDOUT) {
-		ciintf_slot_shutdown(ca, slot);
+		ciintf_unlocked_slot_shutdown(ca, slot);
 		printk(KERN_INFO "budget-av: cam ejected 5\n");
 	}
+
+	mutex_unlock(&budget_av->camlock);
+
 	return result;
 }
 
@@ -213,6 +241,8 @@ static int ciintf_slot_reset(struct dvb_
 	if (slot != 0)
 		return -EINVAL;
 
+	mutex_lock(&budget_av->camlock);
+
 	dprintk(1, "ciintf_slot_reset\n");
 	budget_av->slot_status = SLOTSTATUS_RESET;
 
@@ -231,10 +261,12 @@ static int ciintf_slot_reset(struct dvb_
 	if (budget_av->reinitialise_demod)
 		dvb_frontend_reinitialise(budget_av->budget.dvb_frontend);
 
+	mutex_unlock(&budget_av->camlock);
+
 	return 0;
 }
 
-static int ciintf_slot_shutdown(struct dvb_ca_en50221 *ca, int slot)
+static int ciintf_unlocked_slot_shutdown(struct dvb_ca_en50221 *ca, int slot)
 {
 	struct budget_av *budget_av = (struct budget_av *) ca->data;
 	struct saa7146_dev *saa = budget_av->budget.dev;
@@ -250,6 +282,18 @@ static int ciintf_slot_shutdown(struct d
 	return 0;
 }
 
+static int ciintf_slot_shutdown(struct dvb_ca_en50221 *ca, int slot)
+{
+	struct budget_av *budget_av = (struct budget_av *) ca->data;
+	int result;
+
+	mutex_lock(&budget_av->camlock);
+	result = ciintf_unlocked_slot_shutdown(ca, slot);
+	mutex_unlock(&budget_av->camlock);
+
+	return result;
+}
+
 static int ciintf_slot_ts_enable(struct dvb_ca_en50221 *ca, int slot)
 {
 	struct budget_av *budget_av = (struct budget_av *) ca->data;
@@ -258,10 +302,13 @@ static int ciintf_slot_ts_enable(struct 
 	if (slot != 0)
 		return -EINVAL;
 
-	dprintk(1, "ciintf_slot_ts_enable: %d\n", budget_av->slot_status);
+	mutex_lock(&budget_av->camlock);
 
+	dprintk(1, "ciintf_slot_ts_enable: %d\n", budget_av->slot_status);
 	ttpci_budget_set_video_port(saa, BUDGET_VIDEO_PORTA);
 
+	mutex_unlock(&budget_av->camlock);
+
 	return 0;
 }
 
@@ -274,6 +321,8 @@ static int ciintf_poll_slot_status(struc
 	if (slot != 0)
 		return -EINVAL;
 
+	mutex_lock(&budget_av->camlock);
+
 	/* test the card detect line - needs to be done carefully
 	 * since it never goes high for some CAMs on this interface (e.g. topuptv) */
 	if (budget_av->slot_status == SLOTSTATUS_NONE) {
@@ -302,8 +351,9 @@ static int ciintf_poll_slot_status(struc
 			printk(KERN_INFO "budget-av: cam inserted B\n");
 		} else if (result < 0) {
 			if (budget_av->slot_status != SLOTSTATUS_NONE) {
-				ciintf_slot_shutdown(ca, slot);
+				ciintf_unlocked_slot_shutdown(ca, slot);
 				printk(KERN_INFO "budget-av: cam ejected 5\n");
+				mutex_unlock(&budget_av->camlock);
 				return 0;
 			}
 		}
@@ -311,20 +361,23 @@ static int ciintf_poll_slot_status(struc
 
 	/* read from attribute memory in reset/ready state to know when the CAM is ready */
 	if (budget_av->slot_status == SLOTSTATUS_RESET) {
-		result = ciintf_read_attribute_mem(ca, slot, 0);
+		result = ciintf_unlocked_read_attribute_mem(ca, slot, 0);
 		if (result == 0x1d) {
 			budget_av->slot_status = SLOTSTATUS_READY;
 		}
 	}
 
 	/* work out correct return code */
+	result = 0;
 	if (budget_av->slot_status != SLOTSTATUS_NONE) {
-		if (budget_av->slot_status & SLOTSTATUS_READY) {
-			return DVB_CA_EN50221_POLL_CAM_PRESENT | DVB_CA_EN50221_POLL_CAM_READY;
-		}
-		return DVB_CA_EN50221_POLL_CAM_PRESENT;
+		result = DVB_CA_EN50221_POLL_CAM_PRESENT;
+		if (budget_av->slot_status & SLOTSTATUS_READY)
+			result |= DVB_CA_EN50221_POLL_CAM_READY;
 	}
-	return 0;
+
+	mutex_unlock(&budget_av->camlock);
+
+	return result;
 }
 
 static int ciintf_init(struct budget_av *budget_av)
@@ -332,6 +385,7 @@ static int ciintf_init(struct budget_av 
 	struct saa7146_dev *saa = budget_av->budget.dev;
 	int result;
 
+	mutex_init(&budget_av->camlock);
 	memset(&budget_av->ca, 0, sizeof(struct dvb_ca_en50221));
 
 	saa7146_setgpio(saa, 0, SAA7146_GPIO_OUTLO);

[-- Attachment #3: dvb_ca_en50221.c.v1.patch --]
[-- Type: text/x-diff, Size: 2340 bytes --]

--- a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c	2008-07-13 23:51:29.000000000 +0200
+++ b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c	2008-08-22 09:05:31.000000000 +0200
@@ -93,6 +93,9 @@ struct dvb_ca_slot {
 	/* current state of the CAM */
 	int slot_state;
 
+	/* slot mutex for serializing data read/write access */
+	struct mutex slotlock;
+
 	/* Number of CAMCHANGES that have occurred since last processing */
 	atomic_t camchange_count;
 
@@ -589,6 +592,8 @@ static int dvb_ca_en50221_read_data(stru
 
 	dprintk("%s\n", __func__);
 
+	mutex_lock(&ca->slot_info[slot].slotlock);
+
 	/* check if we have space for a link buf in the rx_buffer */
 	if (ebuf == NULL) {
 		int buf_free;
@@ -687,6 +692,7 @@ static int dvb_ca_en50221_read_data(stru
 	status = bytes_read;
 
 exit:
+	mutex_unlock(&ca->slot_info[slot].slotlock);
 	return status;
 }
 
@@ -710,15 +716,22 @@ static int dvb_ca_en50221_write_data(str
 
 	dprintk("%s\n", __func__);
 
+	mutex_lock(&ca->slot_info[slot].slotlock);
 
-	// sanity check
+	/* sanity check */
 	if (bytes_write > ca->slot_info[slot].link_buf_size)
 		return -EINVAL;
 
-	/* check if interface is actually waiting for us to read from it, or if a read is in progress */
+	/* it is possible we are dealing with a single buffer implementation,
+	   thus if there is data available for read or if there is even a read
+	   already in progress, we do nothing but awake the kernel thread to
+	   process the data if necessary. */
 	if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0)
 		goto exitnowrite;
 	if (status & (STATUSREG_DA | STATUSREG_RE)) {
+		if (status & STATUSREG_DA)
+			dvb_ca_en50221_thread_wakeup(ca);
+
 		status = -EAGAIN;
 		goto exitnowrite;
 	}
@@ -767,6 +780,7 @@ exit:
 	ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, IRQEN);
 
 exitnowrite:
+	mutex_unlock(&ca->slot_info[slot].slotlock);
 	return status;
 }
 EXPORT_SYMBOL(dvb_ca_en50221_camchange_irq);
@@ -1662,6 +1676,7 @@ int dvb_ca_en50221_init(struct dvb_adapt
 	for (i = 0; i < slot_count; i++) {
 		memset(&ca->slot_info[i], 0, sizeof(struct dvb_ca_slot));
 		ca->slot_info[i].slot_state = DVB_CA_SLOTSTATE_NONE;
+		mutex_init(&ca->slot_info[i].slotlock);
 		atomic_set(&ca->slot_info[i].camchange_count, 0);
 		ca->slot_info[i].camchange_type = DVB_CA_EN50221_CAMCHANGE_REMOVED;
 	}

[-- Attachment #4: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] budget_av / dvb_ca_en50221: fixes ci/cam handling especially on SMP machines

[Oliver Endriss]

Matthias Dahl wrote:
> Hi Oliver.
> 
> I can happily report that with the following two patches applied, I haven't 
> seen a single case where the cam stopped working due to i/o errors or 
> anything like it.
> 
> The budget_av patch is basically your patch just a bit extended which I 
> thought was necessary to cover all relevant cases. Works just fine.
> 
> The dvb_ca_en50221 patch introduces the concept of slot lock that means, you 
> can either read or write to a slot but concurrent i/o on a slot is no longer 
> allowed. This case was already thought of and partly taken care of but 
> unfortunately due to the missing locking mechanism, it just made the race 
> condition harder to trigger but not impossible... especially on SMP systems 
> where this is easier to hit. That's way I introduced a mutex. I left the 
> original check in there but it actually never should get triggered anymore. 
> Right now actually, if it gets triggered, one could assume the ci/cam is in 
> an undefined state and trigger a reinit, like it's done on a few other 
> places.
> 
> Could you please apply those patches to the dvb tree and maybe get into the 
> official 2.6.27? Those bugs haven been around for quite some time now and 
> without the patches, they are not so hard to trigger.

Finally I had some time to review your patches.

budget-av:
Could you please elaborate why 
- ciintf_slot_ts_enable
- ciintf_slot_shutdown
require locking? I cannot see that.

ciintf_slot_reset:
Ok, although I doubt that it will make any difference, because the
routine will kill tuner and CI interface anyway...

ciintf_poll_slot_status:
Hm, I think my version was also correct.


dvb_ca_en50221.c:
Ok. but I need your signed-off-by for this patch.

Btw, wouldn't it be better to remove the locking stuff from budget-av.c,
and do all locking in dvb_ca_en50221.c?

Imho this would be a far better solution (only one mutex, not two).
Could you implement that?

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] budget_av / dvb_ca_en50221: fixes ci/cam handling especially on SMP machines

[Matthias Dahl]

Hello Oliver.

On Saturday 23 August 2008 22:58:39 Oliver Endriss wrote:

> Finally I had some time to review your patches.

No problem. I am currently short on time either so I know what it's like. :-)

> budget-av:
> Could you please elaborate why
> - ciintf_slot_ts_enable
> - ciintf_slot_shutdown
> require locking? I cannot see that.

The thought behind that was to make it impossible that any of all the ciintf 
functions could interfere with each other and thus possibly get the ci intf 
in an undefined state. So to prevent that only one one of them is allowed to 
be active at any given time. But you are right one could have left those out 
of there... sorry, my fault.

> ciintf_slot_reset:
> Ok, although I doubt that it will make any difference, because the
> routine will kill tuner and CI interface anyway...

Basically, see above. :-) Nevertheless I still think we should make absolutely 
certain that nothing interferes with the reset function because otherwise we 
may end up with an inoperational ci system just like the issues I had. So 
having the mutex in there won't hurt at the very least.

> ciintf_poll_slot_status:
> Hm, I think my version was also correct.

Same applies here. I guess I was just too overly cautious trying to get rid of 
those problems. The only reason I saw here was that slot_status might have 
gotten changed when the mutex was released early. But that clearly shouldn't 
cause any harm either. Man, I guess was asleep while doing those. That's 
already kinda embarrassing...

> Btw, wouldn't it be better to remove the locking stuff from budget-av.c,
> and do all locking in dvb_ca_en50221.c?

That was exactly what I was working on before I got your patch but as your 
patch seemed to work fine initially I put that on hold and later just started 
from scratch and only put the locks in [read|write]_data because then I knew 
what was causing the trouble.

> Imho this would be a far better solution (only one mutex, not two).
> Could you implement that?

Actually I wanted to work on it today but I am dealing with some health issues 
so I didn't get around to it. I try to get it done in the next few days. So 
with some extra days for testing, hopefully I will have something next 
weekend.

So long,
matthias.

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] budget_av / dvb_ca_en50221: fixes ci/cam handling especially on SMP machines

[Oliver Endriss]

Matthias Dahl wrote:
> Hello Oliver.
> 
> On Saturday 23 August 2008 22:58:39 Oliver Endriss wrote:
> 
> > Finally I had some time to review your patches.
> 
> No problem. I am currently short on time either so I know what it's like. :-)
> 
> > budget-av:
> > Could you please elaborate why
> > - ciintf_slot_ts_enable
> > - ciintf_slot_shutdown
> > require locking? I cannot see that.
> 
> The thought behind that was to make it impossible that any of all the ciintf 
> functions could interfere with each other and thus possibly get the ci intf 
> in an undefined state. So to prevent that only one one of them is allowed to 
> be active at any given time. But you are right one could have left those out 
> of there... sorry, my fault.
> 
> > ciintf_slot_reset:
> > Ok, although I doubt that it will make any difference, because the
> > routine will kill tuner and CI interface anyway...
> 
> Basically, see above. :-) Nevertheless I still think we should make absolutely 
> certain that nothing interferes with the reset function because otherwise we 
> may end up with an inoperational ci system just like the issues I had. So 
> having the mutex in there won't hurt at the very least.

Agreed.

> > ciintf_poll_slot_status:
> > Hm, I think my version was also correct.
> 
> Same applies here. I guess I was just too overly cautious trying to get rid of 
> those problems. The only reason I saw here was that slot_status might have 
> gotten changed when the mutex was released early. But that clearly shouldn't 
> cause any harm either. Man, I guess was asleep while doing those. That's 
> already kinda embarrassing...
> 
> > Btw, wouldn't it be better to remove the locking stuff from budget-av.c,
> > and do all locking in dvb_ca_en50221.c?
> 
> That was exactly what I was working on before I got your patch but as your 
> patch seemed to work fine initially I put that on hold and later just started 
> from scratch and only put the locks in [read|write]_data because then I knew 
> what was causing the trouble.

When I wrote the budget-av patch, I assumed that dvb_ca_en50221.c was
fine. I read the comments in dvb_ca_en50221.h and noticed that the locks
were missing in budget-av.c.

As the budget-av patch was not sufficient, it is better to handle the
locking in dvb_ca_en50221.c.

> > Imho this would be a far better solution (only one mutex, not two).
> > Could you implement that?

Note that the *_irq functions in dvb_ca_en50221.c might be called from
interrupt context. (Does not apply for budget-av, but for budget-ci.)
Interrupt routines cannot use the mutex stuff...

> Actually I wanted to work on it today but I am dealing with some health issues 
> so I didn't get around to it. I try to get it done in the next few days. So 
> with some extra days for testing, hopefully I will have something next 
> weekend.

No need to hurry. Bug fixes can be submitted to the kernel anytime, even
when the merge window (for new features) has closed.

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] budget_av / dvb_ca_en50221: fixes ci/cam handling especially on SMP machines

[Matthias Dahl]

Hi Oliver.

Just wanted to post a status note that I was too busy to get anything useful 
done. Next weekend should be okay though, finally.

So far I've decided to go with a rather conservative locking instead of a more 
finer grained one because in the end it'll make things easier to handle and 
won't introduce new problems. In more detail:

 - a slot is locked as long as the kernel thread is processing it or if some
   user ioctl needs access to it

 - reading won't introduce any new locks because it doesn't access the h/w
   directly and thus doesn't need locks (only works on the slot ringbuffer)

I hope that this will work out and the longer lock in the kernel thread won't 
introduce new trouble. For the underlying implementations like budget_[av|ci] 
this means they are save from concurrent accesses to one slot but concurrent 
accesses to different slots are still possible and have to be taken care of 
there. Nevertheless this is rather irrelevant at the moment because there is 
no driver using dvb_ca_en50221 yet which provides more than one slot afaik.

Ok... as soon as I have something tested and ready, I'll let you know.

So long,
matthias.

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb