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

Michael Trimarchi trimarchi at gandalf.sssup.it
Tue Mar 31 17:37:36 CEST 2009


Hi
Werner Almesberger wrote:
> Michael Trimarchi wrote:
>   
>> I hope that this one is little bit better :)
>>     
>
> *Much* nicer ! :-) However, ...
>
>   
>> +    init_rwsem(&ar->arHwAvail);
>> +    up_write(&ar->arHwAvail);
>>     
>
> The rwsem is already unlocked after init_rwsem. So I'm not sure what
> the up_write does. Probably nothing good.
>   
Sorry it was removed during test, but I forget to commit.
> Did you test if it works ?
>
>   
>> +#define DECLARE_LOCKED_VERSION(_name) \
>>     
>
> Nice use of macros ! In your original patch, I hadn't even noticed
> just how many functions you had to change :)
>
> One small nit: mixed-case names aren't common in the kernel. How
> about lock_ar6000 or such ?
>   
Ok
>   
>> +    ret = _name(dev, info, p, data); \
>> +    ret = ar6000_hw_avail_unlock(ar); \
>> +    return ret; \
>>     
>
> Ignoring return values ? Tsk, tsk :-) Since it cannot fail, you
> probably don't want ar6000_hw_avail_unlock to return a status at all.
>
>   
This is wrong.
> In fact, you could trim this down to simply
>
> 	AR_SOFTC_T *ar = (AR_SOFTC_T *) netdev_priv(dev);	\
> 	int ret = -EAGAIN;					\
> 								\
> 	if (down_read_trylock(&ar->arHwAvail))			\
> 		ret = _name(dev, info, p, data);		\
> 	up_read(&ar->arHwAvail);				\
> 	return ret;						\
> }
>
> This compiles into even less code than your version (after fixing
> it) and you can see all that's happening at one glance.
>
>   
I test it and the problem disappear, ok I will fix. I see the other patches,
I'm not sure if you just reduce the window or your patch is ok, I must 
take a look
to the net part to undestand if it is correct.
> Thanks,
> - Werner
>
>   
Michael



More information about the openmoko-kernel mailing list