* [RFCv2 PATCH 01/15] v4l2-dev: use mutex_lock_interruptible instead of plain mutex_lock
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
@ 2010-11-16 21:55 ` Hans Verkuil
2010-11-17 8:23 ` Arnd Bergmann
2010-11-16 21:55 ` [RFCv2 PATCH 02/15] BKL: trivial BKL removal from V4L2 radio drivers Hans Verkuil
` (13 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:55 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Where reasonable use mutex_lock_interruptible instead of mutex_lock.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/v4l2-dev.c | 44 +++++++++++++++++++++++++++++----------
1 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 0ca7978..8eb0756 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -191,8 +191,12 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf,
if (!vdev->fops->read)
return -EINVAL;
- if (vdev->lock)
- mutex_lock(vdev->lock);
+ if (vdev->lock) {
+ int res = mutex_lock_interruptible(vdev->lock);
+
+ if (res)
+ return res;
+ }
if (video_is_registered(vdev))
ret = vdev->fops->read(filp, buf, sz, off);
if (vdev->lock)
@@ -208,8 +212,12 @@ static ssize_t v4l2_write(struct file *filp, const char __user *buf,
if (!vdev->fops->write)
return -EINVAL;
- if (vdev->lock)
- mutex_lock(vdev->lock);
+ if (vdev->lock) {
+ int res = mutex_lock_interruptible(vdev->lock);
+
+ if (res)
+ return res;
+ }
if (video_is_registered(vdev))
ret = vdev->fops->write(filp, buf, sz, off);
if (vdev->lock)
@@ -224,8 +232,8 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
if (!vdev->fops->poll)
return ret;
- if (vdev->lock)
- mutex_lock(vdev->lock);
+ if (vdev->lock && mutex_lock_interruptible(vdev->lock))
+ return ret;
if (video_is_registered(vdev))
ret = vdev->fops->poll(filp, poll);
if (vdev->lock)
@@ -239,8 +247,12 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
int ret = -ENODEV;
if (vdev->fops->unlocked_ioctl) {
- if (vdev->lock)
- mutex_lock(vdev->lock);
+ if (vdev->lock) {
+ int res = mutex_lock_interruptible(vdev->lock);
+
+ if (res)
+ return res;
+ }
if (video_is_registered(vdev))
ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
if (vdev->lock)
@@ -264,8 +276,12 @@ static int v4l2_mmap(struct file *filp, struct vm_area_struct *vm)
if (!vdev->fops->mmap)
return ret;
- if (vdev->lock)
- mutex_lock(vdev->lock);
+ if (vdev->lock) {
+ int res = mutex_lock_interruptible(vdev->lock);
+
+ if (res)
+ return res;
+ }
if (video_is_registered(vdev))
ret = vdev->fops->mmap(filp, vm);
if (vdev->lock)
@@ -291,8 +307,11 @@ static int v4l2_open(struct inode *inode, struct file *filp)
video_get(vdev);
mutex_unlock(&videodev_lock);
if (vdev->fops->open) {
- if (vdev->lock)
- mutex_lock(vdev->lock);
+ if (vdev->lock) {
+ ret = mutex_lock_interruptible(vdev->lock);
+ if (ret)
+ goto err;
+ }
if (video_is_registered(vdev))
ret = vdev->fops->open(filp);
else
@@ -301,6 +320,7 @@ static int v4l2_open(struct inode *inode, struct file *filp)
mutex_unlock(vdev->lock);
}
+err:
/* decrease the refcount in case of an error */
if (ret)
video_put(vdev);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 02/15] BKL: trivial BKL removal from V4L2 radio drivers
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
2010-11-16 21:55 ` [RFCv2 PATCH 01/15] v4l2-dev: use mutex_lock_interruptible instead of plain mutex_lock Hans Verkuil
@ 2010-11-16 21:55 ` Hans Verkuil
2010-11-16 21:55 ` [RFCv2 PATCH 03/15] cadet: use unlocked_ioctl Hans Verkuil
` (12 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:55 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
The patch converts a bunch of V4L2 radio drivers to unlocked_ioctl.
These are all simple conversions: most already had a lock and so the ioctl
fop could simply be replaced by unlocked_ioctl.
radio-miropcm20.c was converted to use the new V4L2 core lock.
While doing this work I noticed that many of these drivers initialized
some more fields or muted audio or something like that *after* creating
the device node. This should be done before the device node is created
to prevent problems. Especially hal tends to grab a device node as soon
as it is created.
In one or two cases the mutex_init was even done after the device creation!
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/radio/radio-aimslab.c | 16 ++++++++--------
drivers/media/radio/radio-aztech.c | 6 +++---
drivers/media/radio/radio-gemtek-pci.c | 6 +++---
drivers/media/radio/radio-gemtek.c | 14 +++++++-------
drivers/media/radio/radio-maestro.c | 14 ++++++--------
drivers/media/radio/radio-maxiradio.c | 2 +-
drivers/media/radio/radio-miropcm20.c | 6 ++++--
drivers/media/radio/radio-rtrack2.c | 10 +++++-----
drivers/media/radio/radio-sf16fmi.c | 7 ++++---
drivers/media/radio/radio-sf16fmr2.c | 11 +++++------
drivers/media/radio/radio-terratec.c | 8 ++++----
drivers/media/radio/radio-trust.c | 18 +++++++++---------
drivers/media/radio/radio-zoltrix.c | 30 +++++++++++++++---------------
13 files changed, 74 insertions(+), 74 deletions(-)
diff --git a/drivers/media/radio/radio-aimslab.c b/drivers/media/radio/radio-aimslab.c
index 5bf4985..05e832f 100644
--- a/drivers/media/radio/radio-aimslab.c
+++ b/drivers/media/radio/radio-aimslab.c
@@ -361,7 +361,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations rtrack_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops rtrack_ioctl_ops = {
@@ -412,13 +412,6 @@ static int __init rtrack_init(void)
rt->vdev.release = video_device_release_empty;
video_set_drvdata(&rt->vdev, rt);
- if (video_register_device(&rt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
- v4l2_device_unregister(&rt->v4l2_dev);
- release_region(rt->io, 2);
- return -EINVAL;
- }
- v4l2_info(v4l2_dev, "AIMSlab RadioTrack/RadioReveal card driver.\n");
-
/* Set up the I/O locking */
mutex_init(&rt->lock);
@@ -430,6 +423,13 @@ static int __init rtrack_init(void)
sleep_delay(2000000); /* make sure it's totally down */
outb(0xc0, rt->io); /* steady volume, mute card */
+ if (video_register_device(&rt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+ v4l2_device_unregister(&rt->v4l2_dev);
+ release_region(rt->io, 2);
+ return -EINVAL;
+ }
+ v4l2_info(v4l2_dev, "AIMSlab RadioTrack/RadioReveal card driver.\n");
+
return 0;
}
diff --git a/drivers/media/radio/radio-aztech.c b/drivers/media/radio/radio-aztech.c
index c223113..dd8a6ab 100644
--- a/drivers/media/radio/radio-aztech.c
+++ b/drivers/media/radio/radio-aztech.c
@@ -324,7 +324,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
static const struct v4l2_file_operations aztech_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops aztech_ioctl_ops = {
@@ -375,6 +375,8 @@ static int __init aztech_init(void)
az->vdev.ioctl_ops = &aztech_ioctl_ops;
az->vdev.release = video_device_release_empty;
video_set_drvdata(&az->vdev, az);
+ /* mute card - prevents noisy bootups */
+ outb(0, az->io);
if (video_register_device(&az->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
v4l2_device_unregister(v4l2_dev);
@@ -383,8 +385,6 @@ static int __init aztech_init(void)
}
v4l2_info(v4l2_dev, "Aztech radio card driver v1.00/19990224 rkroll@exploits.org\n");
- /* mute card - prevents noisy bootups */
- outb(0, az->io);
return 0;
}
diff --git a/drivers/media/radio/radio-gemtek-pci.c b/drivers/media/radio/radio-gemtek-pci.c
index 7903967..28fa85b 100644
--- a/drivers/media/radio/radio-gemtek-pci.c
+++ b/drivers/media/radio/radio-gemtek-pci.c
@@ -361,7 +361,7 @@ MODULE_DEVICE_TABLE(pci, gemtek_pci_id);
static const struct v4l2_file_operations gemtek_pci_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops gemtek_pci_ioctl_ops = {
@@ -422,11 +422,11 @@ static int __devinit gemtek_pci_probe(struct pci_dev *pdev, const struct pci_dev
card->vdev.release = video_device_release_empty;
video_set_drvdata(&card->vdev, card);
+ gemtek_pci_mute(card);
+
if (video_register_device(&card->vdev, VFL_TYPE_RADIO, nr_radio) < 0)
goto err_video;
- gemtek_pci_mute(card);
-
v4l2_info(v4l2_dev, "Gemtek PCI Radio (rev. %d) found at 0x%04x-0x%04x.\n",
pdev->revision, card->iobase, card->iobase + card->length - 1);
diff --git a/drivers/media/radio/radio-gemtek.c b/drivers/media/radio/radio-gemtek.c
index 73985f6..2599364 100644
--- a/drivers/media/radio/radio-gemtek.c
+++ b/drivers/media/radio/radio-gemtek.c
@@ -378,7 +378,7 @@ static int gemtek_probe(struct gemtek *gt)
static const struct v4l2_file_operations gemtek_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static int vidioc_querycap(struct file *file, void *priv,
@@ -577,12 +577,6 @@ static int __init gemtek_init(void)
gt->vdev.release = video_device_release_empty;
video_set_drvdata(>->vdev, gt);
- if (video_register_device(>->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
- v4l2_device_unregister(v4l2_dev);
- release_region(gt->io, 1);
- return -EBUSY;
- }
-
/* Set defaults */
gt->lastfreq = GEMTEK_LOWFREQ;
gt->bu2614data = 0;
@@ -590,6 +584,12 @@ static int __init gemtek_init(void)
if (initmute)
gemtek_mute(gt);
+ if (video_register_device(>->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+ v4l2_device_unregister(v4l2_dev);
+ release_region(gt->io, 1);
+ return -EBUSY;
+ }
+
return 0;
}
diff --git a/drivers/media/radio/radio-maestro.c b/drivers/media/radio/radio-maestro.c
index 08f1051..6af61bf 100644
--- a/drivers/media/radio/radio-maestro.c
+++ b/drivers/media/radio/radio-maestro.c
@@ -299,7 +299,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations maestro_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops maestro_ioctl_ops = {
@@ -383,22 +383,20 @@ static int __devinit maestro_probe(struct pci_dev *pdev,
dev->vdev.release = video_device_release_empty;
video_set_drvdata(&dev->vdev, dev);
+ if (!radio_power_on(dev)) {
+ retval = -EIO;
+ goto errfr1;
+ }
+
retval = video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr);
if (retval) {
v4l2_err(v4l2_dev, "can't register video device!\n");
goto errfr1;
}
- if (!radio_power_on(dev)) {
- retval = -EIO;
- goto errunr;
- }
-
v4l2_info(v4l2_dev, "version " DRIVER_VERSION "\n");
return 0;
-errunr:
- video_unregister_device(&dev->vdev);
errfr1:
v4l2_device_unregister(v4l2_dev);
errfr:
diff --git a/drivers/media/radio/radio-maxiradio.c b/drivers/media/radio/radio-maxiradio.c
index 255d40d..6459a22 100644
--- a/drivers/media/radio/radio-maxiradio.c
+++ b/drivers/media/radio/radio-maxiradio.c
@@ -346,7 +346,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
static const struct v4l2_file_operations maxiradio_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops maxiradio_ioctl_ops = {
diff --git a/drivers/media/radio/radio-miropcm20.c b/drivers/media/radio/radio-miropcm20.c
index 4ff8854..3fb76e3 100644
--- a/drivers/media/radio/radio-miropcm20.c
+++ b/drivers/media/radio/radio-miropcm20.c
@@ -33,6 +33,7 @@ struct pcm20 {
unsigned long freq;
int muted;
struct snd_miro_aci *aci;
+ struct mutex lock;
};
static struct pcm20 pcm20_card = {
@@ -72,7 +73,7 @@ static int pcm20_setfreq(struct pcm20 *dev, unsigned long freq)
static const struct v4l2_file_operations pcm20_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static int vidioc_querycap(struct file *file, void *priv,
@@ -229,7 +230,7 @@ static int __init pcm20_init(void)
return -ENODEV;
}
strlcpy(v4l2_dev->name, "miropcm20", sizeof(v4l2_dev->name));
-
+ mutex_init(&dev->lock);
res = v4l2_device_register(NULL, v4l2_dev);
if (res < 0) {
@@ -242,6 +243,7 @@ static int __init pcm20_init(void)
dev->vdev.fops = &pcm20_fops;
dev->vdev.ioctl_ops = &pcm20_ioctl_ops;
dev->vdev.release = video_device_release_empty;
+ dev->vdev.lock = &dev->lock;
video_set_drvdata(&dev->vdev, dev);
if (video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr) < 0)
diff --git a/drivers/media/radio/radio-rtrack2.c b/drivers/media/radio/radio-rtrack2.c
index a79296a..8d6ea59 100644
--- a/drivers/media/radio/radio-rtrack2.c
+++ b/drivers/media/radio/radio-rtrack2.c
@@ -266,7 +266,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations rtrack2_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops rtrack2_ioctl_ops = {
@@ -315,6 +315,10 @@ static int __init rtrack2_init(void)
dev->vdev.release = video_device_release_empty;
video_set_drvdata(&dev->vdev, dev);
+ /* mute card - prevents noisy bootups */
+ outb(1, dev->io);
+ dev->muted = 1;
+
mutex_init(&dev->lock);
if (video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
v4l2_device_unregister(v4l2_dev);
@@ -324,10 +328,6 @@ static int __init rtrack2_init(void)
v4l2_info(v4l2_dev, "AIMSlab Radiotrack II card driver.\n");
- /* mute card - prevents noisy bootups */
- outb(1, dev->io);
- dev->muted = 1;
-
return 0;
}
diff --git a/drivers/media/radio/radio-sf16fmi.c b/drivers/media/radio/radio-sf16fmi.c
index 985359d..b5a5f89 100644
--- a/drivers/media/radio/radio-sf16fmi.c
+++ b/drivers/media/radio/radio-sf16fmi.c
@@ -260,7 +260,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations fmi_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops fmi_ioctl_ops = {
@@ -382,6 +382,9 @@ static int __init fmi_init(void)
mutex_init(&fmi->lock);
+ /* mute card - prevents noisy bootups */
+ fmi_mute(fmi);
+
if (video_register_device(&fmi->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
v4l2_device_unregister(v4l2_dev);
release_region(fmi->io, 2);
@@ -391,8 +394,6 @@ static int __init fmi_init(void)
}
v4l2_info(v4l2_dev, "card driver at 0x%x\n", fmi->io);
- /* mute card - prevents noisy bootups */
- fmi_mute(fmi);
return 0;
}
diff --git a/drivers/media/radio/radio-sf16fmr2.c b/drivers/media/radio/radio-sf16fmr2.c
index 52c7bbb..dc3f04c 100644
--- a/drivers/media/radio/radio-sf16fmr2.c
+++ b/drivers/media/radio/radio-sf16fmr2.c
@@ -376,7 +376,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations fmr2_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops fmr2_ioctl_ops = {
@@ -424,6 +424,10 @@ static int __init fmr2_init(void)
fmr2->vdev.release = video_device_release_empty;
video_set_drvdata(&fmr2->vdev, fmr2);
+ /* mute card - prevents noisy bootups */
+ fmr2_mute(fmr2->io);
+ fmr2_product_info(fmr2);
+
if (video_register_device(&fmr2->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
v4l2_device_unregister(v4l2_dev);
release_region(fmr2->io, 2);
@@ -431,11 +435,6 @@ static int __init fmr2_init(void)
}
v4l2_info(v4l2_dev, "SF16FMR2 radio card driver at 0x%x.\n", fmr2->io);
- /* mute card - prevents noisy bootups */
- mutex_lock(&fmr2->lock);
- fmr2_mute(fmr2->io);
- fmr2_product_info(fmr2);
- mutex_unlock(&fmr2->lock);
debug_print((KERN_DEBUG "card_type %d\n", fmr2->card_type));
return 0;
}
diff --git a/drivers/media/radio/radio-terratec.c b/drivers/media/radio/radio-terratec.c
index fc1c860..a326639 100644
--- a/drivers/media/radio/radio-terratec.c
+++ b/drivers/media/radio/radio-terratec.c
@@ -338,7 +338,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations terratec_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops terratec_ioctl_ops = {
@@ -389,6 +389,9 @@ static int __init terratec_init(void)
mutex_init(&tt->lock);
+ /* mute card - prevents noisy bootups */
+ tt_write_vol(tt, 0);
+
if (video_register_device(&tt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
v4l2_device_unregister(&tt->v4l2_dev);
release_region(tt->io, 2);
@@ -396,9 +399,6 @@ static int __init terratec_init(void)
}
v4l2_info(v4l2_dev, "TERRATEC ActivRadio Standalone card driver.\n");
-
- /* mute card - prevents noisy bootups */
- tt_write_vol(tt, 0);
return 0;
}
diff --git a/drivers/media/radio/radio-trust.c b/drivers/media/radio/radio-trust.c
index 9d6dcf8..22fa9cc 100644
--- a/drivers/media/radio/radio-trust.c
+++ b/drivers/media/radio/radio-trust.c
@@ -344,7 +344,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations trust_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops trust_ioctl_ops = {
@@ -396,14 +396,6 @@ static int __init trust_init(void)
tr->vdev.release = video_device_release_empty;
video_set_drvdata(&tr->vdev, tr);
- if (video_register_device(&tr->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
- v4l2_device_unregister(v4l2_dev);
- release_region(tr->io, 2);
- return -EINVAL;
- }
-
- v4l2_info(v4l2_dev, "Trust FM Radio card driver v1.0.\n");
-
write_i2c(tr, 2, TDA7318_ADDR, 0x80); /* speaker att. LF = 0 dB */
write_i2c(tr, 2, TDA7318_ADDR, 0xa0); /* speaker att. RF = 0 dB */
write_i2c(tr, 2, TDA7318_ADDR, 0xc0); /* speaker att. LR = 0 dB */
@@ -418,6 +410,14 @@ static int __init trust_init(void)
/* mute card - prevents noisy bootups */
tr_setmute(tr, 1);
+ if (video_register_device(&tr->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+ v4l2_device_unregister(v4l2_dev);
+ release_region(tr->io, 2);
+ return -EINVAL;
+ }
+
+ v4l2_info(v4l2_dev, "Trust FM Radio card driver v1.0.\n");
+
return 0;
}
diff --git a/drivers/media/radio/radio-zoltrix.c b/drivers/media/radio/radio-zoltrix.c
index f31eab9..af99c5b 100644
--- a/drivers/media/radio/radio-zoltrix.c
+++ b/drivers/media/radio/radio-zoltrix.c
@@ -377,7 +377,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
static const struct v4l2_file_operations zoltrix_fops =
{
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops zoltrix_ioctl_ops = {
@@ -424,20 +424,6 @@ static int __init zoltrix_init(void)
return res;
}
- strlcpy(zol->vdev.name, v4l2_dev->name, sizeof(zol->vdev.name));
- zol->vdev.v4l2_dev = v4l2_dev;
- zol->vdev.fops = &zoltrix_fops;
- zol->vdev.ioctl_ops = &zoltrix_ioctl_ops;
- zol->vdev.release = video_device_release_empty;
- video_set_drvdata(&zol->vdev, zol);
-
- if (video_register_device(&zol->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
- v4l2_device_unregister(v4l2_dev);
- release_region(zol->io, 2);
- return -EINVAL;
- }
- v4l2_info(v4l2_dev, "Zoltrix Radio Plus card driver.\n");
-
mutex_init(&zol->lock);
/* mute card - prevents noisy bootups */
@@ -452,6 +438,20 @@ static int __init zoltrix_init(void)
zol->curvol = 0;
zol->stereo = 1;
+ strlcpy(zol->vdev.name, v4l2_dev->name, sizeof(zol->vdev.name));
+ zol->vdev.v4l2_dev = v4l2_dev;
+ zol->vdev.fops = &zoltrix_fops;
+ zol->vdev.ioctl_ops = &zoltrix_ioctl_ops;
+ zol->vdev.release = video_device_release_empty;
+ video_set_drvdata(&zol->vdev, zol);
+
+ if (video_register_device(&zol->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+ v4l2_device_unregister(v4l2_dev);
+ release_region(zol->io, 2);
+ return -EINVAL;
+ }
+ v4l2_info(v4l2_dev, "Zoltrix Radio Plus card driver.\n");
+
return 0;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 03/15] cadet: use unlocked_ioctl
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
2010-11-16 21:55 ` [RFCv2 PATCH 01/15] v4l2-dev: use mutex_lock_interruptible instead of plain mutex_lock Hans Verkuil
2010-11-16 21:55 ` [RFCv2 PATCH 02/15] BKL: trivial BKL removal from V4L2 radio drivers Hans Verkuil
@ 2010-11-16 21:55 ` Hans Verkuil
2010-11-16 21:55 ` [RFCv2 PATCH 04/15] tea5764: convert to unlocked_ioctl Hans Verkuil
` (11 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:55 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Converted from ioctl to unlocked_ioctl.
This driver already used an internal lock, but it was missing in cadet_open and
cadet_release and it was not used correctly in cadet_read.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/radio/radio-cadet.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
index b701ea6..bc9ad08 100644
--- a/drivers/media/radio/radio-cadet.c
+++ b/drivers/media/radio/radio-cadet.c
@@ -328,11 +328,10 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
unsigned char readbuf[RDS_BUFFER];
int i = 0;
+ mutex_lock(&dev->lock);
if (dev->rdsstat == 0) {
- mutex_lock(&dev->lock);
dev->rdsstat = 1;
outb(0x80, dev->io); /* Select RDS fifo */
- mutex_unlock(&dev->lock);
init_timer(&dev->readtimer);
dev->readtimer.function = cadet_handler;
dev->readtimer.data = (unsigned long)dev;
@@ -340,12 +339,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
add_timer(&dev->readtimer);
}
if (dev->rdsin == dev->rdsout) {
+ mutex_unlock(&dev->lock);
if (file->f_flags & O_NONBLOCK)
return -EWOULDBLOCK;
interruptible_sleep_on(&dev->read_queue);
+ mutex_lock(&dev->lock);
}
while (i < count && dev->rdsin != dev->rdsout)
readbuf[i++] = dev->rdsbuf[dev->rdsout++];
+ mutex_unlock(&dev->lock);
if (copy_to_user(data, readbuf, i))
return -EFAULT;
@@ -525,9 +527,11 @@ static int cadet_open(struct file *file)
{
struct cadet *dev = video_drvdata(file);
+ mutex_lock(&dev->lock);
dev->users++;
if (1 == dev->users)
init_waitqueue_head(&dev->read_queue);
+ mutex_unlock(&dev->lock);
return 0;
}
@@ -535,11 +539,13 @@ static int cadet_release(struct file *file)
{
struct cadet *dev = video_drvdata(file);
+ mutex_lock(&dev->lock);
dev->users--;
if (0 == dev->users) {
del_timer_sync(&dev->readtimer);
dev->rdsstat = 0;
}
+ mutex_unlock(&dev->lock);
return 0;
}
@@ -559,7 +565,7 @@ static const struct v4l2_file_operations cadet_fops = {
.open = cadet_open,
.release = cadet_release,
.read = cadet_read,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.poll = cadet_poll,
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 04/15] tea5764: convert to unlocked_ioctl
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (2 preceding siblings ...)
2010-11-16 21:55 ` [RFCv2 PATCH 03/15] cadet: use unlocked_ioctl Hans Verkuil
@ 2010-11-16 21:55 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 05/15] si4713: " Hans Verkuil
` (10 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:55 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Convert from ioctl to unlocked_ioctl using the v4l2 core lock.
Also removed the 'exclusive access' limitation. There was no need for it
and it violates the v4l2 spec as well.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/radio/radio-tea5764.c | 49 ++++++----------------------------
1 files changed, 9 insertions(+), 40 deletions(-)
diff --git a/drivers/media/radio/radio-tea5764.c b/drivers/media/radio/radio-tea5764.c
index 789d2ec..0e71d81 100644
--- a/drivers/media/radio/radio-tea5764.c
+++ b/drivers/media/radio/radio-tea5764.c
@@ -142,7 +142,6 @@ struct tea5764_device {
struct video_device *videodev;
struct tea5764_regs regs;
struct mutex mutex;
- int users;
};
/* I2C code related */
@@ -458,41 +457,10 @@ static int vidioc_s_audio(struct file *file, void *priv,
return 0;
}
-static int tea5764_open(struct file *file)
-{
- /* Currently we support only one device */
- struct tea5764_device *radio = video_drvdata(file);
-
- mutex_lock(&radio->mutex);
- /* Only exclusive access */
- if (radio->users) {
- mutex_unlock(&radio->mutex);
- return -EBUSY;
- }
- radio->users++;
- mutex_unlock(&radio->mutex);
- file->private_data = radio;
- return 0;
-}
-
-static int tea5764_close(struct file *file)
-{
- struct tea5764_device *radio = video_drvdata(file);
-
- if (!radio)
- return -ENODEV;
- mutex_lock(&radio->mutex);
- radio->users--;
- mutex_unlock(&radio->mutex);
- return 0;
-}
-
/* File system interface */
static const struct v4l2_file_operations tea5764_fops = {
.owner = THIS_MODULE,
- .open = tea5764_open,
- .release = tea5764_close,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops tea5764_ioctl_ops = {
@@ -527,7 +495,7 @@ static int __devinit tea5764_i2c_probe(struct i2c_client *client,
int ret;
PDEBUG("probe");
- radio = kmalloc(sizeof(struct tea5764_device), GFP_KERNEL);
+ radio = kzalloc(sizeof(struct tea5764_device), GFP_KERNEL);
if (!radio)
return -ENOMEM;
@@ -555,12 +523,7 @@ static int __devinit tea5764_i2c_probe(struct i2c_client *client,
i2c_set_clientdata(client, radio);
video_set_drvdata(radio->videodev, radio);
-
- ret = video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr);
- if (ret < 0) {
- PWARN("Could not register video device!");
- goto errrel;
- }
+ radio->videodev->lock = &radio->mutex;
/* initialize and power off the chip */
tea5764_i2c_read(radio);
@@ -568,6 +531,12 @@ static int __devinit tea5764_i2c_probe(struct i2c_client *client,
tea5764_mute(radio, 1);
tea5764_power_down(radio);
+ ret = video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr);
+ if (ret < 0) {
+ PWARN("Could not register video device!");
+ goto errrel;
+ }
+
PINFO("registered.");
return 0;
errrel:
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 05/15] si4713: convert to unlocked_ioctl
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (3 preceding siblings ...)
2010-11-16 21:55 ` [RFCv2 PATCH 04/15] tea5764: convert to unlocked_ioctl Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 06/15] typhoon: " Hans Verkuil
` (9 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Convert ioctl to unlocked_ioctl. Note that for this driver the locking
is done inside the sub-device.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/radio/radio-si4713.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index 6a43578..3a84c3d 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -53,7 +53,8 @@ struct radio_si4713_device {
/* radio_si4713_fops - file operations interface */
static const struct v4l2_file_operations radio_si4713_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ /* Note: locking is done at the subdev level in the i2c driver. */
+ .unlocked_ioctl = video_ioctl2,
};
/* Video4Linux Interface */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 06/15] typhoon: convert to unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (4 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 05/15] si4713: " Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 07/15] dsbr100: " Hans Verkuil
` (8 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Convert the typhoon driver from ioctl to unlocked_ioctl.
When doing this I noticed a bug where curfreq was not initialized correctly
to mutefreq (it wasn't multiplied by 16).
The initialization is now also done before the device node is created.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/radio/radio-typhoon.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/media/radio/radio-typhoon.c b/drivers/media/radio/radio-typhoon.c
index b1f6305..8dbbf08 100644
--- a/drivers/media/radio/radio-typhoon.c
+++ b/drivers/media/radio/radio-typhoon.c
@@ -317,7 +317,7 @@ static int vidioc_log_status(struct file *file, void *priv)
static const struct v4l2_file_operations typhoon_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops typhoon_ioctl_ops = {
@@ -344,18 +344,18 @@ static int __init typhoon_init(void)
strlcpy(v4l2_dev->name, "typhoon", sizeof(v4l2_dev->name));
dev->io = io;
- dev->curfreq = dev->mutefreq = mutefreq;
if (dev->io == -1) {
v4l2_err(v4l2_dev, "You must set an I/O address with io=0x316 or io=0x336\n");
return -EINVAL;
}
- if (dev->mutefreq < 87000 || dev->mutefreq > 108500) {
+ if (mutefreq < 87000 || mutefreq > 108500) {
v4l2_err(v4l2_dev, "You must set a frequency (in kHz) used when muting the card,\n");
v4l2_err(v4l2_dev, "e.g. with \"mutefreq=87500\" (87000 <= mutefreq <= 108500)\n");
return -EINVAL;
}
+ dev->curfreq = dev->mutefreq = mutefreq << 4;
mutex_init(&dev->lock);
if (!request_region(dev->io, 8, "typhoon")) {
@@ -378,17 +378,17 @@ static int __init typhoon_init(void)
dev->vdev.ioctl_ops = &typhoon_ioctl_ops;
dev->vdev.release = video_device_release_empty;
video_set_drvdata(&dev->vdev, dev);
+
+ /* mute card - prevents noisy bootups */
+ typhoon_mute(dev);
+
if (video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
v4l2_device_unregister(&dev->v4l2_dev);
release_region(dev->io, 8);
return -EINVAL;
}
v4l2_info(v4l2_dev, "port 0x%x.\n", dev->io);
- v4l2_info(v4l2_dev, "mute frequency is %lu kHz.\n", dev->mutefreq);
- dev->mutefreq <<= 4;
-
- /* mute card - prevents noisy bootups */
- typhoon_mute(dev);
+ v4l2_info(v4l2_dev, "mute frequency is %lu kHz.\n", mutefreq);
return 0;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (5 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 06/15] typhoon: " Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-18 14:40 ` David Ellingsworth
2010-11-16 21:56 ` [RFCv2 PATCH 08/15] BKL: trivial ioctl -> unlocked_ioctl video driver conversions Hans Verkuil
` (7 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Trivial conversion to unlocked_ioctl.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/radio/dsbr100.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index ed9cd7a..c3e952f 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -605,7 +605,7 @@ static void usb_dsbr100_video_device_release(struct video_device *videodev)
/* File system interface */
static const struct v4l2_file_operations usb_dsbr100_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops usb_dsbr100_ioctl_ops = {
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
2010-11-16 21:56 ` [RFCv2 PATCH 07/15] dsbr100: " Hans Verkuil
@ 2010-11-18 14:40 ` David Ellingsworth
2010-11-18 14:46 ` Hans Verkuil
0 siblings, 1 reply; 24+ messages in thread
From: David Ellingsworth @ 2010-11-18 14:40 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Arnd Bergmann
This driver has quite a few locking issues that would only be made
worse by your patch. A much better patch for this can be found here:
http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c
Regards,
David Ellingsworth
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
2010-11-18 14:40 ` David Ellingsworth
@ 2010-11-18 14:46 ` Hans Verkuil
2010-11-18 15:10 ` David Ellingsworth
0 siblings, 1 reply; 24+ messages in thread
From: Hans Verkuil @ 2010-11-18 14:46 UTC (permalink / raw)
To: David Ellingsworth; +Cc: linux-media, Arnd Bergmann
> This driver has quite a few locking issues that would only be made
> worse by your patch. A much better patch for this can be found here:
>
> http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c
Much too big for 2.6.37. I'll just drop this patch from my patch series.
Instead it will rely on the new lock in v4l2_device (BKL replacement) that
serializes all ioctls. For 2.6.38 we can convert it to core assisted
locking which is much easier.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
2010-11-18 14:46 ` Hans Verkuil
@ 2010-11-18 15:10 ` David Ellingsworth
2010-11-18 15:29 ` Hans Verkuil
0 siblings, 1 reply; 24+ messages in thread
From: David Ellingsworth @ 2010-11-18 15:10 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Arnd Bergmann
On Thu, Nov 18, 2010 at 9:46 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> This driver has quite a few locking issues that would only be made
>> worse by your patch. A much better patch for this can be found here:
>>
>> http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c
>
> Much too big for 2.6.37. I'll just drop this patch from my patch series.
> Instead it will rely on the new lock in v4l2_device (BKL replacement) that
> serializes all ioctls. For 2.6.38 we can convert it to core assisted
> locking which is much easier.
>
I don't see how this patch is really any bigger than some of the
others in your series. Sure there are a lot of deletions, but the
number of additions is quite small. There are much worse ways all the
locking issues in this driver could have been corrected. This patch
manages to do just that while reducing the overall size of the driver
at the same time.
Regards,
David Ellingsworth
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
2010-11-18 15:10 ` David Ellingsworth
@ 2010-11-18 15:29 ` Hans Verkuil
2010-11-18 15:58 ` David Ellingsworth
0 siblings, 1 reply; 24+ messages in thread
From: Hans Verkuil @ 2010-11-18 15:29 UTC (permalink / raw)
To: David Ellingsworth; +Cc: linux-media, Arnd Bergmann
> On Thu, Nov 18, 2010 at 9:46 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>>> This driver has quite a few locking issues that would only be made
>>> worse by your patch. A much better patch for this can be found here:
>>>
>>> http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c
>>
>> Much too big for 2.6.37. I'll just drop this patch from my patch series.
>> Instead it will rely on the new lock in v4l2_device (BKL replacement)
>> that
>> serializes all ioctls. For 2.6.38 we can convert it to core assisted
>> locking which is much easier.
>>
>
> I don't see how this patch is really any bigger than some of the
> others in your series. Sure there are a lot of deletions, but the
> number of additions is quite small. There are much worse ways all the
> locking issues in this driver could have been corrected. This patch
> manages to do just that while reducing the overall size of the driver
> at the same time.
Basically there are two reasons why I'm not including this in my patch
series: 1) you disagreed with my patch and 2) I disagree with your patch.
In this case the patch for 2.6.37 should either be small and trivial, or
we should postpone it to 2.6.38 and use proper core-assisted locking
(which makes the most sense for this driver in my opinion). And I really
dislike those BUG_ONs you've added, to be honest. Sorry about that...
Anyway, feel free to post your own patch for this driver. I've no problems
with that at all. It is clear that this driver takes more time to get a
proper patch for that makes everyone happy, and I really need to make the
git pull request for 2.6.37 tomorrow as we don't want to do this too late
in the 2.6.37 cycle.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 PATCH 07/15] dsbr100: convert to unlocked_ioctl.
2010-11-18 15:29 ` Hans Verkuil
@ 2010-11-18 15:58 ` David Ellingsworth
0 siblings, 0 replies; 24+ messages in thread
From: David Ellingsworth @ 2010-11-18 15:58 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Arnd Bergmann
On Thu, Nov 18, 2010 at 10:29 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> On Thu, Nov 18, 2010 at 9:46 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>
>>>> This driver has quite a few locking issues that would only be made
>>>> worse by your patch. A much better patch for this can be found here:
>>>>
>>>> http://desource.dyndns.org/~atog/gitweb?p=linux-media.git;a=commitdiff;h=9c5d8ebb602e9af46902c5f3d4d4cc80227d3f7c
>>>
>>> Much too big for 2.6.37. I'll just drop this patch from my patch series.
>>> Instead it will rely on the new lock in v4l2_device (BKL replacement)
>>> that
>>> serializes all ioctls. For 2.6.38 we can convert it to core assisted
>>> locking which is much easier.
>>>
>>
>> I don't see how this patch is really any bigger than some of the
>> others in your series. Sure there are a lot of deletions, but the
>> number of additions is quite small. There are much worse ways all the
>> locking issues in this driver could have been corrected. This patch
>> manages to do just that while reducing the overall size of the driver
>> at the same time.
>
> Basically there are two reasons why I'm not including this in my patch
> series: 1) you disagreed with my patch and 2) I disagree with your patch.
>
> In this case the patch for 2.6.37 should either be small and trivial, or
> we should postpone it to 2.6.38 and use proper core-assisted locking
> (which makes the most sense for this driver in my opinion). And I really
> dislike those BUG_ONs you've added, to be honest. Sorry about that...
The reality is that the BUG_ONs should never be hit. I added those
because I don't have the hardware needed to test my changes. So if I
missed something, the issue would be very apparent to whoever was
doing the testing. If someone would actually test this series, the
BUG_ONs could be removed. Maybe a WARN_ON would have been better, but
to me accessing the device struct without holding the lock is a bug.
The only valid case where it would be okay to do that would be from
within the probe function before the device is registered.
>
> Anyway, feel free to post your own patch for this driver. I've no problems
> with that at all. It is clear that this driver takes more time to get a
> proper patch for that makes everyone happy, and I really need to make the
> git pull request for 2.6.37 tomorrow as we don't want to do this too late
> in the 2.6.37 cycle.
>
I understand, but if you or anyone else had actually bothered to
review this patch when it was submitted back in May then it might have
made its way into the kernel by now. At the time the patch was
written, core assisted locking wasn't even implemented. Granted the
ioctl added to correct the locking might cause a cache miss, but this
is acceptable for such a simple driver. This is a good first step
towards core assisted locking.
Regards,
David Ellingsworth
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFCv2 PATCH 08/15] BKL: trivial ioctl -> unlocked_ioctl video driver conversions
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (6 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 07/15] dsbr100: " Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 09/15] sn9c102: convert to unlocked_ioctl Hans Verkuil
` (6 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
These drivers could be trivially converted to unlocked_ioctl since they
already did locking.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/arv.c | 2 +-
drivers/media/video/bw-qcam.c | 2 +-
drivers/media/video/c-qcam.c | 2 +-
drivers/media/video/meye.c | 14 +++++++-------
drivers/media/video/pms.c | 2 +-
drivers/media/video/w9966.c | 2 +-
6 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/media/video/arv.c b/drivers/media/video/arv.c
index 31e7a12..f989f28 100644
--- a/drivers/media/video/arv.c
+++ b/drivers/media/video/arv.c
@@ -712,7 +712,7 @@ static int ar_initialize(struct ar *ar)
static const struct v4l2_file_operations ar_fops = {
.owner = THIS_MODULE,
.read = ar_read,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops ar_ioctl_ops = {
diff --git a/drivers/media/video/bw-qcam.c b/drivers/media/video/bw-qcam.c
index 935e0c9..c119350 100644
--- a/drivers/media/video/bw-qcam.c
+++ b/drivers/media/video/bw-qcam.c
@@ -860,7 +860,7 @@ static ssize_t qcam_read(struct file *file, char __user *buf,
static const struct v4l2_file_operations qcam_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.read = qcam_read,
};
diff --git a/drivers/media/video/c-qcam.c b/drivers/media/video/c-qcam.c
index 6e4b196..24fc009 100644
--- a/drivers/media/video/c-qcam.c
+++ b/drivers/media/video/c-qcam.c
@@ -718,7 +718,7 @@ static ssize_t qcam_read(struct file *file, char __user *buf,
static const struct v4l2_file_operations qcam_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.read = qcam_read,
};
diff --git a/drivers/media/video/meye.c b/drivers/media/video/meye.c
index 2be23bc..48d2c24 100644
--- a/drivers/media/video/meye.c
+++ b/drivers/media/video/meye.c
@@ -1659,7 +1659,7 @@ static const struct v4l2_file_operations meye_fops = {
.open = meye_open,
.release = meye_release,
.mmap = meye_mmap,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.poll = meye_poll,
};
@@ -1831,12 +1831,6 @@ static int __devinit meye_probe(struct pci_dev *pcidev,
msleep(1);
mchip_set(MCHIP_MM_INTA, MCHIP_MM_INTA_HIC_1_MASK);
- if (video_register_device(meye.vdev, VFL_TYPE_GRABBER,
- video_nr) < 0) {
- v4l2_err(v4l2_dev, "video_register_device failed\n");
- goto outvideoreg;
- }
-
mutex_init(&meye.lock);
init_waitqueue_head(&meye.proc_list);
meye.brightness = 32 << 10;
@@ -1858,6 +1852,12 @@ static int __devinit meye_probe(struct pci_dev *pcidev,
sony_pic_camera_command(SONY_PIC_COMMAND_SETCAMERAPICTURE, 0);
sony_pic_camera_command(SONY_PIC_COMMAND_SETCAMERAAGC, 48);
+ if (video_register_device(meye.vdev, VFL_TYPE_GRABBER,
+ video_nr) < 0) {
+ v4l2_err(v4l2_dev, "video_register_device failed\n");
+ goto outvideoreg;
+ }
+
v4l2_info(v4l2_dev, "Motion Eye Camera Driver v%s.\n",
MEYE_DRIVER_VERSION);
v4l2_info(v4l2_dev, "mchip KL5A72002 rev. %d, base %lx, irq %d\n",
diff --git a/drivers/media/video/pms.c b/drivers/media/video/pms.c
index 7129b50..7551907 100644
--- a/drivers/media/video/pms.c
+++ b/drivers/media/video/pms.c
@@ -932,7 +932,7 @@ static ssize_t pms_read(struct file *file, char __user *buf,
static const struct v4l2_file_operations pms_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.read = pms_read,
};
diff --git a/drivers/media/video/w9966.c b/drivers/media/video/w9966.c
index 635420d..019ee20 100644
--- a/drivers/media/video/w9966.c
+++ b/drivers/media/video/w9966.c
@@ -815,7 +815,7 @@ out:
static const struct v4l2_file_operations w9966_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.read = w9966_v4l_read,
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 09/15] sn9c102: convert to unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (7 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 08/15] BKL: trivial ioctl -> unlocked_ioctl video driver conversions Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 10/15] et61x251_core: trivial conversion " Hans Verkuil
` (5 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Trivial conversion, this driver used a mutex already.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/sn9c102/sn9c102_core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/sn9c102/sn9c102_core.c b/drivers/media/video/sn9c102/sn9c102_core.c
index 28e19da..f49fbfb 100644
--- a/drivers/media/video/sn9c102/sn9c102_core.c
+++ b/drivers/media/video/sn9c102/sn9c102_core.c
@@ -3238,7 +3238,7 @@ static const struct v4l2_file_operations sn9c102_fops = {
.owner = THIS_MODULE,
.open = sn9c102_open,
.release = sn9c102_release,
- .ioctl = sn9c102_ioctl,
+ .unlocked_ioctl = sn9c102_ioctl,
.read = sn9c102_read,
.poll = sn9c102_poll,
.mmap = sn9c102_mmap,
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 10/15] et61x251_core: trivial conversion to unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (8 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 09/15] sn9c102: convert to unlocked_ioctl Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 11/15] cafe_ccic: replace ioctl by unlocked_ioctl Hans Verkuil
` (4 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/et61x251/et61x251_core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/et61x251/et61x251_core.c b/drivers/media/video/et61x251/et61x251_core.c
index a5cfc76..bb16409 100644
--- a/drivers/media/video/et61x251/et61x251_core.c
+++ b/drivers/media/video/et61x251/et61x251_core.c
@@ -2530,7 +2530,7 @@ static const struct v4l2_file_operations et61x251_fops = {
.owner = THIS_MODULE,
.open = et61x251_open,
.release = et61x251_release,
- .ioctl = et61x251_ioctl,
+ .unlocked_ioctl = et61x251_ioctl,
.read = et61x251_read,
.poll = et61x251_poll,
.mmap = et61x251_mmap,
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 11/15] cafe_ccic: replace ioctl by unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (9 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 10/15] et61x251_core: trivial conversion " Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 12/15] sh_vou: convert to unlocked_ioctl Hans Verkuil
` (3 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Trivial change, approved by Jonathan Corbet <corbet@lwn.net>.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/cafe_ccic.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/cafe_ccic.c b/drivers/media/video/cafe_ccic.c
index d147525..f85c933 100644
--- a/drivers/media/video/cafe_ccic.c
+++ b/drivers/media/video/cafe_ccic.c
@@ -1773,7 +1773,7 @@ static const struct v4l2_file_operations cafe_v4l_fops = {
.read = cafe_v4l_read,
.poll = cafe_v4l_poll,
.mmap = cafe_v4l_mmap,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static const struct v4l2_ioctl_ops cafe_v4l_ioctl_ops = {
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 12/15] sh_vou: convert to unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (10 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 11/15] cafe_ccic: replace ioctl by unlocked_ioctl Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 13/15] radio-timb: " Hans Verkuil
` (2 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/sh_vou.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/media/video/sh_vou.c b/drivers/media/video/sh_vou.c
index 0f49061..858b2f8 100644
--- a/drivers/media/video/sh_vou.c
+++ b/drivers/media/video/sh_vou.c
@@ -75,6 +75,7 @@ struct sh_vou_device {
int pix_idx;
struct videobuf_buffer *active;
enum sh_vou_status status;
+ struct mutex fop_lock;
};
struct sh_vou_file {
@@ -235,7 +236,7 @@ static void free_buffer(struct videobuf_queue *vq, struct videobuf_buffer *vb)
vb->state = VIDEOBUF_NEEDS_INIT;
}
-/* Locking: caller holds vq->vb_lock mutex */
+/* Locking: caller holds fop_lock mutex */
static int sh_vou_buf_setup(struct videobuf_queue *vq, unsigned int *count,
unsigned int *size)
{
@@ -257,7 +258,7 @@ static int sh_vou_buf_setup(struct videobuf_queue *vq, unsigned int *count,
return 0;
}
-/* Locking: caller holds vq->vb_lock mutex */
+/* Locking: caller holds fop_lock mutex */
static int sh_vou_buf_prepare(struct videobuf_queue *vq,
struct videobuf_buffer *vb,
enum v4l2_field field)
@@ -306,7 +307,7 @@ static int sh_vou_buf_prepare(struct videobuf_queue *vq,
return 0;
}
-/* Locking: caller holds vq->vb_lock mutex and vq->irqlock spinlock */
+/* Locking: caller holds fop_lock mutex and vq->irqlock spinlock */
static void sh_vou_buf_queue(struct videobuf_queue *vq,
struct videobuf_buffer *vb)
{
@@ -1190,7 +1191,7 @@ static int sh_vou_open(struct file *file)
V4L2_BUF_TYPE_VIDEO_OUTPUT,
V4L2_FIELD_NONE,
sizeof(struct videobuf_buffer), vdev,
- NULL);
+ &vou_dev->fop_lock);
return 0;
}
@@ -1292,7 +1293,7 @@ static const struct v4l2_file_operations sh_vou_fops = {
.owner = THIS_MODULE,
.open = sh_vou_open,
.release = sh_vou_release,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.mmap = sh_vou_mmap,
.poll = sh_vou_poll,
};
@@ -1331,6 +1332,7 @@ static int __devinit sh_vou_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&vou_dev->queue);
spin_lock_init(&vou_dev->lock);
+ mutex_init(&vou_dev->fop_lock);
atomic_set(&vou_dev->use_count, 0);
vou_dev->pdata = vou_pdata;
vou_dev->status = SH_VOU_IDLE;
@@ -1388,6 +1390,7 @@ static int __devinit sh_vou_probe(struct platform_device *pdev)
vdev->tvnorms |= V4L2_STD_PAL;
vdev->v4l2_dev = &vou_dev->v4l2_dev;
vdev->release = video_device_release;
+ vdev->lock = &vou_dev->fop_lock;
vou_dev->vdev = vdev;
video_set_drvdata(vdev, vou_dev);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 13/15] radio-timb: convert to unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (11 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 12/15] sh_vou: convert to unlocked_ioctl Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 14/15] V4L: improve the BKL replacement heuristic Hans Verkuil
2010-11-16 21:56 ` [RFCv2 PATCH 15/15] cx18: convert to unlocked_ioctl Hans Verkuil
14 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/radio/radio-timb.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/media/radio/radio-timb.c b/drivers/media/radio/radio-timb.c
index b8bb3ef..a185610 100644
--- a/drivers/media/radio/radio-timb.c
+++ b/drivers/media/radio/radio-timb.c
@@ -34,6 +34,7 @@ struct timbradio {
struct v4l2_subdev *sd_dsp;
struct video_device video_dev;
struct v4l2_device v4l2_dev;
+ struct mutex lock;
};
@@ -142,7 +143,7 @@ static const struct v4l2_ioctl_ops timbradio_ioctl_ops = {
static const struct v4l2_file_operations timbradio_fops = {
.owner = THIS_MODULE,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
};
static int __devinit timbradio_probe(struct platform_device *pdev)
@@ -164,6 +165,7 @@ static int __devinit timbradio_probe(struct platform_device *pdev)
}
tr->pdata = *pdata;
+ mutex_init(&tr->lock);
strlcpy(tr->video_dev.name, "Timberdale Radio",
sizeof(tr->video_dev.name));
@@ -171,6 +173,7 @@ static int __devinit timbradio_probe(struct platform_device *pdev)
tr->video_dev.ioctl_ops = &timbradio_ioctl_ops;
tr->video_dev.release = video_device_release_empty;
tr->video_dev.minor = -1;
+ tr->video_dev.lock = &tr->lock;
strlcpy(tr->v4l2_dev.name, DRIVER_NAME, sizeof(tr->v4l2_dev.name));
err = v4l2_device_register(NULL, &tr->v4l2_dev);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 PATCH 14/15] V4L: improve the BKL replacement heuristic
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (12 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 13/15] radio-timb: " Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-17 8:23 ` Arnd Bergmann
2010-11-16 21:56 ` [RFCv2 PATCH 15/15] cx18: convert to unlocked_ioctl Hans Verkuil
14 siblings, 1 reply; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
The BKL replacement mutex had some serious performance side-effects on
V4L drivers. It is replaced by a better heuristic that works around the
worst of the side-effects.
Read the v4l2-dev.c comments for the whole sorry story. This is a
temporary measure only until we can convert all v4l drivers to use
unlocked_ioctl.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/v4l2-dev.c | 37 ++++++++++++++++++++++++++++++++++---
drivers/media/video/v4l2-device.c | 1 +
include/media/v4l2-dev.h | 2 +-
include/media/v4l2-device.h | 2 ++
4 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 8eb0756..59ef642 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -258,11 +258,42 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
if (vdev->lock)
mutex_unlock(vdev->lock);
} else if (vdev->fops->ioctl) {
- /* TODO: convert all drivers to unlocked_ioctl */
- lock_kernel();
+ /* This code path is a replacement for the BKL. It is a major
+ * hack but it will have to do for those drivers that are not
+ * yet converted to use unlocked_ioctl.
+ *
+ * There are two options: if the driver implements struct
+ * v4l2_device, then the lock defined there is used to
+ * serialize the ioctls. Otherwise the v4l2 core lock defined
+ * below is used. This lock is really bad since it serializes
+ * completely independent devices.
+ *
+ * Both variants suffer from the same problem: if the driver
+ * sleeps, then it blocks all ioctls since the lock is still
+ * held. This is very common for VIDIOC_DQBUF since that
+ * normally waits for a frame to arrive. As a result any other
+ * ioctl calls will proceed very, very slowly since each call
+ * will have to wait for the VIDIOC_QBUF to finish. Things that
+ * should take 0.01s may now take 10-20 seconds.
+ *
+ * The workaround is to *not* take the lock for VIDIOC_DQBUF.
+ * This actually works OK for videobuf-based drivers, since
+ * videobuf will take its own internal lock.
+ */
+ static DEFINE_MUTEX(v4l2_ioctl_mutex);
+ struct mutex *m = vdev->v4l2_dev ?
+ &vdev->v4l2_dev->ioctl_lock : &v4l2_ioctl_mutex;
+
+ if (cmd != VIDIOC_DQBUF) {
+ int res = mutex_lock_interruptible(m);
+
+ if (res)
+ return res;
+ }
if (video_is_registered(vdev))
ret = vdev->fops->ioctl(filp, cmd, arg);
- unlock_kernel();
+ if (cmd != VIDIOC_DQBUF)
+ mutex_unlock(m);
} else
ret = -ENOTTY;
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 0b08f96..7fe6f92 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -35,6 +35,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
INIT_LIST_HEAD(&v4l2_dev->subdevs);
spin_lock_init(&v4l2_dev->lock);
+ mutex_init(&v4l2_dev->ioctl_lock);
v4l2_dev->dev = dev;
if (dev == NULL) {
/* If dev == NULL, then name must be filled in by the caller */
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 15802a0..59dec5a 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -39,7 +39,7 @@ struct v4l2_file_operations {
ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
unsigned int (*poll) (struct file *, struct poll_table_struct *);
- long (*ioctl) (struct file *, unsigned int, unsigned long);
+ long (*ioctl __deprecated) (struct file *, unsigned int, unsigned long);
long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
int (*open) (struct file *);
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 6648036..b16f307 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -51,6 +51,8 @@ struct v4l2_device {
unsigned int notification, void *arg);
/* The control handler. May be NULL. */
struct v4l2_ctrl_handler *ctrl_handler;
+ /* BKL replacement mutex. Temporary solution only. */
+ struct mutex ioctl_lock;
};
/* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev.
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFCv2 PATCH 14/15] V4L: improve the BKL replacement heuristic
2010-11-16 21:56 ` [RFCv2 PATCH 14/15] V4L: improve the BKL replacement heuristic Hans Verkuil
@ 2010-11-17 8:23 ` Arnd Bergmann
0 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2010-11-17 8:23 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
On Tuesday 16 November 2010 22:56:45 Hans Verkuil wrote:
> The BKL replacement mutex had some serious performance side-effects on
> V4L drivers. It is replaced by a better heuristic that works around the
> worst of the side-effects.
>
> Read the v4l2-dev.c comments for the whole sorry story. This is a
> temporary measure only until we can convert all v4l drivers to use
> unlocked_ioctl.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/video/v4l2-dev.c | 37 ++++++++++++++++++++++++++++++++++---
> drivers/media/video/v4l2-device.c | 1 +
> include/media/v4l2-dev.h | 2 +-
> include/media/v4l2-device.h | 2 ++
> 4 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 8eb0756..59ef642 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -258,11 +258,42 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> if (vdev->lock)
> mutex_unlock(vdev->lock);
> } else if (vdev->fops->ioctl) {
> - /* TODO: convert all drivers to unlocked_ioctl */
> - lock_kernel();
> + /* This code path is a replacement for the BKL. It is a major
> + * hack but it will have to do for those drivers that are not
> + * yet converted to use unlocked_ioctl.
> + *
> + * There are two options: if the driver implements struct
> + * v4l2_device, then the lock defined there is used to
> + * serialize the ioctls. Otherwise the v4l2 core lock defined
> + * below is used. This lock is really bad since it serializes
> + * completely independent devices.
> + *
> + * Both variants suffer from the same problem: if the driver
> + * sleeps, then it blocks all ioctls since the lock is still
> + * held. This is very common for VIDIOC_DQBUF since that
> + * normally waits for a frame to arrive. As a result any other
> + * ioctl calls will proceed very, very slowly since each call
> + * will have to wait for the VIDIOC_QBUF to finish. Things that
> + * should take 0.01s may now take 10-20 seconds.
> + *
> + * The workaround is to *not* take the lock for VIDIOC_DQBUF.
> + * This actually works OK for videobuf-based drivers, since
> + * videobuf will take its own internal lock.
> + */
> + static DEFINE_MUTEX(v4l2_ioctl_mutex);
> + struct mutex *m = vdev->v4l2_dev ?
> + &vdev->v4l2_dev->ioctl_lock : &v4l2_ioctl_mutex;
> +
> + if (cmd != VIDIOC_DQBUF) {
> + int res = mutex_lock_interruptible(m);
> +
> + if (res)
> + return res;
> + }
> if (video_is_registered(vdev))
> ret = vdev->fops->ioctl(filp, cmd, arg);
> - unlock_kernel();
> + if (cmd != VIDIOC_DQBUF)
> + mutex_unlock(m);
> } else
> ret = -ENOTTY;
>
> diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
> index 0b08f96..7fe6f92 100644
> --- a/drivers/media/video/v4l2-device.c
> +++ b/drivers/media/video/v4l2-device.c
> @@ -35,6 +35,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
>
> INIT_LIST_HEAD(&v4l2_dev->subdevs);
> spin_lock_init(&v4l2_dev->lock);
> + mutex_init(&v4l2_dev->ioctl_lock);
> v4l2_dev->dev = dev;
> if (dev == NULL) {
> /* If dev == NULL, then name must be filled in by the caller */
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 15802a0..59dec5a 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -39,7 +39,7 @@ struct v4l2_file_operations {
> ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
> ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
> unsigned int (*poll) (struct file *, struct poll_table_struct *);
> - long (*ioctl) (struct file *, unsigned int, unsigned long);
> + long (*ioctl __deprecated) (struct file *, unsigned int, unsigned long);
> long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
> int (*mmap) (struct file *, struct vm_area_struct *);
> int (*open) (struct file *);
> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
> index 6648036..b16f307 100644
> --- a/include/media/v4l2-device.h
> +++ b/include/media/v4l2-device.h
> @@ -51,6 +51,8 @@ struct v4l2_device {
> unsigned int notification, void *arg);
> /* The control handler. May be NULL. */
> struct v4l2_ctrl_handler *ctrl_handler;
> + /* BKL replacement mutex. Temporary solution only. */
> + struct mutex ioctl_lock;
> };
>
> /* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFCv2 PATCH 15/15] cx18: convert to unlocked_ioctl.
2010-11-16 21:55 [RFCv2 PATCH 00/15] BKL removal Hans Verkuil
` (13 preceding siblings ...)
2010-11-16 21:56 ` [RFCv2 PATCH 14/15] V4L: improve the BKL replacement heuristic Hans Verkuil
@ 2010-11-16 21:56 ` Hans Verkuil
2010-11-16 22:35 ` Andy Walls
14 siblings, 1 reply; 24+ messages in thread
From: Hans Verkuil @ 2010-11-16 21:56 UTC (permalink / raw)
To: linux-media; +Cc: Arnd Bergmann
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/cx18/cx18-streams.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/cx18/cx18-streams.c b/drivers/media/video/cx18/cx18-streams.c
index 9045f1e..ab461e2 100644
--- a/drivers/media/video/cx18/cx18-streams.c
+++ b/drivers/media/video/cx18/cx18-streams.c
@@ -41,7 +41,7 @@ static struct v4l2_file_operations cx18_v4l2_enc_fops = {
.read = cx18_v4l2_read,
.open = cx18_v4l2_open,
/* FIXME change to video_ioctl2 if serialization lock can be removed */
- .ioctl = cx18_v4l2_ioctl,
+ .unlocked_ioctl = cx18_v4l2_ioctl,
.release = cx18_v4l2_close,
.poll = cx18_v4l2_enc_poll,
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFCv2 PATCH 15/15] cx18: convert to unlocked_ioctl.
2010-11-16 21:56 ` [RFCv2 PATCH 15/15] cx18: convert to unlocked_ioctl Hans Verkuil
@ 2010-11-16 22:35 ` Andy Walls
0 siblings, 0 replies; 24+ messages in thread
From: Andy Walls @ 2010-11-16 22:35 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Devin Heitmueller, linux-media, Arnd Bergmann
On Tue, 2010-11-16 at 22:56 +0100, Hans Verkuil wrote:
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
> drivers/media/video/cx18/cx18-streams.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/cx18/cx18-streams.c b/drivers/media/video/cx18/cx18-streams.c
> index 9045f1e..ab461e2 100644
> --- a/drivers/media/video/cx18/cx18-streams.c
> +++ b/drivers/media/video/cx18/cx18-streams.c
> @@ -41,7 +41,7 @@ static struct v4l2_file_operations cx18_v4l2_enc_fops = {
> .read = cx18_v4l2_read,
> .open = cx18_v4l2_open,
> /* FIXME change to video_ioctl2 if serialization lock can be removed */
> - .ioctl = cx18_v4l2_ioctl,
> + .unlocked_ioctl = cx18_v4l2_ioctl,
> .release = cx18_v4l2_close,
> .poll = cx18_v4l2_enc_poll,
> };
Hans,
Because I haven't done my homework on the ALSA API, could you also add
snd_cx18_lock()/snd_cx18_unlock() in snd_cx18_pcm_ioctl()?
Devin,
Do you know off the top of your head if any other operations in
cx18-alsa-* may need additional locking?
I am an ALSA API callback idiot. :)
Regards,
Andy
^ permalink raw reply [flat|nested] 24+ messages in thread