[PATCH] ARM: OMAPFB: panel-sony-acx565akm: fix missing mutex unlocks

From: Ivaylo Dimitrov <redacted>

Commit c37dd677988ca50bc8bc60ab5ab053720583c168 fixes the unbalanced
unlock in acx565akm_enable but introduces another problem - if
acx565akm_panel_power_on exits early, the mutex is not unlocked. Fix
that by unlocking the mutex on early return. Also add mutex protection in
acx565akm_panel_power_off and remove an unused variable

Signed-off-by: Ivaylo Dimitrov <redacted>
---
 .../omap2/displays-new/panel-sony-acx565akm.c      |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
index d94f35d..a093d3a 100644
--- a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
+++ b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c

@@ -535,6 +535,7 @@ static int acx565akm_panel_power_on(struct omap_dss_device *dssdev)
 
 	r = in->ops.sdi->enable(in);
 	if (r) {
+		mutex_unlock(&ddata->mutex);
 		pr_err("%s sdi enable failed\n", __func__);
 		return r;
 	}

@@ -546,6 +547,7 @@ static int acx565akm_panel_power_on(struct omap_dss_device *dssdev)
 		gpio_set_value(ddata->reset_gpio, 1);
 
 	if (ddata->enabled) {
+		mutex_unlock(&ddata->mutex);
 		dev_dbg(&ddata->spi->dev, "panel already enabled\n");
 		return 0;
 	}

@@ -578,10 +580,14 @@ static void acx565akm_panel_power_off(struct omap_dss_device *dssdev)
 	struct panel_drv_data *ddata = to_panel_data(dssdev);
 	struct omap_dss_device *in = ddata->in;
 
+	mutex_lock(&ddata->mutex);
+
 	dev_dbg(dssdev->dev, "%s\n", __func__);
 
-	if (!ddata->enabled)
+	if (!ddata->enabled) {
+		mutex_unlock(&ddata->mutex);
 		return;
+	}
 
 	set_display_state(ddata, 0);
 	set_sleep_mode(ddata, 1);

@@ -601,11 +607,13 @@ static void acx565akm_panel_power_off(struct omap_dss_device *dssdev)
 	msleep(100);
 
 	in->ops.sdi->disable(in);
