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 <redacted>
---
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(-)
@@ -1669,8 +1674,12 @@ static int mtk_hdmi_audio_hook_plugged_cb(struct device *dev, void *data,{structmtk_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);return0;
@@ -1729,6 +1738,7 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)returnret;}+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
Hi, Tzung-Bi:
On Thu, 2020-02-13 at 15:59 +0800, Tzung-Bi Shih wrote:
quoted hunk
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 <redacted>
---
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(-)
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
quoted hunk
+ 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);
On Fri, Feb 14, 2020 at 3:07 PM CK Hu [off-list ref] 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
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 [off-list ref] wrote:
quoted
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
On Fri, Feb 14, 2020 at 4:34 PM CK Hu [off-list ref] 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.
On Sat, Feb 15, 2020 at 7:59 AM Tzung-Bi Shih [off-list ref] wrote:
On Fri, Feb 14, 2020 at 4:34 PM CK Hu [off-list ref] wrote:
quoted
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.