Werner Almesberger wrote:
Michael Trimarchi wrote:
Subject: [PATCH] The ioctl wext etc, seems to be broken because they don't take
any lock
I now had a closer look at your patch. Sorry for taking so long.
You've solved a nasty problem and I think the logic of your patch is
good in general. However, there are two things that could be improved:
- you had to change a lot of returns to ret = ...; goto out;
There is an easier way: if you have a resource that's being held
throughout most of a function, you can just put it into a wrapper
function and leave the returns intact. E.g.,
static int do_foo(stuff)
{
...
if (bad_things_happen)
return -EPICFAILURE;
...
}
static int foo(stuff)
{
int ret;
if (!try_grab_whatever())
return -ETHISSUCKS;
ret = do_foo();
release_whatever();
return ret;
}
Ok,
- as a rule of thumb, if you need more than two variables to implement
a single locking/synchronization mechanism, there's usually an easier
way to do it. In this case, I think you only need one variable :-)
First of all, you should be able to get rid of the reference count
if you use a read-write lock: make the ioctls take a read lock and
the avail/unavail take a write lock. This moves the reference count
into the lock.
->ioctl_start ..... by a process, your lock is not acquire
<--- in the middle you acquire the hardware lock and don't made the
hw_unvalaible
---> release the lock at the end
-> call ioctl_wext crash
Using the hw_unvalaible you are sure that the function return. The priv data
I think that are valid until the intertal eth reference count is valid.
Michael
