On Fri, 2012-02-17 at 11:06 -0800, Stephen Hemminger wrote:
> > I'd try to make the statement expression visually
> > distinct. Something like:
> >
> > wait_event(rdev->dev_wait,
> > ({
> > int __count;
> > mutex_lock(&rdev->devlist_mtx);
> > __count = rdev->opencount;
> > mutex_unlock(&rdev->devlist_mtx);
> > __count == 0;
> > })
> > );
> >
>
> I prefer to see this done as an inline function
>
> wait_event(rdev->dev_wait, is_foo_ready(rdev))
>
> Also, in this case wrapping a condition with a mutex really is
> meaningless because the state is longer protected out side the
> protected region; in other words the mutex here is bogus and
> provides no additional protection.
I don't really care about all the changes suggested here -- feel free to
make them. One thing I'd like to point out though is that generally the
mutex might serve a purpose even here. In this specific case, it
currently doesn't, but I still think it's safer to keep it in case
somebody modifies other code. The case where it matters is when the
modification of the "opencount" variable isn't the last thing that
happens in a locked section, but you here want or need to wait for
everything happening in that section to be done.
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
