| Message ID | 20200213153226.I477092c2f104fd589133436c3ae4590e6fc6323b@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mediatek: fix race condition for HDMI jack status reporting | expand |
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);
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.
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
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.
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.
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);
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(-)