Re: drm: exynos: mixer: fix using usleep() in atomic context

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3017 bytes --]

On Mon, Dec 01, 2014 at 05:16:17PM +0100, Tobias Jakobi wrote:
> On 2014-12-01 16:54, Thierry Reding wrote:
> >On Sun, Nov 30, 2014 at 01:35:25AM +0100, tjakobi@math.uni-bielefeld.de
> >wrote:
> >>From: Tomasz Stanislawski <t.stanislaws@samsung.com>
> >>
> >>This patch fixes calling usleep_range() after taking reg_slock
> >>using spin_lock_irqsave(). The mdelay() is used instead.
> >>Waiting in atomic context is not the best idea in general.
> >>Hopefully, waiting occurs only when Video Processor fails
> >>to reset correctly.
> >>
> >>Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> >>---
> >> drivers/gpu/drm/exynos/exynos_mixer.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
> >>b/drivers/gpu/drm/exynos/exynos_mixer.c
> >>index a41c84e..cc7cccc 100644
> >>--- a/drivers/gpu/drm/exynos/exynos_mixer.c
> >>+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> >>@@ -632,7 +632,7 @@ static void vp_win_reset(struct mixer_context *ctx)
> >> 		/* waiting until VP_SRESET_PROCESSING is 0 */
> >> 		if (~vp_reg_read(res, VP_SRESET) & VP_SRESET_PROCESSING)
> >> 			break;
> >>-		usleep_range(10000, 12000);
> >>+		mdelay(10);
> >> 	}
> >> 	WARN(tries == 0, "failed to reset Video Processor\n");
> >> }
> >
> >I can't see a reason why you would need to hold the lock around this
> >code. Perhaps a better way to fix this would be to drop the lock before
> >calling vp_win_reset()?
> >
> >Thierry
> 
> Hmm, I'm pretty new to spinlocks (only have worked with the usual mutex
> stuff in userspace), but wouldn't that mean that it is then possible for
> mixer_win_reset to execute while a (previous) vp_win_reset is still running?

Indeed it would. I didn't look properly. Looking more closely it seems
the call stack for this looks something like:

	vp_win_reset()
	mixer_win_reset()
	mixer_poweron()
	mixer_dpms()
	exynos_drm_crtc_dpms()

Which can then be called from two places:

	exynos_drm_crtc_commit()
	drm_crtc_helper_set_mode()
	drm_crtc_helper_set_config()

	drm_helper_connector_dpms()
	drm_crtc_helper_set_config()

drm_crtc_helper_set_config() itself must be called with the all modeset
locks held, so I don't see a way how vp_win_reset() could be called
concurrently.

Anyway, even if you're still concerned about concurrent accesses to the
register you'd better lock this section with a mutex to avoid excessive
spinning. In fact I think a better option would be to extend the mutex
in mixer_poweron() to encompass the whole function. This currently looks
broken because one process could go to sleep in pm_runtime_get_sync() or
clk_prepare_enable() and another process start running mixer_poweron()
concurrently, getting to the second mutex_lock() sooner than the first
process. So the lock being dropped between checking for ctx->powered and
setting it doesn't actually prevent a race.

Then again, nobody seems to have cared so far...

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]