login
Search
For most drivers the generic ioctl handler does the work and we update it
and it becomes the unlocked_ioctl method. Older drivers use the usercopy
method so we make it do the work. Finally there are a few special cases.
Signed-off-by: Alan Cox <alan@redhat.com>
diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index 2ca3e9c..964fc31 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -3349,7 +3349,7 @@ static const struct file_operations bttv_fops =
.owner = THIS_MODULE,
.open = bttv_open,
.release = bttv_release,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.compat_ioctl = v4l_compat_ioctl32,
.llseek = no_llseek,
.read = bttv_read,
@@ -3630,7 +3630,7 @@ static const struct file_operations radio_fops =
.read = radio_read,
.release = radio_release,
.compat_ioctl = v4l_compat_ioctl32,
- .ioctl = video_ioctl2,
+ .unlocked_ioctl = video_ioctl2,
.llseek = no_llseek,
.poll = radio_poll,
};
diff --git a/drivers/media/video/bw-qcam.c b/drivers/media/video/bw-qcam.c
index b364ada..e953bc9 100644
--- a/drivers/media/video/bw-qcam.c
+++ b/drivers/media/video/bw-qcam.c
@@ -863,10 +863,9 @@ static int qcam_do_ioctl(struct inode *inode, struct file *file,
return 0;
}
-static int qcam_ioctl(struct inode *inode, struct file *file,
- unsigned int cmd, unsigned long arg)
+static long qcam_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- return video_usercopy(inode, file, cmd, arg, qcam_do_ioctl);
+ return video_usercopy(file, cmd, arg, qcam_do_ioctl);
}
static ssize_t qcam_read(struct file *file, char __user *buf,
@@ -897,7 +896,7 @@ static const struct file_operations qcam_fops = {
.owner = THIS_MODULE,
.open = video_exclusive_open,
.release = video_exclusive_release,
- .ioctl = qcam_ioctl,
+ .unlocked_ioctl = qcam_ioctl,
#ifdef CONFIG_COMPAT
...I'd like to start planning out the changes to eliminate the BKL from cx18. Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for these drivers' ioctls? I'm assuming it's not the mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff. Thanks. --
To the best of my knowledge there is no need for a BKL in ivtv or cx18. It was just laziness on my part that I hadn't switched to unlocked_ioctl yet. If you know of a reason why it should be kept for now, then I'd like to know, otherwise the BKL can be removed altogether for ivtv/cx18. I suspect you just pushed the lock down into the driver and in that case you can just remove it for ivtv/cx18. Regards, --
Hmm, of course you just pushed down the lock, the subject said so. Sorry, it's early morning here and I'm apparently not yet fully awake :-) Regards, Hans --
On Fri, 23 May 2008 08:16:05 +0200 I suspect that this is true with almost all drivers. Yet, we may need to enable BKL or add other locks on a few places. For example, the old tuner i2c probing method relies on a global and unique static var to identify radio and tv tuners. I can foresee a race condition there, if you have two boards probing tuners at the same time. Of course, we need to do several tests, since probably there are some issues that we cannot foresee. Cheers, Mauro --
As it stood previous for historical reasons the kernel called the driver ioctl method already holding the big kernel lock. That lock effectively serialized a lot of ioctl processing and also serializes against module loading and registration/open for the most part. If all the resources you are working on within the ioctl handler are driver owned as is likely with a video capture driver, and you have sufficient locking of your own you can drop the lock. video_usercopy currently also uses the BKL so you might want to copy a version to video_usercopy_unlocked() without that. A small number of other things still depend on the BKL but probably don't affect a capture driver: Setting file flags and using the deprecated sleep_on and interruptible_sleep_on. Other things like memory allocators wait_event, wake_up and the like all do their own locking internally and that is a model we want to end up at - whether everything has "proper" locking. You may also find a driver depends on other things (eg video on i2c and you can't yet prove the i2c (or don't know) is locked entirely privately in which cases I've ended up leaving the drivers I worked on with lock_kernel(); ret = subsys_some_foo(bar); unlock_kernel(); which makes it clear what the remaining problem points are so they can be revisited. For a lot of ioctl conversions the big issue is "two ioctls at once in parallel" which can happen even on a single opener device. The other tends be stuff like "ioctl versus unplug" although for PCI and many other subsystems the resources are ref-counted so a pci_dev won't vanish on you without warning. Alan --
On Fri, 23 May 2008 05:09:19 -0400 In the specific case of ivtv and cx18, I think that the better would be to convert it first to video_ioctl2. Then, remove the BKL, with a video_ioctl2_unlocked version. Douglas already did an experimental patch converting ivtv to video_ioctl2 and sent to Hans. It needs testing, since he doesn't have any ivtv board. It should be trivial to port this to cx18, since both drivers have similar structures. Douglas, Could you send this patch to the ML for people to review and for Andy to port it to cx18? Cheers, Mauro --
Unless there is an objection, I would prefer to take Douglas' patch and merge it into the v4l-dvb ivtv driver myself. There were several things in the patch I didn't like, but I need to 'work' with it a bit to see how/if it can be done better. I can work on it tonight and tomorrow. Hopefully it is finished by then. I can move the BKL down at the same time for ivtv. It is unlikely that I will have time to do cx18 as well as I'm abroad from Wednesday until Monday, but I think Andy can do that easily based on the ivtv changes. Regards, Hans --
I didn't analyze his patch in detail. However, it is a starting point This strategy seems OK to me. Cheers, Mauro --
Weird, that was always intended to be that way, I'm not sure what happened. In any case, consider this part to be Acked-by: Jonathan Corbet <corbet@lwn.net> This forces the BKL on all V4L2 drivers whether or not they need it. Given that this code can, in fact, be a path where latency matters (consider VIDIOC_DQBUF), I'm not sure that's the best way to do it. On the other hand, the next level of BKL pushdown would be painful as all hell, given the massive number of callbacks in the V4L2 API. So I'm thinking it might be justified to create a video_ioctl2_locked() for V4L2 drivers which are not yet known to be safe in the absence of the BKL. The amount of extra code would be quite small, and it would let safe drivers operate BKL-free. jon --
The problem is that currently they are almost all unsafe - I did a quick survey as part of the changes. Pushing it down to the video2_ioctl is a starting point, but the v4l layer is going to need a lot of love and its own gradual migration. Right now we've gone from BKL buried in fs to BKL at top of v4l layer which is indeed only a starting point. I'd assumed the same as you are think a new video_ioctl2 and switching drivers one by one (I was assuming adding video2_ioctl_unlocked()) Alan --
It sound like we're thinking the same way, yes. Except I'd pick the long name for the one we want to get rid of :) BTW, drop me a pointer if you'd like this series pulled into the ever-growing bkl-removal tree. jon --
Its a single big patch right now and I need to split it up further and tidy it, plus see what the maintainers take and think. Once I've done that I'd be in favour of actually patching ->ioctl out of the bkl-removal tree and letting people fix the other architectures by neccessity ;) Alan --
Here is my Ack for the ivtv/cx18 parts of this patch: Acked-by: Hans Verkuil <hverkuil@xs4all.nl> Regards, Hans --
The pvrusb2 driver already should not require the BKL. Though since this driver (like every V4L driver) is using video_usercopy() then it's obviously going along for the ride here with respect to the overall issue here in pushing the lock down into video_usercopy(). In the mean time, for the pvrusb2 portion of this patch... Acked-By: Mike Isely <isely@pobox.com> -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 --
On Thu, 22 May 2008 22:37:00 +0100
I liked the patch also.
Still, it didn't apply cleanly on my -git tree, probably due to some fixes at
cx18 and ivtv (see the attached log). Also, IMO, it would be better if you
split drivers/net/tun.c into a different changeset.
I think you should add a video_ioctl2_unlocked() function, without BKL (and keeping
BKL explicit locks at video_ioctl2). This would help on migrating the drivers
to the unlocked version, since I suspect that most drivers already have enough
locks for the removal of BKL.
I would just implement video_ioctl2 as:
video_ioctl2_locked (...)
{
lock_kernel();
video_ioctl2_unlocked();
unlock_kernel();
}
The next step can be to add the obvious locks inside video_ioctl2_unlocked(). Like, for
example, locking the VIDIOC_S calls, if someone is calling the corresponding
VIDIOC_G or VIDIOC_TRY ones.
Cheers,
Mauro.
---
patching file drivers/media/video/bt8xx/bttv-driver.c
patching file drivers/media/video/bw-qcam.c
patching file drivers/media/video/c-qcam.c
patching file drivers/media/video/cafe_ccic.c
patching file drivers/media/video/cpia.c
patching file drivers/media/video/cpia2/cpia2_v4l.c
patching file drivers/media/video/cx18/cx18-ioctl.c
patching file drivers/media/video/cx18/cx18-ioctl.h
patching file drivers/media/video/cx18/cx18-streams.c
Hunk #1 FAILED at 39.
1 out of 1 hunk FAILED -- saving rejects to file drivers/media/video/cx18/cx18-streams.c.rej
patching file drivers/media/video/cx23885/cx23885-417.c
patching file drivers/media/video/cx23885/cx23885-video.c
patching file drivers/media/video/cx88/cx88-blackbird.c
patching file drivers/media/video/cx88/cx88-video.c
patching file drivers/media/video/dabusb.c
patching file drivers/media/video/em28xx/em28xx-video.c
patching file drivers/media/video/et61x251/et61x251_core.c
patching file drivers/media/video/ivtv/ivtv-ioctl.c
patching file drivers/media/video/ivtv/ivtv-ioctl.h
Hunk #1 FAILED at 24.
1 out of 1 hunk FAILED -- saving rejects ...Concentrate on the dats structures not the code - its one of those oft quoted and very true rules - "lock data not code" I'll tidy these up later in the week as I get time and merge them against a current linux-next tree in bits with the rework done. Alan --
On Mon, 26 May 2008 16:23:17 -0400 True. I'm thinking on locking the data that is being GET/SET by VIDIOC_foo ioctls. I can see a few strategies for removing KBL. The hardest and optimal scenario is to look inside all drivers, fix the locks (and pray for a further patch to not break them). I'm afraid that this is a long term strategy. There are also other strategies that will also produce non-optimal results, but may avoid the usage of BKL. For example, a very simple scenario would simply replace BKL by one mutex. This way, just one ioctl could be handled at the same time. This is something dumb, but will free the machine to do other tasks while an ioctl is being executed. I bet this would work with all (or almost all) drivers. I don't see any big drawback of this scenario, from the userspace POV. Smarter scenarios can be designed if you use different mutexes for different groups of data, at video_ioctl2 level. Of course, at video_ioctl2 level, you don't see the real data. However, you'll see data blocks. For example, if you're calling a VIDIOC_S_CROP, you're certainly changing whatever data structures you have that changes crop window. A concurrent call to VIDIOC_G_CROP needs to be blocked, otherwise the get will return a data that is being changing. IMO, such trivial dependencies can be safely handled at vidioc_ioctl2. We may try to group VIDIOC functions into some mutexes to avoid bad usages. For example, one mutex for ioctl's that touches on video buffers. This will lock on calls for iocls like VIDIOC_DBUF, VIDIOC_REQBUF, etc. We take some care with ioctls that would affect all groups of data, like VIDIOC_S_STD. This ioctl will likely change several data structures that affects other get operations, like changing resolution. It may also need to stop and/or return -EBUSY, if video buffers are mmapped. Such scenarios wouldn't avoid the need of implementing a few locks at the Seems fine to me ;) Cheers, Mauro --
On Mon, 26 May 2008 18:01:54 -0400 Hmm... it maybe an interesting interim solution to create such function, and moving the drivers to it. What if we create 3 functions: video_ioctl2_bkl() video_ioctl2_serialized() video_ioctl2_unlocked() The first patch will point .ioctl_unlock to video_ioctl2_bkl. A next step would be to move the drivers to use the serialized one. I suspect that this will work properly on all devices that are using video_ioctl2, if the videobuf locks are now 100% ok. So, it is just a matter of doing some stress tests. We may start with vivi, since we have a complete domain on what this driver is doing (e.g. no hardware surprises). After having all those drivers using the _serialized() one, we can remove the bkl. Then, we can focus on properly fixing the locks inside the drivers, and moving one by one to video_ioctl2_unlocked. IMO, we need to create a multi-thread stress userspace tool for checking the locks at the ioctls. There are a few testing utils at mercurial tree, under v4l2-apps/test. This can be a starting point for this tool. Also, Brandon improved one of those tools to work with multithread. What do you think? Cheers, Mauro --
On Tue, 27 May 2008 10:10:39 -0300 So we're replacing the big kernel lock with the big v4l2 lock. That might help the situation, but you'd need to be sure to serialize against other calls (open(), for example) which are also currently done I don't think that stress tools are the way to eliminate the BKL. You'll never find all the problems that way. There's really no way to avoid the task of actually *looking* at each driver and ensuring that it has its act together with regard to locking. jon --
On Tue, 27 May 2008 09:41:44 -0600 True, but on a quick analysis, I suspect that this is already somewhat broken on some drivers. Since the other methods don't explicitly call BKL (and, AFAIK, kernel open handler don't call it neither), if a program 1 is opening a device and initializing some data, and a program 2 starts doing ioctl, interrupting program 1 execution in the middle of a data initialization procedure, you may have a race condition, since some devices initialize some device global data during open [1]. [1] For example: cx88 and saa7134 will change some data structures if you open a device via /dev/radio or via /dev/video. So, if program 1 opens as radio and program 2 opens as video, you may have a race condition. A way to fix this is to initialize such structs only by the first program that is opening the device, True. Yet, just looking at the code may not be enough, since people make mistakes. The recent changes at videobuf lock showed this. The lock fix patches caused several new locking issues. It is safer to have a tool to test and stress the driver before going to production. Cheers, Mauro --
On Tue, 27 May 2008 13:31:00 -0300 In fact, 2.6.26 and prior kernels *do* acquire the BKL on open (for char devices) - that's the behavior that the bkl-removal tree is there to do away with. So, for example, I've pushed that acquisition down into video_open() instead. So, for now, open() is serialized against ioctl() in video drivers. As soon as you take the BKL out of ioctl(), though, that won't happen, unless the mutex you use is also acquired in the open path. jon --
On Tue, 27 May 2008 10:37:55 -0600 Ok. A few drivers seem to be almost read to work without BKL. For example, em28xx has already a lock at the operations that change values at "dev" struct, including open() method. However, since the lock is not called at get operations, it needs to be fixed. I would also change it from mutex to a read/write semaphore, since two (or more) get operations can safely happen in parallel. Cheers, Mauro --
Hello Mauro, On Tue, May 27, 2008 at 2:59 PM, Mauro Carvalho Chehab Please bear in mind that we have not worked out the locking semantics for hybrid tuner devices, and it's entirely possible that the get() routines will need to switch the tuner mode, which would eliminate any benefits of converting to a read/write semaphore. I'm not sure yet exactly how that's going to work, but it's something that might prompt you to defer converting it from a mutex until we have that worked out. -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller --
Hi Devin,
On Tue, 27 May 2008 15:26:27 -0400
Arjan pointed some good reasons about why we shouldn't use r/w semaphores. So,
The hybrid device mode lock is somewhat complex. The simplest solution would be
to block an open() call, if the device is already used by a different mode.
This will minimize things like firmware reload, on xc3028 devices, where you
have different firmwares for analog and digital modes.
Also, some USB devices (like HVR-900/HVR-950) switches off analog audio and tv
demod chips when in digital mode (the reverse is also true - e.g. digital demod
is switched off at analog mode).
So, if you are in digital mode, on HVR-900, and changes to analog, you'll need
to re-initialize tvp5150/msp3400. The current code for those devices handles
this at open(). If we let this to happen later, we'll need to re-send the video
and audio parameters to all I2C connected devices, when switching mode.
On the other hand, some userspace apps, like mythtv, opens both analog and
digital API's. I'm not sure if it does this at the same time, but, if so, a
lock at open() will cause a regression there (someone told me that this is the
case - I didn't test it here yet).
One possible solution of providing a proper code to change mode, and not
blocking open() would be to write something like this:
static int check_and_switch_mode(void *priv, int digital)
{
struct dev_foo *dev = priv;
mutex_lock(dev->lock);
if (digital)
return change_to_digital(dev);
else
return change_to_analog(dev);
mutex_unlock(dev->lock);
}
Since this should be called for every valid V4L2 and DVB ioctl, the better
place for it would be to add this as a new function callback, at video_ioctl2.
Something like [1]:
--- a/linux/drivers/media/video/videodev.c Tue May 27 16:02:56 2008 -0300
+++ b/linux/drivers/media/video/videodev.c Tue May 27 17:34:04 2008 -0300
@@ -821,6 +821,10 @@
v4l_print_ioctl(vfd->name, cmd);
printk("\n");
}
+
+ if (_IOC_TYPE(cmd)=='v') || ...Hello Mauro, On Tue, May 27, 2008 at 5:00 PM, Mauro Carvalho Chehab These are all good points (and I had come to some of the same conclusions). I was just trying to say that the fact that we haven't worked through these details is a good reason to not attempt to optimize the locking routines in the em28xx driver at this point. But it seems like Arjan's comments make that a moot argument anyway. Thanks, -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller --
MythTV's mythbackend can open both sides of the card at the same time and the cx18 driver supports it. On my HVR-1600, MythTV may have the digital side of the card open pulling EPG data off of the ATSC broadcasts, when I open up the MythTV frontend and start watching live TV on the analog side of the card. MythTV also supports Picture-in-Picture using both the analog and digital parts of the --
Hello Andy, In this case, what you see as a 'feature' in MythTV is actually a problem in our case. While the HVR-1600 can support this scenario, the HVR-950 can only use one or the other (the em28xx chip uses GPIOs to enable the demodulator and presumably you should never have both demodulators enabled at the same time). Because of this we need a lock. If MythTV only opened one device or the other at a time, we could put the lock on the open() call, but since MythTV opens both simultaneously even though it may only be using one, we would need a much more granular lock. Certainly I'm not blaming MythTV for this behavior, but it will make the locking much more complicated in some hybrid devices. -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller --
I don't think a lock would be good for MythTV or any other app that open()s multiple nodes at once. How can an app know that it's dead-locking or barring itself via the kernel driver? Maybe return an EBUSY or E-something else for these cases when Myth tries to open() the second device node, when there's an underlying factor that requires things to be mutually exclusive. Allowing things like read() to allow hardware mode switching between analog and digital seems like it could result in really weird behaviors at the application. I'll cite a precedent: ivtv returns EBUSY on open() when there's a conflict with it's various analog devices nodes that depend on the same underlying hardware: MPG, YUV, FM Radio, etc. I note the man page for open() doesn't list EBUSY as a valid errno. However, the V4L2 API Spec does list EBUSY as a valid errno for V4L2 I like to blame MythTV for a lot of things. ;) But in this case I can't. The driver probably shouldn't hold a lock and suspend an open() indefinitely (IMO). It should say the device is BUSY as that is the truth: an underlying hardware device or resource is busy. Regards, Andy --
Pardon me for not being clear. I wasn't suggesting having a mutex for on the open call itself. The mutex we have in the em28xx driver is only in place when we are *switching* between modes. The thinking was to have open return EBUSY if the device is already in use in the other mode, but we weren't sure if that would cause problems with MythTV (since the open call would fail) If MythTV can gracefully handle that scenario, then that would be the ideal solution from a driver Yeah, this was just me not being clear. -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller --
The posix spec doesn't limit the errors returnable this way - it says if the error is one of the ones listed it must return the error code stated so Agreed - especially if it is opened with O_NDELAY --
Actually, it depends on how the card was defined. There are cards that only utilize the analog part of the cx23418, cards that can do both digital and analog at the same time and cards that can do either digital or analog. The cx18 driver supports the first two cases but not yet the last. Regards, Hans --
On Tue, 27 May 2008 15:59:42 -0300 \ please don't use rw/sems just because there MIGHT be parallel. THey're more expensive than mutexes by quite a bit and you get a lot less checking from lockdep. They make sense for very specific, very read biased, contended cases. But please don't use them "just because"... --
On Tue, 27 May 2008 12:50:41 -0700 Good point. The nature of get operations on V4L are not worthy enough to justify the loss of lockdep checking. Cheers, Mauro --
Stress testing is rarely useful here - things like ioctl races you find by thinking evil thoughts. Stress tests that fail obviously help find some stuff but its error cases and hotplug corner cases that are usually the most foul --