Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting
@ 2020-02-13  7:59 Tzung-Bi Shih
  2020-02-14  7:07 ` CK Hu
  0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2020-02-13  7:59 UTC (permalink / raw)
  To: broonie, airlied, daniel
  Cc: alsa-devel, cychiang, tzungbi, dri-devel, ck.hu, linux-mediatek,
	p.zabel, matthias.bgg, dgreid, linux-arm-kernel

hdmi_conn_detect and mtk_hdmi_audio_hook_plugged_cb would be called
by different threads.

Imaging the following calling sequence:
           Thread A                            Thread B
--------------------------------------------------------------------
mtk_hdmi_audio_hook_plugged_cb()
mtk_cec_hpd_high() -> disconnected
                                     hdmi_conn_detect()
                                     mtk_cec_hpd_high() -> connected
                                     plugged_cb(connected)
plugged_cb(disconnected)

The latest disconnected is false reported.  Makes mtk_cec_hpd_high
and plugged_cb atomic to fix.

plugged_cb and codec_dev are also in danger of race condition.  Instead
of using mutex to protect them:
- Checks NULLs first.
- Uses WRITE_ONCE() to prevent store tearing (i.e. write to plugged_cb
  after codec_dev).
- Uses codec_dev as a signal to report HDMI jack status.

Fixes: 5d3c64477392 ("drm/mediatek: support HDMI jack status reporting")

Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
Previous discussion: https://patchwork.kernel.org/patch/11367625/
Previous attempt: https://patchwork.kernel.org/patch/11378413/

 drivers/gpu/drm/mediatek/mtk_hdmi.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 03aeb73005ef..b1e5d0c538fa 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -12,6 +12,7 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
+#include <linux/mutex.h>
 #include <linux/of_platform.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
@@ -171,6 +172,7 @@ struct mtk_hdmi {
 	bool enabled;
 	hdmi_codec_plugged_cb plugged_cb;
 	struct device *codec_dev;
+	struct mutex update_plugged_status_lock;
 };
 
 static inline struct mtk_hdmi *hdmi_ctx_from_bridge(struct drm_bridge *b)
@@ -1199,10 +1201,13 @@ static void mtk_hdmi_clk_disable_audio(struct mtk_hdmi *hdmi)
 static enum drm_connector_status
 mtk_hdmi_update_plugged_status(struct mtk_hdmi *hdmi)
 {
-	bool connected = mtk_cec_hpd_high(hdmi->cec_dev);
+	bool connected;
 
-	if (hdmi->plugged_cb && hdmi->codec_dev)
+	mutex_lock(&hdmi->update_plugged_status_lock);
+	connected = mtk_cec_hpd_high(hdmi->cec_dev);
+	if (hdmi->codec_dev)
 		hdmi->plugged_cb(hdmi->codec_dev, connected);
+	mutex_unlock(&hdmi->update_plugged_status_lock);
 
 	return connected ?
 	       connector_status_connected : connector_status_disconnected;
@@ -1669,8 +1674,12 @@ static int mtk_hdmi_audio_hook_plugged_cb(struct device *dev, void *data,
 {
 	struct mtk_hdmi *hdmi = data;
 
-	hdmi->plugged_cb = fn;
-	hdmi->codec_dev = codec_dev;
+	if (!fn || !codec_dev)
+		return -EINVAL;
+
+	/* Use WRITE_ONCE() to prevent store tearing. */
+	WRITE_ONCE(hdmi->plugged_cb, fn);
+	WRITE_ONCE(hdmi->codec_dev, codec_dev);
 	mtk_hdmi_update_plugged_status(hdmi);
 
 	return 0;
@@ -1729,6 +1738,7 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	mutex_init(&hdmi->update_plugged_status_lock);
 	platform_set_drvdata(pdev, hdmi);
 
 	ret = mtk_hdmi_output_init(hdmi);
-- 
2.25.0.225.g125e21ebc7-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting
  2020-02-13  7:59 [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting Tzung-Bi Shih
@ 2020-02-14  7:07 ` CK Hu
  2020-02-14  7:35   ` Tzung-Bi Shih
  0 siblings, 1 reply; 6+ messages in thread
From: CK Hu @ 2020-02-14  7:07 UTC (permalink / raw)
  To: Tzung-Bi Shih
  Cc: alsa-devel, p.zabel, airlied, dri-devel, broonie, linux-mediatek,
	daniel, matthias.bgg, dgreid, linux-arm-kernel, cychiang

Hi, Tzung-Bi:

On Thu, 2020-02-13 at 15:59 +0800, Tzung-Bi Shih wrote:
> hdmi_conn_detect and mtk_hdmi_audio_hook_plugged_cb would be called
> by different threads.
> 
> Imaging the following calling sequence:
>            Thread A                            Thread B
> --------------------------------------------------------------------
> mtk_hdmi_audio_hook_plugged_cb()
> mtk_cec_hpd_high() -> disconnected
>                                      hdmi_conn_detect()
>                                      mtk_cec_hpd_high() -> connected
>                                      plugged_cb(connected)
> plugged_cb(disconnected)
> 
> The latest disconnected is false reported.  Makes mtk_cec_hpd_high
> and plugged_cb atomic to fix.
> 
> plugged_cb and codec_dev are also in danger of race condition.  Instead
> of using mutex to protect them:
> - Checks NULLs first.
> - Uses WRITE_ONCE() to prevent store tearing (i.e. write to plugged_cb
>   after codec_dev).
> - Uses codec_dev as a signal to report HDMI jack status.
> 
> Fixes: 5d3c64477392 ("drm/mediatek: support HDMI jack status reporting")
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
> ---
> Previous discussion: https://patchwork.kernel.org/patch/11367625/
> Previous attempt: https://patchwork.kernel.org/patch/11378413/
> 
>  drivers/gpu/drm/mediatek/mtk_hdmi.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index 03aeb73005ef..b1e5d0c538fa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -12,6 +12,7 @@
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/mutex.h>
>  #include <linux/of_platform.h>
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
> @@ -171,6 +172,7 @@ struct mtk_hdmi {
>  	bool enabled;
>  	hdmi_codec_plugged_cb plugged_cb;
>  	struct device *codec_dev;
> +	struct mutex update_plugged_status_lock;
>  };
>  
>  static inline struct mtk_hdmi *hdmi_ctx_from_bridge(struct drm_bridge *b)
> @@ -1199,10 +1201,13 @@ static void mtk_hdmi_clk_disable_audio(struct mtk_hdmi *hdmi)
>  static enum drm_connector_status
>  mtk_hdmi_update_plugged_status(struct mtk_hdmi *hdmi)
>  {
> -	bool connected = mtk_cec_hpd_high(hdmi->cec_dev);
> +	bool connected;
>  
> -	if (hdmi->plugged_cb && hdmi->codec_dev)
> +	mutex_lock(&hdmi->update_plugged_status_lock);
> +	connected = mtk_cec_hpd_high(hdmi->cec_dev);
> +	if (hdmi->codec_dev)
>  		hdmi->plugged_cb(hdmi->codec_dev, connected);
> +	mutex_unlock(&hdmi->update_plugged_status_lock);
>  
>  	return connected ?
>  	       connector_status_connected : connector_status_disconnected;
> @@ -1669,8 +1674,12 @@ static int mtk_hdmi_audio_hook_plugged_cb(struct device *dev, void *data,
>  {
>  	struct mtk_hdmi *hdmi = data;
>  
> -	hdmi->plugged_cb = fn;
> -	hdmi->codec_dev = codec_dev;
> +	if (!fn || !codec_dev)

I think sound driver could be removed for some reason, and fn should be
set to NULL before sound driver removed. In this case, codec_dev != NULL
and fn == NULL.

Regards,
CK

> +		return -EINVAL;
> +
> +	/* Use WRITE_ONCE() to prevent store tearing. */
> +	WRITE_ONCE(hdmi->plugged_cb, fn);
> +	WRITE_ONCE(hdmi->codec_dev, codec_dev);
>  	mtk_hdmi_update_plugged_status(hdmi);
>  
>  	return 0;
> @@ -1729,6 +1738,7 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	mutex_init(&hdmi->update_plugged_status_lock);
>  	platform_set_drvdata(pdev, hdmi);
>  
>  	ret = mtk_hdmi_output_init(hdmi);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting
  2020-02-14  7:07 ` CK Hu
