Re: [PATCH] drm/mediatek: dp: drm_err => dev_err in HPD path to avoid NULL ptr

[CK Hu (胡俊光)]

Hi, Douglas:

On Thu, 2025-01-16 at 09:42 -0800, Douglas Anderson wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> The function mtk_dp_wait_hpd_asserted() may be called before the
> `mtk_dp->drm_dev` pointer is assigned in mtk_dp_bridge_attach().
> Specifically it can be called via this callpath:
>  - mtk_edp_wait_hpd_asserted
>  - [panel probe]
>  - dp_aux_ep_probe
> 
> Using "drm" level prints anywhere in this callpath causes a NULL
> pointer dereference. Change the error message directly in
> mtk_dp_wait_hpd_asserted() to dev_err() to avoid this. Also change the
> error messages in mtk_dp_parse_capabilities(), which is called by
> mtk_dp_wait_hpd_asserted().
> 
> While touching these prints, also add the error code to them to make
> future debugging easier.

Reviewed-by: CK Hu <ck.hu@xxxxxxxxxxxx>

> 
> Fixes: 7eacba9a083b ("drm/mediatek: dp: Add .wait_hpd_asserted() for AUX bus")
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> Unfortunately, I have only been able to compile-time test this code. I
> hit the NULL pointer dereference on a device that's nowhere near
> upstream and it was running (sigh) a heavily modified copy of this
> code where the eDP stuff has been forked out of DP. Specifically, you
> can see <https://urldefense.com/v3/__https://crrev.com/c/6073744__;!!CTRNKA9wMg0ARbw!jb1QV1V5hUEEdobvFihzW88qPITWculzska3y5GtFBXuxiiOWeavhqC5GItPDzbQ0Jdiz7hEGpvaOdsCxA$ >. It's pretty easy to understand
> that the same problem affects both codebases though, so I'm posting
> this "blind" in the hopes to at least fix upstream.
> 
> I'll also note that the fact that mtk_edp_wait_hpd_asserted() calls
> mtk_dp_parse_capabilities() feels weird/wrong to me based on other eDP
> code I've worked on, but I've only barely looked at the Mediatek
> driver and perhaps others have already debated this. In any case,
> that's not directly related to this patch.
> 
>  drivers/gpu/drm/mediatek/mtk_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 0687672f0e52..ccd104d8851f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1763,7 +1763,7 @@ static int mtk_dp_parse_capabilities(struct mtk_dp *mtk_dp)
> 
>         ret = drm_dp_dpcd_readb(&mtk_dp->aux, DP_MSTM_CAP, &val);
>         if (ret < 1) {
> -               drm_err(mtk_dp->drm_dev, "Read mstm cap failed\n");
> +               dev_err(mtk_dp->dev, "Read mstm cap failed: %zd\n", ret);
>                 return ret == 0 ? -EIO : ret;
>         }
> 
> @@ -1773,7 +1773,7 @@ static int mtk_dp_parse_capabilities(struct mtk_dp *mtk_dp)
>                                         DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
>                                         &val);
>                 if (ret < 1) {
> -                       drm_err(mtk_dp->drm_dev, "Read irq vector failed\n");
> +                       dev_err(mtk_dp->dev, "Read irq vector failed: %zd\n", ret);
>                         return ret == 0 ? -EIO : ret;
>                 }
> 
> @@ -2056,7 +2056,7 @@ static int mtk_dp_wait_hpd_asserted(struct drm_dp_aux *mtk_aux, unsigned long wa
> 
>         ret = mtk_dp_parse_capabilities(mtk_dp);
>         if (ret) {
> -               drm_err(mtk_dp->drm_dev, "Can't parse capabilities\n");
> +               dev_err(mtk_dp->dev, "Can't parse capabilities: %d\n", ret);
>                 return ret;
>         }
> 
> --
> 2.48.0.rc2.279.g1de40edade-goog
>