Re: [PATCH v3 3/3] drm/msm/dp: check main link status before start aux read

[khsieh]

On 2021-04-20 16:38, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-04-16 10:38:51)
> > Maybe when the cable is disconnected the DP phy should be shutdown and
> > some bit in the phy could effectively "cut off" the aux channel and 
> > then
> > NAKs would start coming through here in the DP controller I/O register
> > space. This patch have DP aux channel read/write to return NAK 
> > immediately
> > if DP controller connection status is in unplugged state.
> >
> > Changes in V3:
> > -- check core_initialized before handle irq_hpd
> > Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_aux.c     |  5 +++++
> >  drivers/gpu/drm/msm/dp/dp_display.c | 14 ++++++++++----
> >  drivers/gpu/drm/msm/dp/dp_link.c    | 20 +++++++++++++++-----
> >  3 files changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> > b/drivers/gpu/drm/msm/dp/dp_aux.c
> > index 7c22bfe..fae3806 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> > *dp_aux,
> >
> >         mutex_lock(&aux->mutex);
> >
> > +       if (!dp_catalog_link_is_connected(aux->catalog)) {
> > +               ret = -ETIMEDOUT;
> > +               goto unlock_exit;
> > +       }
> > +
>
> This still makes me concerned. Any possibility to not do this and have
> the phy cut the connection off and have this transfer timeout
> immediately?
no, we have to wait hardware AUX_NACK timeout.
only this or the abort flag used last time.
Last time you have kernel crash because of service irq_hpd while clock 
is turned off.
I have add core_initialized checking wiinin irq_hpd to prevent this.
I think abort flag approach is safer.

> >         aux->native = msg->request & (DP_AUX_NATIVE_WRITE & 
> > DP_AUX_NATIVE_READ);
> >
> >         /* Ignore address only message */
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 1784e11..db3f45e 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -571,7 +571,7 @@ static int dp_hpd_plug_handle(struct 
> > dp_display_private *dp, u32 data)
> >                 dp->hpd_state = ST_DISCONNECTED;
> >
> >                 if (ret == -ECONNRESET) { /* cable unplugged */
> > -                       dp->core_initialized = false;
> > +                       DRM_ERROR("dongle unplugged = %d\n", ret);
>
> Is this a debug message?
>
> >                 }
> >
> >         } else {
> > @@ -711,9 +711,15 @@ static int dp_irq_hpd_handle(struct 
> > dp_display_private *dp, u32 data)
> >                 return 0;
> >         }
> >
> > -       ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> > -       if (ret == -ECONNRESET) { /* cable unplugged */
> > -               dp->core_initialized = false;
> > +       /*
> > +        * dp core (ahb/aux clks) must be initialized before
> > +        * irq_hpd be handled
> > +        */
> > +       if (dp->core_initialized) {
> > +               ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> > +               if (ret == -ECONNRESET) { /* cable unplugged */
> > +                       DRM_ERROR("dongle unplugged = %d\n", ret);
>
> Another debug message?
>
> > +               }
> >         }
> >
> >         mutex_unlock(&dp->event_mutex);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_link.c 
> > b/drivers/gpu/drm/msm/dp/dp_link.c
> > index be986da..53ecae6 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_link.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> > @@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct 
> > dp_link *dp_link)
> >         return 0;
> >  }
> >
> > -static void dp_link_parse_sink_status_field(struct dp_link_private 
> > *link)
> > +static int dp_link_parse_sink_status_field(struct dp_link_private 
> > *link)
> >  {
> >         int len = 0;
> >
> >         link->prev_sink_count = link->dp_link.sink_count;
> > -       dp_link_parse_sink_count(&link->dp_link);
> > +       len = dp_link_parse_sink_count(&link->dp_link);
> > +       if (len < 0) {
> > +               DRM_ERROR("DP parse sink count failed\n");
> > +               return len;
> > +       }
> >
> >         len = drm_dp_dpcd_read_link_status(link->aux,
> >                 link->link_status);
> > -       if (len < DP_LINK_STATUS_SIZE)
> > +       if (len < DP_LINK_STATUS_SIZE) {
> >                 DRM_ERROR("DP link status read failed\n");
> > -       dp_link_parse_request(link);
> > +               return len;
> > +       }
> > +
> > +       return dp_link_parse_request(link);
> >  }
> >
> >  /**
> > @@ -1032,7 +1039,10 @@ int dp_link_process_request(struct dp_link 
> > *dp_link)
> >
> >         dp_link_reset_data(link);
> >
> > -       dp_link_parse_sink_status_field(link);
> > +       ret = dp_link_parse_sink_status_field(link);
> > +       if (ret) {
> > +               return ret;
> > +       }
> >
> >         if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> >                 dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> > --
>
> Can you split this part off into another patch? It seems to stand on 
> its
> own as it makes the code more robust to transfer errors in the sink
> parsing code.