@ 2020-02-14  7:35   ` Tzung-Bi Shih
  2020-02-14  8:34     ` CK Hu
  0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2020-02-14  7:35 UTC (permalink / raw)
  To: CK Hu
  Cc: ALSA development, p.zabel, David Airlie, dri-devel, Mark Brown,
	linux-mediatek, Daniel Vetter, Matthias Brugger, Dylan Reid,
	linux-arm-kernel, Jimmy Cheng-Yi Chiang

On Fri, Feb 14, 2020 at 3:07 PM CK Hu <ck.hu@mediatek.com> wrote:
> I think sound driver could be removed for some reason, and fn should be
> set to NULL before sound driver removed. In this case, codec_dev != NULL
> and fn == NULL.

No..if you see sound/soc/codecs/hdmi-codec.c, plugged_cb is statically
allocated.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting
  2020-02-14  7:35   ` Tzung-Bi Shih
@ 2020-02-14  8:34     ` CK Hu
  2020-02-14 23:59       ` Tzung-Bi Shih
  0 siblings, 1 reply; 6+ messages in thread
From: CK Hu @ 2020-02-14  8:34 UTC (permalink / raw)
  To: Tzung-Bi Shih
  Cc: ALSA development, p.zabel, David Airlie, dri-devel, Mark Brown,
	linux-mediatek, Daniel Vetter, Matthias Brugger, Dylan Reid,
	linux-arm-kernel, Jimmy Cheng-Yi Chiang

Hi, Tzung-Bi:

On Fri, 2020-02-14 at 15:35 +0800, Tzung-Bi Shih wrote:
> On Fri, Feb 14, 2020 at 3:07 PM CK Hu <ck.hu@mediatek.com> wrote:
> > I think sound driver could be removed for some reason, and fn should be
> > set to NULL before sound driver removed. In this case, codec_dev != NULL
> > and fn == NULL.
> 
> No..if you see sound/soc/codecs/hdmi-codec.c, plugged_cb is statically
> allocated.

It looks like that even though sound driver is removed, hdmi driver
would still callback to sound core. This is so weird. After sound driver
is removed, hdmi driver would callback with codec_dev which is invalid.
I think this may cause some problem.

Regards,
CK
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting
  2020-02-14  8:34     ` CK Hu