+
+	mutex_unlock(&ddata->mutex);
+
 }
 
 static int acx565akm_enable(struct omap_dss_device *dssdev)
 {
-	struct panel_drv_data *ddata = to_panel_data(dssdev);
 	int r;
 
 	dev_dbg(dssdev->dev, "%s\n", __func__);

-- 
1.5.6.1

On Mon 2013-12-30 18:17:52, Ivaylo Dimitrov wrote:

From: Ivaylo Dimitrov <redacted>

Commit c37dd677988ca50bc8bc60ab5ab053720583c168 fixes the unbalanced
unlock in acx565akm_enable but introduces another problem - if
acx565akm_panel_power_on exits early, the mutex is not unlocked. Fix
that by unlocking the mutex on early return. Also add mutex protection in
acx565akm_panel_power_off and remove an unused variable

Signed-off-by: Ivaylo Dimitrov <redacted>

Reviewed-by: Pavel Machek <redacted>
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

On 04.01.2014 14:51, Pavel Machek wrote:

On Mon 2013-12-30 18:17:52, Ivaylo Dimitrov wrote:

quoted

From: Ivaylo Dimitrov <redacted>

Commit c37dd677988ca50bc8bc60ab5ab053720583c168 fixes the unbalanced
unlock in acx565akm_enable but introduces another problem - if
acx565akm_panel_power_on exits early, the mutex is not unlocked. Fix
that by unlocking the mutex on early return. Also add mutex protection in
acx565akm_panel_power_off and remove an unused variable

Signed-off-by: Ivaylo Dimitrov <redacted>

Reviewed-by: Pavel Machek <redacted>

Hmm, I introduced a bug with that patch (recursive lock), will send a 
new version that fixes it

Regards,
Ivo

From: Ivaylo Dimitrov <redacted>

Commit c37dd677988ca50bc8bc60ab5ab053720583c168 fixes the unbalanced
unlock in acx565akm_enable but introduces another problem - if
acx565akm_panel_power_on exits early, the mutex is not unlocked. Fix
that by unlocking the mutex on early return. Also add mutex protection in
acx565akm_panel_power_off and remove an unused variable

Signed-off-by: Ivaylo Dimitrov <redacted>
---
 .../omap2/displays-new/panel-sony-acx565akm.c      |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
index d94f35d..9aef7fa 100644
--- a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
+++ b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c

@@ -535,6 +535,7 @@ static int acx565akm_panel_power_on(struct omap_dss_device *dssdev)
 
 	r = in->ops.sdi->enable(in);
 	if (r) {
+		mutex_unlock(&ddata->mutex);
 		pr_err("%s sdi enable failed\n", __func__);
 		return r;
 	}

@@ -546,6 +547,7 @@ static int acx565akm_panel_power_on(struct omap_dss_device *dssdev)
 		gpio_set_value(ddata->reset_gpio, 1);
 
 	if (ddata->enabled) {
+		mutex_unlock(&ddata->mutex);
 		dev_dbg(&ddata->spi->dev, "panel already enabled\n");
 		return 0;
 	}

@@ -578,10 +580,14 @@ static void acx565akm_panel_power_off(struct omap_dss_device *dssdev)
 	struct panel_drv_data *ddata = to_panel_data(dssdev);
 	struct omap_dss_device *in = ddata->in;
 
+	mutex_lock(&ddata->mutex);
+
 	dev_dbg(dssdev->dev, "%s\n", __func__);
 
-	if (!ddata->enabled)
+	if (!ddata->enabled) {
+		mutex_unlock(&ddata->mutex);
 		return;
+	}
 
 	set_display_state(ddata, 0);
 	set_sleep_mode(ddata, 1);

@@ -601,11 +607,13 @@ static void acx565akm_panel_power_off(struct omap_dss_device *dssdev)
 	msleep(100);
 
 	in->ops.sdi->disable(in);
+
+	mutex_unlock(&ddata->mutex);
+
 }
 
 static int acx565akm_enable(struct omap_dss_device *dssdev)
 {
-	struct panel_drv_data *ddata = to_panel_data(dssdev);
 	int r;
 
 	dev_dbg(dssdev->dev, "%s\n", __func__);

@@ -627,16 +635,12 @@ static int acx565akm_enable(struct omap_dss_device *dssdev)
 
 static void acx565akm_disable(struct omap_dss_device *dssdev)
 {
-	struct panel_drv_data *ddata = to_panel_data(dssdev);
-
 	dev_dbg(dssdev->dev, "%s\n", __func__);
 
 	if (!omapdss_device_is_enabled(dssdev))
 		return;
 
-	mutex_lock(&ddata->mutex);
 	acx565akm_panel_power_off(dssdev);
-	mutex_unlock(&ddata->mutex);
 
 	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
 }

-- 
1.5.6.1

On 2014-01-05 15:13, Ivaylo Dimitrov wrote:

From: Ivaylo Dimitrov <redacted>

Commit c37dd677988ca50bc8bc60ab5ab053720583c168 fixes the unbalanced
unlock in acx565akm_enable but introduces another problem - if
acx565akm_panel_power_on exits early, the mutex is not unlocked. Fix
that by unlocking the mutex on early return. Also add mutex protection in
acx565akm_panel_power_off and remove an unused variable

I think this is just getting more messy. How about we more or less revert the c37dd677988ca50bc8bc60ab5ab053720583c168 and fix it like this:

diff --git a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
index 714ee92dfb9f..8e97d06921ff 100644
--- a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
+++ b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c

@@ -346,28 +346,22 @@ static int acx565akm_get_actual_brightness(struct panel_drv_data *ddata)
 static int acx565akm_bl_update_status(struct backlight_device *dev)
 {
 	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
-	int r;
 	int level;
 
 	dev_dbg(&ddata->spi->dev, "%s\n", __func__);
 
-	mutex_lock(&ddata->mutex);
-
 	if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
 			dev->props.power == FB_BLANK_UNBLANK)
 		level = dev->props.brightness;
 	else
 		level = 0;
 
-	r = 0;
 	if (ddata->has_bc)
 		acx565akm_set_brightness(ddata, level);
 	else
-		r = -ENODEV;
-
-	mutex_unlock(&ddata->mutex);
+		return -ENODEV;
 
-	return r;
+	return 0;
 }
 
 static int acx565akm_bl_get_intensity(struct backlight_device *dev)

@@ -390,9 +384,33 @@ static int acx565akm_bl_get_intensity(struct backlight_device *dev)
 	return 0;
 }
 
+static int acx565akm_bl_update_status_locked(struct backlight_device *dev)
+{
+	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
+	int r;
+
+	mutex_lock(&ddata->mutex);
+	r = acx565akm_bl_update_status(dev);
+	mutex_unlock(&ddata->mutex);
+
+	return r;
+}
+
+static int acx565akm_bl_get_intensity_locked(struct backlight_device *dev)
+{
+	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
+	int r;
+
+	mutex_lock(&ddata->mutex);
+	r = acx565akm_bl_get_intensity(dev);
+	mutex_unlock(&ddata->mutex);
+
+	return r;
+}
+
 static const struct backlight_ops acx565akm_bl_ops = {
-	.get_brightness = acx565akm_bl_get_intensity,
-	.update_status  = acx565akm_bl_update_status,
+	.get_brightness = acx565akm_bl_get_intensity_locked,
+	.update_status  = acx565akm_bl_update_status_locked,
 };
 
 /*--------------------Auto Brightness control via Sysfs---------------------*/

@@ -526,8 +544,6 @@ static int acx565akm_panel_power_on(struct omap_dss_device *dssdev)
 	struct omap_dss_device *in = ddata->in;
 	int r;
 
