I'm doing some driver cleanup on the ixj telephony driver, mostly copy/
paste and common code refactoring. I'm also looking at trying to get
rid of the BKL from the 2 places its used, so I was wondering if we
have a list of kernel calls that still require the BKL to be held, and
if we don't have a list would it be worthwhile to put one into
Documentation, I'd be willing to maintain the list as we removed the
BKL from the last core kernel places.
-Richard
Richard Holden <[email protected]> writes:
> I'm doing some driver cleanup on the ixj telephony driver, mostly
> copy/ paste and common code refactoring. I'm also looking at trying to
> get rid of the BKL from the 2 places its used, so I was wondering if
> we have a list of kernel calls that still require the BKL to be held,
> and if we don't have a list would it be worthwhile to put one into
> Documentation, I'd be willing to maintain the list as we removed the
> BKL from the last core kernel places.
The standard answer is to check the callers.
But in general there are very little (in fact I cannot think of any)
general kernel utility functions left that need the BKL and if they
then usually take it on their own. So you should be ok
assuming that general kernel code (e.g. code living in kernel/*
or lib/*) doesn't need it.
If you rely on other driver frameworks that might be different,
but a good rule of thumb is to just check that directly if it
uses lock_kernel at all.
The reason that a lot of driver code still uses BKL implicitely
(e.g. in ioctl code) is more because it always needs someone
to audit.
-Andi
--
[email protected]
On Mon, 6 Oct 2008 22:24:12 -0600
Richard Holden <[email protected]> wrote:
> I'm doing some driver cleanup on the ixj telephony driver, mostly copy/
> paste and common code refactoring. I'm also looking at trying to get
> rid of the BKL from the 2 places its used, so I was wondering if we
> have a list of kernel calls that still require the BKL to be held, and
> if we don't have a list would it be worthwhile to put one into
> Documentation, I'd be willing to maintain the list as we removed the
> BKL from the last core kernel places.
Its never so simple - a lot of the functions it depends what data you are
using or passing to them whether they are BKL safe.
Which bits use the BKL and what do you need to know is safe ?
On Oct 7, 2008, at 10:35 AM, Alan Cox wrote:
>
> Its never so simple - a lot of the functions it depends what data
> you are
> using or passing to them whether they are BKL safe.
>
> Which bits use the BKL and what do you need to know is safe ?
Actually, it looks like I was looking at 2 different places, the
simple removal is in phonedev.c that you authored, as far as I can
tell the only thing the BKL is protecting is a call to request_module,
it may be some nesting rules that I don't understand but I think if it
is still needed there we could push the lock_kernel call down to the
site of the request_module call. See patch below.
Thanks,
Richard Holden
Signed-off-by: Richard Holden <[email protected]>
diff --git a/drivers/telephony/phonedev.c b/drivers/telephony/phonedev.c
index 4d74ba3..fd1424c 100644
--- a/drivers/telephony/phonedev.c
+++ b/drivers/telephony/phonedev.c
@@ -54,14 +54,16 @@ static int phone_open(struct inode *inode, struct
file *file)
if (minor >= PHONE_NUM_DEVICES)
return -ENODEV;
- lock_kernel();
+
mutex_lock(&phone_lock);
p = phone_device[minor];
if (p)
new_fops = fops_get(p->f_op);
if (!new_fops) {
mutex_unlock(&phone_lock);
+ lock_kernel();
request_module("char-major-%d-%d", PHONE_MAJOR, minor);
+ unlock_kernel();
mutex_lock(&phone_lock);
p = phone_device[minor];
if (p == NULL || (new_fops = fops_get(p->f_op)) == NULL)
@@ -81,7 +83,6 @@ static int phone_open(struct inode *inode, struct
file *file)
fops_put(old_fops);
end:
mutex_unlock(&phone_lock);
- unlock_kernel();
return err;
}
> - lock_kernel();
> +
> mutex_lock(&phone_lock);
> p = phone_device[minor];
> if (p)
> new_fops = fops_get(p->f_op);
> if (!new_fops) {
> mutex_unlock(&phone_lock);
> + lock_kernel();
> request_module("char-major-%d-%d", PHONE_MAJOR, minor);
> + unlock_kernel();
> mutex_lock(&phone_lock);
> p = phone_device[minor];
> if (p == NULL || (new_fops = fops_get(p->f_op)) == NULL)
> @@ -81,7 +83,6 @@ static int phone_open(struct inode *inode, struct
> file *file)
> fops_put(old_fops);
> end:
> mutex_unlock(&phone_lock);
> - unlock_kernel();
> return err;
> }
Looks like the BKL can go entirely there providing phone_device[] is
always rechecked back in the phone mutex, request_module is happy nowdays
without the BKL