linux-kernel - Re: [PATCH] Cleaning up code formatting errors in net/wireless pointed out by checkpatch.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1329734722.3458.7.camel@jlt3.sipsolutions.net>
Date:	Mon, 20 Feb 2012 11:45:22 +0100
From:	Johannes Berg <johannes@...solutions.net>
To:	Stephen Hemminger <shemminger@...tta.com>
Cc:	Joe Perches <joe@...ches.com>,
	Luis Felipe Strano Moraes <lfelipe@...fusion.mobi>,
	linville@...driver.com, davem@...emloft.net,
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Cleaning up code formatting errors in net/wireless
 pointed out by checkpatch.

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/