Skip to content

Commit

Permalink
OMAPFB: simplify locking
Browse files Browse the repository at this point in the history 
Kernel lock verification code has lately detected possible circular
locking in omapfb. The exact problem is unclear, but omapfb's current
locking seems to be overly complex.

This patch simplifies the locking in the following ways:

- Remove explicit omapfb mem region locking. I couldn't figure out the
  need for this, as long as we take care to take omapfb lock.

- Get omapfb lock always, even if the operation is possibly only related
  to one fb_info. Better safe than sorry, and normally there's only one
  user for the fb so this shouldn't matter.

- Make sure fb_info lock is taken first, then omapfb lock.

With this patch the warnings about possible circular locking does not
happen anymore.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
  • Loading branch information
Tomi Valkeinen committed Dec 7, 2012
1 parent 636f4e1 commit b41deec
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 132 deletions.
68 changes: 9 additions & 59 deletions drivers/video/omap2/omapfb/omapfb-ioctl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,6 @@ static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
goto out;
}

/* Take the locks in a specific order to keep lockdep happy */
if (old_rg->id < new_rg->id) {
omapfb_get_mem_region(old_rg);
omapfb_get_mem_region(new_rg);
} else if (new_rg->id < old_rg->id) {
omapfb_get_mem_region(new_rg);
omapfb_get_mem_region(old_rg);
} else
omapfb_get_mem_region(old_rg);

if (pi->enabled && !new_rg->size) {
/*
* This plane's memory was freed, can't enable it
Expand Down Expand Up @@ -146,16 +136,6 @@ static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
goto undo;
}

/* Release the locks in a specific order to keep lockdep happy */
if (old_rg->id > new_rg->id) {
omapfb_put_mem_region(old_rg);
omapfb_put_mem_region(new_rg);
} else if (new_rg->id > old_rg->id) {
omapfb_put_mem_region(new_rg);
omapfb_put_mem_region(old_rg);
} else
omapfb_put_mem_region(old_rg);

return 0;

undo:
Expand All @@ -166,15 +146,6 @@ static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)

ovl->set_overlay_info(ovl, &old_info);
put_mem:
/* Release the locks in a specific order to keep lockdep happy */
if (old_rg->id > new_rg->id) {
omapfb_put_mem_region(old_rg);
omapfb_put_mem_region(new_rg);
} else if (new_rg->id > old_rg->id) {
omapfb_put_mem_region(new_rg);
omapfb_put_mem_region(old_rg);
} else
omapfb_put_mem_region(old_rg);
out:
dev_err(fbdev->dev, "setup_plane failed\n");

Expand Down Expand Up @@ -224,10 +195,9 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
if (display && display->driver->sync)
display->driver->sync(display);

rg = ofbi->region;
mutex_lock(&fbi->mm_lock);

down_write_nested(&rg->lock, rg->id);
atomic_inc(&rg->lock_count);
rg = ofbi->region;

if (rg->size == size && rg->type == mi->type)
goto out;
Expand Down Expand Up @@ -261,9 +231,7 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
}

out:
atomic_dec(&rg->lock_count);
up_write(&rg->lock);

mutex_unlock(&fbi->mm_lock);
return r;
}

Expand All @@ -272,14 +240,12 @@ static int omapfb_query_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
struct omapfb_info *ofbi = FB2OFB(fbi);
struct omapfb2_mem_region *rg;

rg = omapfb_get_mem_region(ofbi->region);
rg = ofbi->region;
memset(mi, 0, sizeof(*mi));

mi->size = rg->size;
mi->type = rg->type;

omapfb_put_mem_region(rg);

return 0;
}

Expand Down Expand Up @@ -318,14 +284,10 @@ int omapfb_set_update_mode(struct fb_info *fbi,
if (mode != OMAPFB_AUTO_UPDATE && mode != OMAPFB_MANUAL_UPDATE)
return -EINVAL;

omapfb_lock(fbdev);

d = get_display_data(fbdev, display);

if (d->update_mode == mode) {
omapfb_unlock(fbdev);
if (d->update_mode == mode)
return 0;
}

r = 0;

Expand All @@ -341,8 +303,6 @@ int omapfb_set_update_mode(struct fb_info *fbi,
r = -EINVAL;
}

omapfb_unlock(fbdev);

return r;
}

