em28xx: return -ENOTTY for tuner + frequency ioctls if the device has no tuner

PATCH In review Frank Schäfer 2 participants 7 messages Jan 13, 2013 – Jan 19, 2013

Subsystems (2)

Media (V4L/DVB) EM28XX VIDEO4LINUX driver

lore

Share & tools

lore

Frank Schäfer

author

2013-01-13 12:50 UTC #0 lore raw

PATCH 1/1 em28xx: return -ENOTTY for tuner + frequency ioctls if the device has no tuner

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-video.c |    8 ++++++++

1 Datei geändert, 8 Zeilen hinzugefügt(+)

drivers/media/usb/em28xx/em28xx-video.c +8 -0

index 2eabf2a..4a7f73c 100644
@@ -1204,6 +1204,8 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1224,6 +1226,8 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1241,6 +1245,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	struct em28xx_fh      *fh  = priv;
 	struct em28xx         *dev = fh->dev;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	if (0 != f->tuner)
 		return -EINVAL;

@@ -1255,6 +1261,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

Signature · 1 line

1.7.10.4

Hans Verkuil

2013-01-14 09:20 UTC #1 lore raw

Quoted 38 lines · On Sun January 13 2013 13:50:50 Frank Schäfer wrote:

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-video.c |    8 ++++++++

1 Datei geändert, 8 Zeilen hinzugefügt(+)

drivers/media/usb/em28xx/em28xx-video.c +6 -0

index 2eabf2a..4a7f73c 100644
@@ -1204,6 +1204,8 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1224,6 +1226,8 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1241,6 +1245,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	struct em28xx_fh      *fh  = priv;
 	struct em28xx         *dev = fh->dev;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	if (0 != f->tuner)
 		return -EINVAL;

