diff mbox series

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

Message ID 20240330-dpu-irq-messages-v1-1-9ce782ae35f9@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible | expand

Commit Message

Dmitry Baryshkov March 30, 2024, 3:53 a.m. UTC 
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(-)


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

Best regards,

Comments

Marijn Suijten March 30, 2024, 10:31 a.m. UTC | #1 
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>
>
Abhinav Kumar March 30, 2024, 7:21 p.m. UTC | #2 
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>
Marijn Suijten March 30, 2024, 8:07 p.m. UTC | #3 
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>
> >
Abhinav Kumar April 5, 2024, 7:29 p.m. UTC | #4 
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,
diff mbox series

Patch

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;
 	}