* [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 related [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, other threads:[~2020-02-17 3:21 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).