-	mutex_lock(&ddata->mutex);
-
 	dev_dbg(&ddata->spi->dev, "%s\n", __func__);
 
 	in->ops.sdi->set_timings(in, &ddata->videomode);

@@ -568,8 +584,6 @@ static int acx565akm_panel_power_on(struct omap_dss_device *dssdev)
 	set_display_state(ddata, 1);
 	set_cabc_mode(ddata, ddata->cabc_mode);
 
-	mutex_unlock(&ddata->mutex);
-
 	return acx565akm_bl_update_status(ddata->bl_dev);
 }
 

@@ -605,6 +619,7 @@ static void acx565akm_panel_power_off(struct omap_dss_device *dssdev)
 
 static int acx565akm_enable(struct omap_dss_device *dssdev)
 {
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
 	int r;
 
 	dev_dbg(dssdev->dev, "%s\n", __func__);

@@ -615,7 +630,9 @@ static int acx565akm_enable(struct omap_dss_device *dssdev)
 	if (omapdss_device_is_enabled(dssdev))
 		return 0;
 
+	mutex_lock(&ddata->mutex);
 	r = acx565akm_panel_power_on(dssdev);
+	mutex_unlock(&ddata->mutex);
 	if (r)
 		return r;
 

On 10.01.2014 12:56, Tomi Valkeinen wrote:

On 2014-01-05 15:13, Ivaylo Dimitrov wrote:

quoted

From: Ivaylo Dimitrov <redacted>

Commit c37dd677988ca50bc8bc60ab5ab053720583c168 fixes the unbalanced
unlock in acx565akm_enable but introduces another problem - if
acx565akm_panel_power_on exits early, the mutex is not unlocked. Fix
that by unlocking the mutex on early return. Also add mutex protection in
acx565akm_panel_power_off and remove an unused variable

I think this is just getting more messy. How about we more or less revert the c37dd677988ca50bc8bc60ab5ab053720583c168 and fix it like this:

I am fine with whatever patch you may come with, as long as it fixes the 
issue.

The patch does not apply cleanly on top of rc7, however I applied it by 
hand. So far it seems it fixes the issue brought by 
c37dd677988ca50bc8bc60ab5ab053720583c168, though I didn't test if 
mutex_lock/mutex_unlock are complementary in every code path (at least 
not explicitly, I guess maemo is doing it for us anyway :) ).

So, shall I send a patch incorporating your code changes, or you will do it?

Regards,
Ivo

On 2014-01-11 11:39, Ivaylo Dimitrov wrote:

The patch does not apply cleanly on top of rc7, however I applied it by
hand. So far it seems it fixes the issue brought by
c37dd677988ca50bc8bc60ab5ab053720583c168, though I didn't test if
mutex_lock/mutex_unlock are complementary in every code path (at least
not explicitly, I guess maemo is doing it for us anyway :) ).

Ok, thanks.

So, shall I send a patch incorporating your code changes, or you will do
it?

I can handle it.

 Tomi

Hi Tomi,

On 13.01.2014 12:20, Tomi Valkeinen wrote:

On 2014-01-11 11:39, Ivaylo Dimitrov wrote:

quoted

The patch does not apply cleanly on top of rc7, however I applied it by
hand. So far it seems it fixes the issue brought by
c37dd677988ca50bc8bc60ab5ab053720583c168, though I didn't test if
mutex_lock/mutex_unlock are complementary in every code path (at least
not explicitly, I guess maemo is doing it for us anyway :) ).

Ok, thanks.

quoted

So, shall I send a patch incorporating your code changes, or you will do
it?

I can handle it.

  Tomi

I still don't see those fixes in mainline, shall I send a patch?

Ivo

On 25/12/15 15:29, Ivaylo Dimitrov wrote:

Hi Tomi,

On 13.01.2014 12:20, Tomi Valkeinen wrote:

quoted

On 2014-01-11 11:39, Ivaylo Dimitrov wrote:

quoted

The patch does not apply cleanly on top of rc7, however I applied it by
hand. So far it seems it fixes the issue brought by
c37dd677988ca50bc8bc60ab5ab053720583c168, though I didn't test if
mutex_lock/mutex_unlock are complementary in every code path (at least
not explicitly, I guess maemo is doing it for us anyway :) ).

Ok, thanks.

quoted

So, shall I send a patch incorporating your code changes, or you will do
it?

I can handle it.

  Tomi

I still don't see those fixes in mainline, shall I send a patch?

Oh, I'm sorry, I must have forgotten about that. Please, send a new patch.

 Tomi

On 29.12.2015 09:46, Tomi Valkeinen wrote:

Oh, I'm sorry, I must have forgotten about that. Please, send a new patch.

  Tomi

Actually it is me to be sorry for making noise, I've missed 
0eb0dafb674cd6bfac2e3204b2f8b907e26b1138 with all those patches moving 
files around.

Ivo