drivers/media/usb/em28xx/em28xx-video.c static int vidioc_s_frequency(struct file *file, void *priv, · line 1268

Quoted 8 lines

drivers/media/usb/em28xx/em28xx-video.c +2 -0

@@ -1255,6 +1261,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

Rather than doing this in each ioctl, I recommend using v4l2_disable_ioctl instead. See for example drivers/media/pci/ivtv/ivtv-streams.c.

Regards,

Hans

Frank Schäfer

author

2013-01-15 17:06 UTC #2 lore raw

Am 14.01.2013 10:20, schrieb Hans Verkuil:

Quoted 41 lines · On Sun January 13 2013 13:50:50 Frank Schäfer wrote:

Quoted 40 lines

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-video.c |    8 ++++++++

1 Datei geändert, 8 Zeilen hinzugefügt(+)

drivers/media/usb/em28xx/em28xx-video.c +6 -0

index 2eabf2a..4a7f73c 100644
@@ -1204,6 +1204,8 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1224,6 +1226,8 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1241,6 +1245,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	struct em28xx_fh      *fh  = priv;
 	struct em28xx         *dev = fh->dev;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	if (0 != f->tuner)
 		return -EINVAL;

@@ -1255,6 +1261,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;

Quoted 8 lines

drivers/media/usb/em28xx/em28xx-video.c static int vidioc_g_tuner(struct file *file, void *priv, · line 1211
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-video.c +2 -0
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
Rather than doing this in each ioctl, I recommend using v4l2_disable_ioctl instead. See for example drivers/media/pci/ivtv/ivtv-streams.c.

Hmm, thanks.
I just did the same we currently do for the VIDIOC_G/S/QUERY_STD and VIDIOC_G/S_AUDIO ioctls, but yeah, disabling seems to be better. Btw, what about VIDIOC_G/S_PARAM ? Do they make sense for cameras ?

Regards,
Frank

Hans Verkuil

2013-01-16 08:31 UTC #3 lore raw

Quoted 51 lines · On Tue 15 January 2013 18:06:57 Frank Schäfer wrote:

Am 14.01.2013 10:20, schrieb Hans Verkuil:

Quoted 41 lines · On Sun January 13 2013 13:50:50 Frank Schäfer wrote:

Quoted 40 lines

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-video.c |    8 ++++++++

1 Datei geändert, 8 Zeilen hinzugefügt(+)

drivers/media/usb/em28xx/em28xx-video.c +6 -0

index 2eabf2a..4a7f73c 100644
@@ -1204,6 +1204,8 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1224,6 +1226,8 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1241,6 +1245,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	struct em28xx_fh      *fh  = priv;
 	struct em28xx         *dev = fh->dev;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	if (0 != f->tuner)
 		return -EINVAL;

@@ -1255,6 +1261,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;

Quoted 8 lines

drivers/media/usb/em28xx/em28xx-video.c static int vidioc_g_tuner(struct file *file, void *priv, · line 1211
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-video.c +2 -0
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
Rather than doing this in each ioctl, I recommend using v4l2_disable_ioctl instead. See for example drivers/media/pci/ivtv/ivtv-streams.c.

Quoted 4 lines

Hmm, thanks.
I just did the same we currently do for the VIDIOC_G/S/QUERY_STD and VIDIOC_G/S_AUDIO ioctls, but yeah, disabling seems to be better. Btw, what about VIDIOC_G/S_PARAM ? Do they make sense for cameras ?

Absolutely. Actually, s_parm should be disabled in the non-camera case since s_parm only makes sense for webcams.

Regards,

Hans

Quoted 5 lines

Regards,
Frank

Frank Schäfer

author

2013-01-18 17:13 UTC #4 lore raw

Am 16.01.2013 09:31, schrieb Hans Verkuil:

Quoted 51 lines · On Tue 15 January 2013 18:06:57 Frank Schäfer wrote:

Quoted 50 lines

Am 14.01.2013 10:20, schrieb Hans Verkuil:

Quoted 49 lines · On Sun January 13 2013 13:50:50 Frank Schäfer wrote:

Quoted 38 lines

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-video.c |    8 ++++++++

1 Datei geändert, 8 Zeilen hinzugefügt(+)

drivers/media/usb/em28xx/em28xx-video.c +6 -0

index 2eabf2a..4a7f73c 100644
@@ -1204,6 +1204,8 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1224,6 +1226,8 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

@@ -1241,6 +1245,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	struct em28xx_fh      *fh  = priv;
 	struct em28xx         *dev = fh->dev;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	if (0 != f->tuner)
 		return -EINVAL;

drivers/media/usb/em28xx/em28xx-video.c static int vidioc_s_frequency(struct file *file, void *priv, · line 1268

Quoted 8 lines

drivers/media/usb/em28xx/em28xx-video.c +2 -0

@@ -1255,6 +1261,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;

Rather than doing this in each ioctl, I recommend using v4l2_disable_ioctl instead. See for example drivers/media/pci/ivtv/ivtv-streams.c.

Quoted 6 lines

Quoted 4 lines
Hmm, thanks.
I just did the same we currently do for the VIDIOC_G/S/QUERY_STD and VIDIOC_G/S_AUDIO ioctls, but yeah, disabling seems to be better. Btw, what about VIDIOC_G/S_PARAM ? Do they make sense for cameras ?
Absolutely. Actually, s_parm should be disabled in the non-camera case since s_parm only makes sense for webcams.

Sorry for the delay, I wanted to take a deeper look into the API spec first...

Actually... why do you think VIDIOC_S/G_PARM should be disabled for non-camera devices ?
At least G_PARM seems to make sense, because frame interval and the number of buffers can always be reported.

Two further questions that came up while reading the spec and the driver code:
1) Which ioctls can VBI devices have ? I'm thinking about VIDIOC_G/S_TUNER, VIDIOC_G/S_FREQUENCY, VIDIOC_G/S/QUERY_STD, VIDIOC_G/S_PARM.
2) Are VIDIOC_G/S_AUDIO generally suitable or even mandatory for radio devices ?
3) Supporting VIDIOC_CROPCAP without VIDIOC_G/S_CROP doesn't make sense, right ? Do you know if G/S_CROP has ever been supported by the em28xx driver ?

Regards,
Frank

Hans Verkuil

2013-01-18 19:01 UTC #5 lore raw

Quoted 56 lines · On Fri January 18 2013 18:14:10 Frank Schäfer wrote:

Am 16.01.2013 09:31, schrieb Hans Verkuil:
Quoted 55 lines · On Tue 15 January 2013 18:06:57 Frank Schäfer wrote:
Quoted 54 lines
Am 14.01.2013 10:20, schrieb Hans Verkuil:
Quoted 41 lines · On Sun January 13 2013 13:50:50 Frank Schäfer wrote:
Quoted 40 lines
Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-video.c |    8 ++++++++
1 Datei geändert, 8 Zeilen hinzugefügt(+)
drivers/media/usb/em28xx/em28xx-video.c +6 -0
index 2eabf2a..4a7f73c 100644
@@ -1204,6 +1204,8 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
@@ -1224,6 +1226,8 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
@@ -1241,6 +1245,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	struct em28xx_fh      *fh  = priv;
 	struct em28xx         *dev = fh->dev;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	if (0 != f->tuner)
 		return -EINVAL;
 
@@ -1255,6 +1261,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-video.c static int vidioc_g_tuner(struct file *file, void *priv, · line 1211
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-video.c +2 -0
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
Rather than doing this in each ioctl, I recommend using v4l2_disable_ioctl instead. See for example drivers/media/pci/ivtv/ivtv-streams.c.
Hmm, thanks.
I just did the same we currently do for the VIDIOC_G/S/QUERY_STD and VIDIOC_G/S_AUDIO ioctls, but yeah, disabling seems to be better. Btw, what about VIDIOC_G/S_PARAM ? Do they make sense for cameras ?

Quoted 8 lines

Quoted 2 lines
Absolutely. Actually, s_parm should be disabled in the non-camera case since s_parm only makes sense for webcams.
Sorry for the delay, I wanted to take a deeper look into the API spec first...
Actually... why do you think VIDIOC_S/G_PARM should be disabled for non-camera devices ?

I didn't say that: only S_PARM makes generally no sense for non-camera devices since the fps can't be changed (G/S_PARM are very ugly ioctls, but that's another story).

Quoted 8 lines

At least G_PARM seems to make sense, because frame interval and the number of buffers can always be reported.
Two further questions that came up while reading the spec and the driver code:
1) Which ioctls can VBI devices have ? I'm thinking about VIDIOC_G/S_TUNER, VIDIOC_G/S_FREQUENCY, VIDIOC_G/S/QUERY_STD, VIDIOC_G/S_PARM.

Correct, except for S_PARM.

Quoted 2 lines

2) Are VIDIOC_G/S_AUDIO generally suitable or even mandatory for radio devices ?

Those should not be used for radio devices.

Quoted 2 lines

3) Supporting VIDIOC_CROPCAP without VIDIOC_G/S_CROP doesn't make sense, right ?