@ 2020-02-14 23:59       ` Tzung-Bi Shih
  2020-02-17  3:21         ` Tzung-Bi Shih
  0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2020-02-14 23:59 UTC (permalink / raw)
  To: CK Hu
  Cc: ALSA development, p.zabel, David Airlie, dri-devel, Mark Brown,
	linux-mediatek, Daniel Vetter, Matthias Brugger, Dylan Reid,
	linux-arm-kernel, Jimmy Cheng-Yi Chiang

On Fri, Feb 14, 2020 at 4:34 PM CK Hu <ck.hu@mediatek.com> wrote:
> It looks like that even though sound driver is removed, hdmi driver
> would still callback to sound core. This is so weird. After sound driver
> is removed, hdmi driver would callback with codec_dev which is invalid.
> I think this may cause some problem.

Will do some tests and get back to you.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting
  2020-02-14 23:59       ` Tzung-Bi Shih
@ 2020-02-17  3:21         ` Tzung-Bi Shih
  0 siblings, 0 replies; 6+ messages in thread
From: Tzung-Bi Shih @ 2020-02-17  3:21 UTC (permalink / raw)
  To: CK Hu
  Cc: ALSA development, p.zabel, David Airlie, dri-devel, Mark Brown,
	linux-mediatek, Daniel Vetter, Matthias Brugger, Dylan Reid,
	linux-arm-kernel, Jimmy Cheng-Yi Chiang

On Sat, Feb 15, 2020 at 7:59 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Fri, Feb 14, 2020 at 4:34 PM CK Hu <ck.hu@mediatek.com> wrote:
> > It looks like that even though sound driver is removed, hdmi driver
> > would still callback to sound core. This is so weird. After sound driver
> > is removed, hdmi driver would callback with codec_dev which is invalid.
> > I think this may cause some problem.
>
> Will do some tests and get back to you.

Please see https://patchwork.kernel.org/cover/11385055/ for the
proposed solution.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13  7:59 [PATCH] drm/mediatek: fix race condition for HDMI jack status reporting Tzung-Bi Shih
2020-02-14  7:07 ` CK Hu
2020-02-14  7:35   ` Tzung-Bi Shih
2020-02-14  8:34     ` CK Hu
2020-02-14 23:59       ` Tzung-Bi Shih
2020-02-17  3:21         ` Tzung-Bi Shih

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git