Ville Syrjälä
Thu, 18 Mar 2010 08:27:01 -0700
On Thu, Mar 18, 2010 at 09:52:39AM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> 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..
Yes but I think in out case it should be fine because if we hit this: t thread 1 thread 2 | | down_read(0) | down_write(1) v down_read(1) then thread 2 will eventually do a up_write() without taking any other region rwsem, and thread 1 can then continue. The other locks we have to worry about are the fb_info mutex which should always be taken before any region lock, and mmap_sem which in mmap() is taken before the region lock. I'm hoping there are no mmap_sem users lurking in the driver that already hold the region rwsem. -- Ville Syrjälä -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html