On Thu, Feb 17, 2011 at 07:17:41PM +0100, Marek Vasut wrote:

Why are you getting rid of the atomic operations ?

Because they're idiotic.  Just because something is called "atomic"
doesn't make it so, and this is one instance where it's absolutely
useless.

The open and release functions are called with a mutex held.  Only
_one_ thread can be inside these at any one time.  So what use does
additionally doing an atomic operation within an already thread-safe
environment gain you?

Besides, "if (ofb->usage++ == 0)" looks suspicious, especially if you later 
declare it as uint32_t.

No.  You're not understanding the code.  This is equivalent to:

	usage = ofb->usage;
	ofb->usage = usage + 1;
	if (usage == 0)

And if you write it like that, then it is obvious.  It's your understanding
of what a post-increment looks like which is suspicious here.

quoted

@@ -733,12 +739,24 @@ static int overlayfb_release(struct fb_info *info,

int user) {
 	struct pxafb_layer *ofb = (struct pxafb_layer*) info;

DTTO, why no atomic?

Because this is already a thread-safe code region.

quoted

 	ofb->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);

@@ -891,7 +910,7 @@ static void __devinit init_pxafb_overlay(struct

pxafb_info *fbi,

 	ofb->id = id;
 	ofb->ops = &ofb_ops[id];

DTTO

An initializing store by which a machine can write the entire contents in
one instruction _is_ by its very nature atomic.

atomic_t is one of the most over(ab)used types because people just don't
think about the code they're writing. ;(