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.
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 ?
> + 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.
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.
Thanks,
- Werner
