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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