Correct. The spec says that CROPCAP is a mandatory ioctl, but that was never enforced and I believe that we decided during the last media summit to correct this in the spec (which hasn't happened yet).

Quoted 2 lines

Do you know if G/S_CROP has ever been supported by the em28xx driver ?

I faintly remember seeing code for this at one time, but I may be wrong. If some patches were posted, then it was a long time ago.

BTW, you are aware of the v4l2-compliance tool? I can't remember if I ever mentioned it to you. It's part of v4l-utils and tests driver compliance with the V4L2 API.

Regards,

Hans

Frank Schäfer

author

2013-01-19 19:13 UTC #6 lore raw

Am 18.01.2013 20:01, schrieb Hans Verkuil:

Quoted 59 lines · On Fri January 18 2013 18:14:10 Frank Schäfer wrote:

Quoted 58 lines
Am 16.01.2013 09:31, schrieb Hans Verkuil:
Quoted 57 lines · On Tue 15 January 2013 18:06:57 Frank Schäfer wrote:
Quoted 50 lines
Am 14.01.2013 10:20, schrieb Hans Verkuil:
Quoted 41 lines · On Sun January 13 2013 13:50:50 Frank Schäfer wrote:
Quoted 40 lines
Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-video.c |    8 ++++++++
1 Datei geändert, 8 Zeilen hinzugefügt(+)
drivers/media/usb/em28xx/em28xx-video.c +6 -0
index 2eabf2a..4a7f73c 100644
@@ -1204,6 +1204,8 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
@@ -1224,6 +1226,8 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
@@ -1241,6 +1245,8 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 	struct em28xx_fh      *fh  = priv;
 	struct em28xx         *dev = fh->dev;
 
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	if (0 != f->tuner)
 		return -EINVAL;
 
@@ -1255,6 +1261,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct em28xx         *dev = fh->dev;
 	int                   rc;
 
Quoted 8 lines
drivers/media/usb/em28xx/em28xx-video.c static int vidioc_g_tuner(struct file *file, void *priv, · line 1211
Quoted 6 lines
drivers/media/usb/em28xx/em28xx-video.c +2 -0
+	if (dev->tuner_type == TUNER_ABSENT)
+		return -ENOTTY;
 	rc = check_dev(dev);
 	if (rc < 0)
 		return rc;
Rather than doing this in each ioctl, I recommend using v4l2_disable_ioctl instead. See for example drivers/media/pci/ivtv/ivtv-streams.c.
Quoted 4 lines
Hmm, thanks.
I just did the same we currently do for the VIDIOC_G/S/QUERY_STD and VIDIOC_G/S_AUDIO ioctls, but yeah, disabling seems to be better. Btw, what about VIDIOC_G/S_PARAM ? Do they make sense for cameras ?
Absolutely. Actually, s_parm should be disabled in the non-camera case since s_parm only makes sense for webcams.

Quoted 8 lines

Quoted 5 lines
Sorry for the delay, I wanted to take a deeper look into the API spec first...
Actually... why do you think VIDIOC_S/G_PARM should be disabled for non-camera devices ?
I didn't say that: only S_PARM makes generally no sense for non-camera devices since the fps can't be changed (G/S_PARM are very ugly ioctls, but that's another story).

Ok, that's how I've understood the spec, too.
The only exception for which S_PARM would make sense for non-camera devices seems to be drivers that support selecting the number of internally used buffers.

Quoted 13 lines

Quoted 9 lines
At least G_PARM seems to make sense, because frame interval and the number of buffers can always be reported.
Two further questions that came up while reading the spec and the driver code:
1) Which ioctls can VBI devices have ? I'm thinking about VIDIOC_G/S_TUNER, VIDIOC_G/S_FREQUENCY, VIDIOC_G/S/QUERY_STD, VIDIOC_G/S_PARM.
Correct, except for S_PARM.
Quoted 2 lines
2) Are VIDIOC_G/S_AUDIO generally suitable or even mandatory for radio devices ?

Quoted 7 lines

Those should not be used for radio devices.
Quoted 2 lines
3) Supporting VIDIOC_CROPCAP without VIDIOC_G/S_CROP doesn't make sense, right ?
Correct. The spec says that CROPCAP is a mandatory ioctl, but that was never enforced and I believe that we decided during the last media summit to correct this in the spec (which hasn't happened yet).

Ah, ok, thanks.

The spec seems to be up-to-date:
"This ioctl must be implemented for video capture or output devices that support cropping and/or scaling and/or have non-square pixels, and for overlay devices."

Quoted 5 lines

Quoted 3 lines
Do you know if G/S_CROP has ever been supported by the em28xx driver ?
I faintly remember seeing code for this at one time, but I may be wrong. If some patches were posted, then it was a long time ago.

Hmmm... must have been even before 2.6.27.

Quoted 3 lines

BTW, you are aware of the v4l2-compliance tool? I can't remember if I ever mentioned it to you. It's part of v4l-utils and tests driver compliance with the V4L2 API.

Yes, I heard about it but never used it up to now. It seems like this is a good reason to test it. :)

I will try to send a patch series that fixes the remaining em28xx ioctl issues tomorrow.
I wouldn't wonder if further questions arise, please be patient with me. ;)

Regards,
Frank