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.
> > @@ -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.
> > 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. ;(
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
