On Wed, Mar 17, 2010 at 09:14:25PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> On Wed, Mar 17, 2010 at 06:34:07PM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> > Hi,
> >
> > couple of minor comments inlined.
> >
> > On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> > [...]
> > > @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
> > > struct omapfb_info *ofbi = FB2OFB(fbi);
> > > struct omapfb2_device *fbdev = ofbi->fbdev;
> > > struct omapfb2_mem_region *rg;
> > > - int r, i;
> > > + int r = 0;
> > > size_t size;
> > > + int i;
> > >
> > > if (mi->type > OMAPFB_MEMTYPE_MAX)
> > > return -EINVAL;
> > >
> > > size = PAGE_ALIGN(mi->size);
> > >
> > > - rg = &ofbi->region;
> > > + rg = ofbi->region;
> > >
> > > - for (i = 0; i < ofbi->num_overlays; i++) {
> > > - if (ofbi->overlays[i]->info.enabled)
> > > - return -EBUSY;
> > > + /* FIXME probably should be a rwsem ... */
> > > + mutex_lock(&rg->mtx);
> > > + while (rg->ref) {
> > > + mutex_unlock(&rg->mtx);
> > > + schedule();
> > > + mutex_lock(&rg->mtx);
> > > + }
> >
> > Yes, rwsem would mean no unnecessary scheduling and also make things
> > clearer.
>
> Just tried it and seems to be mostly OK. We get lockdep checking as a
> bonus. It didn't like setup_plane taking the same rwsem twice so I
> added a check to see if the old and new regions are the same and just
> lock once in that case. I thought rwsem was supposed to be OK with
> read recursion but perhaps I was mitaken, or perhaps it's just lockdep
> that's misbehaving.
Ah ok, so it's not so obvious change. Nested read locks could really lead
to a deadlock I think. A read lock will block if there is a write waiter
in the queue to avoid write starvation..
--Imre
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
