[RFC AR6000 patch V2] Set hardware unavailable during wext_ioctl when the suspend or rfkill is send

Michael Trimarchi trimarchi at gandalf.sssup.it
Tue Mar 31 08:07:40 CEST 2009


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




More information about the openmoko-kernel mailing list