Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932408AbbBBED0 (ORCPT ); Sun, 1 Feb 2015 23:03:26 -0500 Received: from mail-vc0-f180.google.com ([209.85.220.180]:46409 "EHLO mail-vc0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755102AbbBBEDM (ORCPT ); Sun, 1 Feb 2015 23:03:12 -0500 MIME-Version: 1.0 In-Reply-To: <54CCE7EF.5040706@rock-chips.com> References: <1422617031-25098-1-git-send-email-ykk@rock-chips.com> <1422617543-25684-1-git-send-email-ykk@rock-chips.com> <20150131114806.GD26493@n2100.arm.linux.org.uk> <54CCE7EF.5040706@rock-chips.com> From: Daniel Kurtz Date: Mon, 2 Feb 2015 12:02:50 +0800 X-Google-Sender-Auth: 9meavCBLpzOAs6AYwJpIatURx4E Message-ID: Subject: Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces To: Yang Kuankuan Cc: Russell King - ARM Linux , David Airlie , Philipp Zabel , Fabio Estevam , Shawn Guo , Rob Clark , Mark Yao , Daniel Vetter , dri-devel , "linux-kernel@vger.kernel.org" , dbehr@chromoum.org, =?UTF-8?Q?Heiko_St=C3=BCbner?= , Douglas Anderson , =?UTF-8?Q?St=C3=A9phane_Marchesin?= , rockchip-discuss Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2872 Lines: 78 Hi ykk, On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan wrote: > > On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote: >> >>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi) >>> +{ >>> + if (hdmi->audio_enable) >>> + return; >>> + >>> + mutex_lock(&hdmi->audio_mutex); >>> + hdmi->audio_enable = true; >>> + hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, >>> HDMI_MC_CLKDIS); >>> + mutex_unlock(&hdmi->audio_mutex); >> >> This is racy. The test needs to be within the mutex-protected region. > > This function will be called by other driver (dw-hdmi-audio), both modify > the variable "hdmi->audio_enable", so i add the mutex-protected. >From your comment it isn't clear whether you understand what Russell meant. He is say you should do the following: { mutex_lock(&hdmi->audio_mutex); if (hdmi->audio_enable) { mutex_unlock(&hdmi->audio_mutex); return; } hdmi->audio_enable = true; hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS); mutex_unlock(&hdmi->audio_mutex); } By the way, it doesn't matter that the function is called from another driver. What matters is that this function can be called concurrently on multiple different threads of execution to change the hdmi audio enable state. >From DRM land, it is called with DRM lock held when enabling/disabling hdmi audio (mode_set / DPMS). It is also called from audio land, when enabling/disabling audio in response to some audio events (userspace ioctls?). I'm not sure exactly how the audio side works, or what locks are involved, but this mutex synchronizes calls from these two worlds to ensure that "hdmi->audio_enable" field always matches the current (intended) status of the hdmi audio clock. This would be useful, for example, if you needed to temporarily disable all HDMI clocks during a mode set, and then restore the audio clock to its pre-mode_set state: // temporarily disable hdmi audio clk dw_hdmi_audio_clk_disable(hdmi); // do mode_set ... dw_hdmi_audio_clk_reenable(hdmi); static void dw_hdmi_audio_clk_reenable() { mutex_lock() if (hdmi->audio_enable) dw_hdmi_audio_clk_enable(hdmi) mutex_unlock() } However, AFAICT, the "hdmi->audio_enable" field is never actually used like this here or in later patches, so it and the mutex do not seem to actually be doing anything useful. In this patch it is probably better to just drop the mutex and audio_enable, and add them as a preparatory patch in the patch set where they will actually be used. -Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/