Expand All @@ -357,14 +317,10 @@ int omapfb_get_update_mode(struct fb_info *fbi,
if (!display)
return -EINVAL;

omapfb_lock(fbdev);

d = get_display_data(fbdev, display);

*mode = d->update_mode;

omapfb_unlock(fbdev);

return 0;
}

Expand Down Expand Up @@ -424,13 +380,10 @@ static int omapfb_set_color_key(struct fb_info *fbi,
struct omapfb_color_key *ck)
{
struct omapfb_info *ofbi = FB2OFB(fbi);
struct omapfb2_device *fbdev = ofbi->fbdev;
int r;
int i;
struct omap_overlay_manager *mgr = NULL;

omapfb_lock(fbdev);

for (i = 0; i < ofbi->num_overlays; i++) {
if (ofbi->overlays[i]->manager) {
mgr = ofbi->overlays[i]->manager;
Expand All @@ -445,22 +398,17 @@ static int omapfb_set_color_key(struct fb_info *fbi,

r = _omapfb_set_color_key(mgr, ck);
err:
omapfb_unlock(fbdev);

return r;
}

static int omapfb_get_color_key(struct fb_info *fbi,
struct omapfb_color_key *ck)
{
struct omapfb_info *ofbi = FB2OFB(fbi);
struct omapfb2_device *fbdev = ofbi->fbdev;
struct omap_overlay_manager *mgr = NULL;
int r = 0;
int i;

omapfb_lock(fbdev);

for (i = 0; i < ofbi->num_overlays; i++) {
if (ofbi->overlays[i]->manager) {
mgr = ofbi->overlays[i]->manager;
Expand All @@ -475,8 +423,6 @@ static int omapfb_get_color_key(struct fb_info *fbi,

*ck = omapfb_color_keys[mgr->id];
err:
omapfb_unlock(fbdev);

return r;
}

Expand Down Expand Up @@ -603,6 +549,8 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)

int r = 0;

omapfb_lock(fbdev);

switch (cmd) {
case OMAPFB_SYNC_GFX:
DBG("ioctl SYNC_GFX\n");
Expand Down Expand Up @@ -908,6 +856,8 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)
r = -EINVAL;
}

omapfb_unlock(fbdev);

if (r < 0)
DBG("ioctl failed: %d\n", r);

Expand Down
40 changes: 10 additions & 30 deletions drivers/video/omap2/omapfb/omapfb-main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,8 +672,6 @@ int check_fb_var(struct fb_info *fbi, struct fb_var_screeninfo *var)

DBG("check_fb_var %d\n", ofbi->id);

WARN_ON(!atomic_read(&ofbi->region->lock_count));

r = fb_mode_to_dss_mode(var, &mode);
if (r) {
DBG("cannot convert var to omap dss mode\n");
Expand Down Expand Up @@ -855,8 +853,6 @@ int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
int rotation = var->rotate;
int i;

WARN_ON(!atomic_read(&ofbi->region->lock_count));

for (i = 0; i < ofbi->num_overlays; i++) {
if (ovl != ofbi->overlays[i])
continue;
Expand Down Expand Up @@ -948,8 +944,6 @@ int omapfb_apply_changes(struct fb_info *fbi, int init)
fill_fb(fbi);
#endif

WARN_ON(!atomic_read(&ofbi->region->lock_count));

for (i = 0; i < ofbi->num_overlays; i++) {
ovl = ofbi->overlays[i];

Expand Down Expand Up @@ -1008,15 +1002,16 @@ int omapfb_apply_changes(struct fb_info *fbi, int init)
static int omapfb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
{
struct omapfb_info *ofbi = FB2OFB(fbi);
struct omapfb2_device *fbdev = ofbi->fbdev;
int r;

DBG("check_var(%d)\n", FB2OFB(fbi)->id);

omapfb_get_mem_region(ofbi->region);
omapfb_lock(fbdev);

r = check_fb_var(fbi, var);

omapfb_put_mem_region(ofbi->region);
omapfb_unlock(fbdev);

return r;
}
Expand All @@ -1025,11 +1020,12 @@ static int omapfb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
static int omapfb_set_par(struct fb_info *fbi)
{
struct omapfb_info *ofbi = FB2OFB(fbi);
struct omapfb2_device *fbdev = ofbi->fbdev;
int r;

DBG("set_par(%d)\n", FB2OFB(fbi)->id);

omapfb_get_mem_region(ofbi->region);
omapfb_lock(fbdev);

set_fb_fix(fbi);

Expand All @@ -1040,7 +1036,7 @@ static int omapfb_set_par(struct fb_info *fbi)
r = omapfb_apply_changes(fbi, 0);

out:
omapfb_put_mem_region(ofbi->region);
omapfb_unlock(fbdev);

return r;
}
Expand All @@ -1049,6 +1045,7 @@ static int omapfb_pan_display(struct fb_var_screeninfo *var,
struct fb_info *fbi)
{
struct omapfb_info *ofbi = FB2OFB(fbi);
struct omapfb2_device *fbdev = ofbi->fbdev;
struct fb_var_screeninfo new_var;
int r;

Expand All @@ -1064,11 +1061,11 @@ static int omapfb_pan_display(struct fb_var_screeninfo *var,

fbi->var = new_var;

omapfb_get_mem_region(ofbi->region);
omapfb_lock(fbdev);

r = omapfb_apply_changes(fbi, 0);

omapfb_put_mem_region(ofbi->region);
omapfb_unlock(fbdev);

return r;
}
Expand All @@ -1077,18 +1074,14 @@ static void mmap_user_open(struct vm_area_struct *vma)
{
struct omapfb2_mem_region *rg = vma->vm_private_data;

omapfb_get_mem_region(rg);
atomic_inc(&rg->map_count);
omapfb_put_mem_region(rg);
}

static void mmap_user_close(struct vm_area_struct *vma)
{
struct omapfb2_mem_region *rg = vma->vm_private_data;

omapfb_get_mem_region(rg);
atomic_dec(&rg->map_count);
omapfb_put_mem_region(rg);
}

static struct vm_operations_struct mmap_user_ops = {
Expand All @@ -1112,7 +1105,7 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
return -EINVAL;
off = vma->vm_pgoff << PAGE_SHIFT;

rg = omapfb_get_mem_region(ofbi->region);
rg = ofbi->region;

start = omapfb_get_region_paddr(ofbi);
len = fix->smem_len;
Expand Down Expand Up @@ -1140,13 +1133,9 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
/* vm_ops.open won't be called for mmap itself. */
atomic_inc(&rg->map_count);

omapfb_put_mem_region(rg);

return 0;

error:
omapfb_put_mem_region(ofbi->region);

return r;
}

Expand Down Expand Up @@ -1914,7 +1903,6 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)

ofbi->region = &fbdev->regions[i];
ofbi->region->id = i;
init_rwsem(&ofbi->region->lock);

/* assign these early, so that fb alloc can use them */
ofbi->rotation_type = def_vrfb ? OMAP_DSS_ROT_VRFB :
Expand Down Expand Up @@ -1946,12 +1934,8 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
/* setup fb_infos */
for (i = 0; i < fbdev->num_fbs; i++) {
struct fb_info *fbi = fbdev->fbs[i];
struct omapfb_info *ofbi = FB2OFB(fbi);

omapfb_get_mem_region(ofbi->region);
r = omapfb_fb_init(fbdev, fbi);
omapfb_put_mem_region(ofbi->region);

if (r) {
dev_err(fbdev->dev, "failed to setup fb_info\n");
return r;
Expand Down Expand Up @@ -1983,12 +1967,8 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)

for (i = 0; i < fbdev->num_fbs; i++) {
struct fb_info *fbi = fbdev->fbs[i];
struct omapfb_info *ofbi = FB2OFB(fbi);

omapfb_get_mem_region(ofbi->region);
r = omapfb_apply_changes(fbi, 1);
omapfb_put_mem_region(ofbi->region);

if (r) {
dev_err(fbdev->dev, "failed to change mode\n");
return r;
Expand Down
Loading

0 comments on commit b41deec

Please sign in to comment.