[PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible

[PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible

[Dmitry Baryshkov]

There is little point in using %ps to print a value known to be NULL. On
the other hand it makes sense to print the callback symbol in the
'invalid IRQ' message. Correct those two error messages to make more
sense.

Fixes: 6893199183f8 ("drm/msm/dpu: stop using raw IRQ indices in the kernel output")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 946dd0135dff..6a0a74832fb6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -525,14 +525,14 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms,
 	int ret;
 
 	if (!irq_cb) {
-		DPU_ERROR("invalid IRQ=[%d, %d] irq_cb:%ps\n",
-			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb);
+		DPU_ERROR("IRQ=[%d, %d] NULL callback\n",
+			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
 		return -EINVAL;
 	}
 
 	if (!dpu_core_irq_is_valid(irq_idx)) {
-		DPU_ERROR("invalid IRQ=[%d, %d]\n",
-			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
+		DPU_ERROR("invalid IRQ=[%d, %d] irq_cb:%ps\n",
+			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb);
 		return -EINVAL;
 	}
 

---
base-commit: 13ee4a7161b6fd938aef6688ff43b163f6d83e37
change-id: 20240330-dpu-irq-messages-5cf13fd7568c

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible

[Marijn Suijten]

On 2024-03-30 05:53:22, Dmitry Baryshkov wrote:
> There is little point in using %ps to print a value known to be NULL. On
> the other hand it makes sense to print the callback symbol in the
> 'invalid IRQ' message. Correct those two error messages to make more
> sense.
> 
> Fixes: 6893199183f8 ("drm/msm/dpu: stop using raw IRQ indices in the kernel output")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Agreed, this is a lot more clear:

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> index 946dd0135dff..6a0a74832fb6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -525,14 +525,14 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms,
>  	int ret;
>  
>  	if (!irq_cb) {
> -		DPU_ERROR("invalid IRQ=[%d, %d] irq_cb:%ps\n",
> -			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb);
> +		DPU_ERROR("IRQ=[%d, %d] NULL callback\n",
> +			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
>  		return -EINVAL;
>  	}
>  
>  	if (!dpu_core_irq_is_valid(irq_idx)) {
> -		DPU_ERROR("invalid IRQ=[%d, %d]\n",
> -			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
> +		DPU_ERROR("invalid IRQ=[%d, %d] irq_cb:%ps\n",
> +			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb);
>  		return -EINVAL;
>  	}
>  
> 
> ---
> base-commit: 13ee4a7161b6fd938aef6688ff43b163f6d83e37
> change-id: 20240330-dpu-irq-messages-5cf13fd7568c
> 
> Best regards,
> -- 
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 

Re: [PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible

[Abhinav Kumar]

On 3/29/2024 8:53 PM, Dmitry Baryshkov wrote:
> There is little point in using %ps to print a value known to be NULL. On
> the other hand it makes sense to print the callback symbol in the
> 'invalid IRQ' message. Correct those two error messages to make more
> sense.
> 
> Fixes: 6893199183f8 ("drm/msm/dpu: stop using raw IRQ indices in the kernel output")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible

[Marijn Suijten]

On 2024-03-30 11:31:37, Marijn Suijten wrote:
> On 2024-03-30 05:53:22, Dmitry Baryshkov wrote:
> > There is little point in using %ps to print a value known to be NULL. On
> > the other hand it makes sense to print the callback symbol in the
> > 'invalid IRQ' message. Correct those two error messages to make more
> > sense.
> > 
> > Fixes: 6893199183f8 ("drm/msm/dpu: stop using raw IRQ indices in the kernel output")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Agreed, this is a lot more clear:
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

Note that, as seen in [1], there are still a few codepaths that only print
"invalid IRQ" without any additional context (if we could even add it - __func__
might be a good start) and inconsistently use DPU_ERROR vs pr_err too :/

Any possibility to address that?

- Marijn

[1]: https://lore.kernel.org/linux-arm-msm/mxwrvnqth5f2vd4m55ryzqgyj7brykiqynzldelanxkuj2zny3@4pqi6p57c2q2/

> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> > index 946dd0135dff..6a0a74832fb6 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> > @@ -525,14 +525,14 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms,
> >  	int ret;
> >  
> >  	if (!irq_cb) {
> > -		DPU_ERROR("invalid IRQ=[%d, %d] irq_cb:%ps\n",
> > -			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb);
> > +		DPU_ERROR("IRQ=[%d, %d] NULL callback\n",
> > +			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
> >  		return -EINVAL;
> >  	}
> >  
> >  	if (!dpu_core_irq_is_valid(irq_idx)) {
> > -		DPU_ERROR("invalid IRQ=[%d, %d]\n",
> > -			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
> > +		DPU_ERROR("invalid IRQ=[%d, %d] irq_cb:%ps\n",
> > +			  DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb);
> >  		return -EINVAL;
> >  	}
> >  
> > 
> > ---
> > base-commit: 13ee4a7161b6fd938aef6688ff43b163f6d83e37
> > change-id: 20240330-dpu-irq-messages-5cf13fd7568c
> > 
> > Best regards,
> > -- 
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 

Re: [PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible

[Abhinav Kumar]

On Sat, 30 Mar 2024 05:53:22 +0200, Dmitry Baryshkov wrote:
> There is little point in using %ps to print a value known to be NULL. On
> the other hand it makes sense to print the callback symbol in the
> 'invalid IRQ' message. Correct those two error messages to make more
> sense.
> 
> 

Applied, thanks!

[1/1] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible
      https://gitlab.freedesktop.org/drm/msm/-/commit/8844f467d6a5

Best regards,
-- 
Abhinav Kumar <quic_abhinavk@quicinc.com>