Re: [PATCH] drm/msm/dp: stop event kernel thread when DP unbind

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/11/2022 5:22 PM, Dmitry Baryshkov wrote:
On 12/04/2022 03:21, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-04-11 17:08:49)
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().

Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>

Should add a Reported-by tag from Dmitry here.

---
  drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++--
  1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..fa1ef8e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -273,6 +274,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
         return rc;
  }

+static void dp_hpd_event_stop(struct dp_display_private *dp_priv);

Why can't the function be defined here?

+
  static void dp_display_unbind(struct device *dev, struct device *master,
                               void *data)
  {
@@ -280,6 +283,8 @@ static void dp_display_unbind(struct device *dev, struct device *master,
         struct drm_device *drm = dev_get_drvdata(master);
         struct msm_drm_private *priv = drm->dev_private;

+       disable_irq(dp->irq);
+       dp_hpd_event_stop(dp);
         dp_power_client_deinit(dp->power);
         dp_aux_unregister(dp->aux);
         priv->dp[dp->id] = NULL;
@@ -1054,7 +1059,7 @@ static int hpd_event_thread(void *data)

         dp_priv = (struct dp_display_private *)data;

-       while (1) {
+       while (!kthread_should_stop()) {
                 if (timeout_mode) {
                         wait_event_timeout(dp_priv->event_q,
                                 (dp_priv->event_pndx == dp_priv->event_gndx), @@ -1137,7 +1142,22 @@ static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
         init_waitqueue_head(&dp_priv->event_q);
         spin_lock_init(&dp_priv->event_lock);

-       kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
+       dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
+
+       if (IS_ERR(dp_priv->ev_tsk))
+               DRM_ERROR("failed to create DP event thread\n");

Why can't we error out? Why can't this kthread be started in probe?

Just my 2c. I don't think starting it in probe is a good idea. The driver uses components, so, in my opinion, the thread should be started from bind and stopped in unbind.

Yes, I also agree it should be started in bind and stopped in unbind.



+}
+
+static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
+{
+       if (IS_ERR(dp_priv->ev_tsk))
+               return;
+
+       kthread_stop(dp_priv->ev_tsk);
+
+       /* reset event q to empty */
+       dp_priv->event_gndx = 0;
+       dp_priv->event_pndx = 0;
  }






[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux