Skip to contentSkip to conversation

ngene: blocking and nonblocking io for sec0

PATCH In review Mauro Carvalho Chehab 3 participants 3 messages Jun 1, 2011 – Jun 21, 2011 Media (V4L/DVB)

lore
Share & tools
lore

Mauro Carvalho Chehab

author
#0 lore raw

PATCH 1/1 ngene: blocking and nonblocking io for sec0

Oliver/Ralph,

Could you please review this patch? On a quick look, it looks fine on my eyes, but I don't have any ngene hardware here for testing.

Thanks!
Mauro

-------- Mensagem original --------
Date: Thu, 12 May 2011 15:47:09 +0200
From: Issa Gorissen <[email protected]>
To: [email protected]
Subject: [PATCH] ngene: blocking and nonblocking io for sec0

Patch allows for blocking or nonblocking io on the ngene sec0 device. It also enforces one reader and one writer at a time.

Signed-off-by Issa Gorissen <[email protected]>

--

linux/drivers/media/dvb/ngene/ngene-dvb.c +48 -21
@@ -53,15 +53,29 @@ static ssize_t ts_write(struct file *fil
 	struct dvb_device *dvbdev = file->private_data;
 	struct ngene_channel *chan = dvbdev->priv;
 	struct ngene *dev = chan->dev;
+	int avail = 0;
+	char nonblock = file->f_flags & O_NONBLOCK;
 
-	if (wait_event_interruptible(dev->tsout_rbuf.queue,
-				     dvb_ringbuffer_free
-				     (&dev->tsout_rbuf) >= count) < 0)
+	if (!count)
 		return 0;
 
-	dvb_ringbuffer_write(&dev->tsout_rbuf, buf, count);
+	if (nonblock) {
+		avail = dvb_ringbuffer_avail(&dev->tsout_rbuf);
+		if (!avail)
+			return -EAGAIN;
+	} else {
+		while (1) {
+			if (wait_event_interruptible(dev->tsout_rbuf.queue,
+						     dvb_ringbuffer_free
+						     (&dev->tsout_rbuf) >= count) >= 0)
+				break;
+		}
+		avail = count;
+	}
+
+	dvb_ringbuffer_write(&dev->tsout_rbuf, buf, avail);
+	return avail;
 
-	return count;
 }
 
 static ssize_t ts_read(struct file *file, char *buf,

@@ -70,22 +84,35 @@ static ssize_t ts_read(struct file *file
 	struct dvb_device *dvbdev = file->private_data;
 	struct ngene_channel *chan = dvbdev->priv;
 	struct ngene *dev = chan->dev;
-	int left, avail;
+	int avail = 0;
+	char nonblock = file->f_flags & O_NONBLOCK;
 
-	left = count;
-	while (left) {
-		if (wait_event_interruptible(
-			    dev->tsin_rbuf.queue,
-			    dvb_ringbuffer_avail(&dev->tsin_rbuf) > 0) < 0)
-			return -EAGAIN;
+	if (!count)
+		return 0;
+
+	if (nonblock) {
 		avail = dvb_ringbuffer_avail(&dev->tsin_rbuf);
-		if (avail > left)
-			avail = left;
-		dvb_ringbuffer_read_user(&dev->tsin_rbuf, buf, avail);
-		left -= avail;
-		buf += avail;
+	} else {
+		while (!avail) {
+			if (wait_event_interruptible(
+				    dev->tsin_rbuf.queue,
+				    dvb_ringbuffer_avail(&dev->tsin_rbuf) > 0) < 0)
+				continue;
+
+			avail = dvb_ringbuffer_avail(&dev->tsin_rbuf);
+		}
 	}
-	return count;
+
+	if (avail > count)
+		avail = count;
+	if (avail > 0)
+		dvb_ringbuffer_read_user(&dev->tsin_rbuf, buf, avail);
+
+	if (!avail)
+		return -EAGAIN;
+	else
+		return avail;
+
 }
 
 static const struct file_operations ci_fops = {

@@ -98,9 +125,9 @@ static const struct file_operations ci_f
 
 struct dvb_device ngene_dvbdev_ci = {
 	.priv    = 0,
-	.readers = -1,
-	.writers = -1,
-	.users   = -1,
+	.readers = 1,
+	.writers = 1,
+	.users   = 2,
 	.fops    = &ci_fops,
 };

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#1 lore raw

Hi,

sorry for the delay, I don't have much time atm.

Quoted 7 lines · On Wednesday 01 June 2011 23:01:52 Mauro Carvalho Chehab wrote:

Oliver/Ralph,

Could you please review this patch? On a quick look, it looks fine on my eyes, but I don't have any ngene hardware here for testing.

Thanks!
Mauro

The patch is not correct. See my comments below.

Quoted 22 lines

-------- Mensagem original --------
Date: Thu, 12 May 2011 15:47:09 +0200
From: Issa Gorissen <[email protected]>
To: [email protected]
Subject: [PATCH] ngene: blocking and nonblocking io for sec0

Patch allows for blocking or nonblocking io on the ngene sec0 device. It also enforces one reader and one writer at a time.

Signed-off-by Issa Gorissen <[email protected]>

--

linux/drivers/media/dvb/ngene/ngene-dvb.c +2 -1
@@ -53,15 +53,29 @@ static ssize_t ts_write(struct file *fil
 	struct dvb_device *dvbdev = file->private_data;
 	struct ngene_channel *chan = dvbdev->priv;
 	struct ngene *dev = chan->dev;
+	int avail = 0;
+	char nonblock = file->f_flags & O_NONBLOCK;
 
-	if (wait_event_interruptible(dev->tsout_rbuf.queue,

linux/drivers/media/dvb/ngene/ngene-dvb.c static ssize_t ts_write(struct file *fil · line 65

Quoted 10 lines
linux/drivers/media/dvb/ngene/ngene-dvb.c +5 -3
-				     dvb_ringbuffer_free
-				     (&dev->tsout_rbuf) >= count) < 0)
+	if (!count)
 		return 0;
 
-	dvb_ringbuffer_write(&dev->tsout_rbuf, buf, count);
+	if (nonblock) {
+		avail = dvb_ringbuffer_avail(&dev->tsout_rbuf);
+		if (!avail)
+			return -EAGAIN;

Wrong. dvb_ringbuffer_avail() returns the number of bytes waiting in the ring buffer. The code must use dvb_ringbuffer_free() instead.

Furthermore the code completely ignores count (the number of bytes to be written).

linux/drivers/media/dvb/ngene/ngene-dvb.c static ssize_t ts_write(struct file *fil · line 72

Quoted 7 lines
linux/drivers/media/dvb/ngene/ngene-dvb.c +7 -0
+	} else {
+		while (1) {
+			if (wait_event_interruptible(dev->tsout_rbuf.queue,
+						     dvb_ringbuffer_free
+						     (&dev->tsout_rbuf) >= count) >= 0)
+				break;
+		}

What is this loop supposed to do?

Quoted 33 lines
linux/drivers/media/dvb/ngene/ngene-dvb.c +11 -12
+		avail = count;
+	}
+
+	dvb_ringbuffer_write(&dev->tsout_rbuf, buf, avail);
+	return avail;
 
-	return count;
 }
 
 static ssize_t ts_read(struct file *file, char *buf,

@@ -70,22 +84,35 @@ static ssize_t ts_read(struct file *file
 	struct dvb_device *dvbdev = file->private_data;
 	struct ngene_channel *chan = dvbdev->priv;
 	struct ngene *dev = chan->dev;
-	int left, avail;
+	int avail = 0;
+	char nonblock = file->f_flags & O_NONBLOCK;
 
-	left = count;
-	while (left) {
-		if (wait_event_interruptible(
-			    dev->tsin_rbuf.queue,
-			    dvb_ringbuffer_avail(&dev->tsin_rbuf) > 0) < 0)
-			return -EAGAIN;
+	if (!count)
+		return 0;
+
+	if (nonblock) {
 		avail = dvb_ringbuffer_avail(&dev->tsin_rbuf);
-		if (avail > left)
-			avail = left;
-		dvb_ringbuffer_read_user(&dev->tsin_rbuf, buf, avail);
-		left -= avail;

linux/drivers/media/dvb/ngene/ngene-dvb.c static ssize_t ts_read(struct file *file · line 103

Quoted 10 lines
linux/drivers/media/dvb/ngene/ngene-dvb.c +9 -1
-		buf += avail;
+	} else {
+		while (!avail) {
+			if (wait_event_interruptible(
+				    dev->tsin_rbuf.queue,
+				    dvb_ringbuffer_avail(&dev->tsin_rbuf) > 0) < 0)
+				continue;
+
+			avail = dvb_ringbuffer_avail(&dev->tsin_rbuf);
+		}

This piece of code is also wrong. The loop does not wait, until there is enough data available. It does not consider 'count'. Furthermore 'count' might be larger than the size of the ring buffer.

The original code was correct, except that it did not handle O_NONBLOCK.

Quoted 17 lines
linux/drivers/media/dvb/ngene/ngene-dvb.c +11 -1
 	}
-	return count;
+
+	if (avail > count)
+		avail = count;
+	if (avail > 0)
+		dvb_ringbuffer_read_user(&dev->tsin_rbuf, buf, avail);
+
+	if (!avail)
+		return -EAGAIN;
+	else
+		return avail;
+
 }
 
 static const struct file_operations ci_fops = {

linux/drivers/media/dvb/ngene/ngene-dvb.c static const struct file_operations ci_f · line 130

Quoted 9 lines
linux/drivers/media/dvb/ngene/ngene-dvb.c +3 -3
@@ -98,9 +125,9 @@ static const struct file_operations ci_f
 
 struct dvb_device ngene_dvbdev_ci = {
 	.priv    = 0,
-	.readers = -1,
-	.writers = -1,
-	.users   = -1,
+	.readers = 1,
+	.writers = 1,
+	.users   = 2,

Ok.

linux/drivers/media/dvb/ngene/ngene-dvb.c static const struct file_operations ci_f · line 132

Quoted 2 lines
linux/drivers/media/dvb/ngene/ngene-dvb.c +0 -0
 	.fops    = &ci_fops,
 };

CU
Oliver

Signature · 5 lines
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------
#2 lore raw

Haven't fully understood the usage of wake_event_interruptible, now I have read its description. Sorry for the lame patch.

Please find a fixed version below.

Patch allows for blocking or nonblocking io on the ngene sec0 device. It also enforces one reader and one writer at a time.

Signed-off-by Issa Gorissen <[email protected]>

--

linux/drivers/media/dvb/ngene/ngene-dvb.c +43 -21
@@ -53,15 +53,30 @@ static ssize_t ts_write(struct file *fil
 	struct dvb_device *dvbdev = file->private_data;
 	struct ngene_channel *chan = dvbdev->priv;
 	struct ngene *dev = chan->dev;
+	int avail = 0;
+	char nonblock = file->f_flags & O_NONBLOCK;
 
-	if (wait_event_interruptible(dev->tsout_rbuf.queue,
-				     dvb_ringbuffer_free
-				     (&dev->tsout_rbuf) >= count) < 0)
+	if (!count)
 		return 0;
 
-	dvb_ringbuffer_write(&dev->tsout_rbuf, buf, count);
+	if (nonblock) {
+		avail = dvb_ringbuffer_free(&dev->tsout_rbuf);
+		if (!avail)
+			return -EAGAIN;
+		if (count < avail)
+			avail = count;
+	} else {
+		if (wait_event_interruptible(dev->tsout_rbuf.queue,
+					     dvb_ringbuffer_free
+					     (&dev->tsout_rbuf) >= count) < 0)
+			return -EINTR;
+
+		avail = count;
+	}
+
+	dvb_ringbuffer_write(&dev->tsout_rbuf, buf, avail);
+	return avail;
 
-	return count;
 }
 
 static ssize_t ts_read(struct file *file, char *buf,

@@ -70,22 +85,29 @@ static ssize_t ts_read(struct file *file
 	struct dvb_device *dvbdev = file->private_data;
 	struct ngene_channel *chan = dvbdev->priv;
 	struct ngene *dev = chan->dev;
-	int left, avail;
+	int avail = 0;
+	char nonblock = file->f_flags & O_NONBLOCK;
 
-	left = count;
-	while (left) {
-		if (wait_event_interruptible(
-			    dev->tsin_rbuf.queue,
-			    dvb_ringbuffer_avail(&dev->tsin_rbuf) > 0) < 0)
-			return -EAGAIN;
+	if (!count)
+		return 0;
+
+	if (nonblock) {
 		avail = dvb_ringbuffer_avail(&dev->tsin_rbuf);
-		if (avail > left)
-			avail = left;
-		dvb_ringbuffer_read_user(&dev->tsin_rbuf, buf, avail);
-		left -= avail;
-		buf += avail;
+		if (!avail)
+			return -EAGAIN;
+		if (avail > count)
+			avail = count;
+	} else {
+		if (wait_event_interruptible(dev->tsin_rbuf.queue,
+					     dvb_ringbuffer_avail
+					     (&dev->tsin_rbuf) >= count) < 0)
+			return -EINTR;
+
+		avail = count;
 	}
-	return count;
+
+	dvb_ringbuffer_read_user(&dev->tsin_rbuf, buf, avail);
+	return avail;
 }
 
 static const struct file_operations ci_fops = {

@@ -98,9 +120,9 @@ static const struct file_operations ci_f
 
 struct dvb_device ngene_dvbdev_ci = {
 	.priv    = 0,
-	.readers = -1,
-	.writers = -1,
-	.users   = -1,
+	.readers = 1,
+	.writers = 1,
+	.users   = 2,
 	.fops    = &ci_fops,
 };