[PATCH v3] drm/hyperv: Added error message for fb size greater than allocated

[PATCH v3] drm/hyperv: Added error message for fb size greater than allocated

[Saurabh Sengar]

Added error message when the size of requested framebuffer is more than
the allocated size by vmbus mmio region for framebuffer

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
v2 -> v3 : then -> than (typo fix)

 drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c b/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
index e82b815..6634818 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
@@ -123,8 +123,11 @@ static int hyperv_pipe_check(struct drm_simple_display_pipe *pipe,
 	if (fb->format->format != DRM_FORMAT_XRGB8888)
 		return -EINVAL;
 
-	if (fb->pitches[0] * fb->height > hv->fb_size)
+	if (fb->pitches[0] * fb->height > hv->fb_size) {
+		drm_err(&hv->dev, "hv->hdev, fb size requested by process %s for %d X %d (pitch %d) is greater than allocated size %ld\n",
+		current->comm, fb->width, fb->height, fb->pitches[0], hv->fb_size);
 		return -EINVAL;
+	}
 
 	return 0;
 }
-- 
1.8.3.1

RE: [PATCH v3] drm/hyperv: Added error message for fb size greater than allocated

[Dexuan Cui]

> Subject: [PATCH v3] drm/hyperv: Added error message for fb size greater than
> allocated
> 
> Added error message when the size of requested framebuffer is more than
> the allocated size by vmbus mmio region for framebuffer

"Added" --> "Add"? My impression is that we don't use past tense in the 
Subject and the commit message. See
"git log drivers/gpu/drm/hyperv/hyperv_drm_modeset.c".
 
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
> @@ -123,8 +123,11 @@ static int hyperv_pipe_check(struct
> drm_simple_display_pipe *pipe,
>  	if (fb->format->format != DRM_FORMAT_XRGB8888)
>  		return -EINVAL;
> 
> -	if (fb->pitches[0] * fb->height > hv->fb_size)
> +	if (fb->pitches[0] * fb->height > hv->fb_size) {
> +		drm_err(&hv->dev, "hv->hdev, fb size requested by process %s
> for %d X %d (pitch %d) is greater than allocated size %ld\n",
Should we use drm_err_ratelimited() instead of drm_err()?

The line exceeds 80 chars.

> +		current->comm, fb->width, fb->height, fb->pitches[0], hv->fb_size);
>  		return -EINVAL;
> +	}

Maybe we can use the below:
	drm_err_ratelimited(&hv->dev, "%s: requested %dX%d (pitch %d) "
                     "exceeds fb_size %ld\n",
                     current->comm, fb->width, fb->height,
                     fb->pitches[0], hv->fb_size);

Note: the first chars of last 3 lines should align with the "&" in the
same column. Please run "scripts/checkpatch.pl" against the patch.

Re: [PATCH v3] drm/hyperv: Added error message for fb size greater than allocated

[Saurabh Singh Sengar]

On Mon, Apr 11, 2022 at 06:40:38AM +0000, Dexuan Cui wrote:
> > Subject: [PATCH v3] drm/hyperv: Added error message for fb size greater than
> > allocated
> > 
> > Added error message when the size of requested framebuffer is more than
> > the allocated size by vmbus mmio region for framebuffer
> 
> "Added" --> "Add"? My impression is that we don't use past tense in the 

Ok.

> Subject and the commit message. See
> "git log drivers/gpu/drm/hyperv/hyperv_drm_modeset.c".
>  
> > --- a/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
> > +++ b/drivers/gpu/drm/hyperv/hyperv_drm_modeset.c
> > @@ -123,8 +123,11 @@ static int hyperv_pipe_check(struct
> > drm_simple_display_pipe *pipe,
> >  	if (fb->format->format != DRM_FORMAT_XRGB8888)
> >  		return -EINVAL;
> > 
> > -	if (fb->pitches[0] * fb->height > hv->fb_size)
> > +	if (fb->pitches[0] * fb->height > hv->fb_size) {
> > +		drm_err(&hv->dev, "hv->hdev, fb size requested by process %s
> > for %d X %d (pitch %d) is greater than allocated size %ld\n",
> Should we use drm_err_ratelimited() instead of drm_err()?

The error is not produced in good cases, for every resolution change which is violating this error should print once.
I suggest having it print every time some application tries to violate the policy set at boot time.
If we use ratelimit many resolutions error change will be suppressed. Please let me know your thoughts.


> 
> The line exceeds 80 chars.

At first I tried braking the line to respect 80 character boundary, but checkpatch.pl reported it as a problem.
And these new lines are suggested by checkpatch.pl itself.
Looks the recent rule realted to 80 charachters are changed. Ref : https://www.theregister.com/2020/06/01/linux_5_7/#:~:text=Linux%20kernel%20overlord%20Linus%20Torvalds,the%20topic%20of%20line%20lengths.

> 
> > +		current->comm, fb->width, fb->height, fb->pitches[0], hv->fb_size);
> >  		return -EINVAL;
> > +	}
> 
> Maybe we can use the below:
> 	drm_err_ratelimited(&hv->dev, "%s: requested %dX%d (pitch %d) "
>                      "exceeds fb_size %ld\n",
>                      current->comm, fb->width, fb->height,
>                      fb->pitches[0], hv->fb_size);
> 
> Note: the first chars of last 3 lines should align with the "&" in the
> same column. Please run "scripts/checkpatch.pl" against the patch.

I have tested checkpatch.pl before sending, for the current patch there is no problem from checkpatch.pl

RE: [PATCH v3] drm/hyperv: Added error message for fb size greater than allocated

[Dexuan Cui]

> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> Sent: Monday, April 11, 2022 12:56 AM
> > >...
> > > -	if (fb->pitches[0] * fb->height > hv->fb_size)
> > > +	if (fb->pitches[0] * fb->height > hv->fb_size) {
> > > +		drm_err(&hv->dev, "hv->hdev, fb size requested by process %s
> > > for %d X %d (pitch %d) is greater than allocated size %ld\n",
> > Should we use drm_err_ratelimited() instead of drm_err()?
> 
> The error is not produced in good cases, for every resolution change which is
> violating this error should print once.

Thanks for the clarification! Then drm_err() looks good to me.

> I suggest having it print every time some application tries to violate the policy
> set at boot time.
> If we use ratelimit many resolutions error change will be suppressed. Please let
> me know your thoughts.
 
> >
> > The line exceeds 80 chars.
> 
> At first I tried braking the line to respect 80 character boundary, but
> checkpatch.pl reported it as a problem.
> And these new lines are suggested by checkpatch.pl itself.
> Looks the recent rule realted to 80 charachters are changed. Ref :
> ...

Good to know! Thanks for sharing the link!

FYI, the default max_line_length in scripts/checkpatch.pl is 100 now:
 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f

"80-chars" is still preferred, but isn't a hard limit. I also noticed
"never break user-visible strings such as printk messages ", so yes you're 
correct. It's perfectly fine to have a not-too-long string that exceeds 80 chars.

> > > +		current->comm, fb->width, fb->height, fb->pitches[0], hv->fb_size);
> > >  		return -EINVAL;
> > > +	}
> >
> > Maybe we can use the below:
> > 	drm_err_ratelimited(&hv->dev, "%s: requested %dX%d (pitch %d) "
> >                      "exceeds fb_size %ld\n",
> >                      current->comm, fb->width, fb->height,
> >                      fb->pitches[0], hv->fb_size);
> >
> > Note: the first chars of last 3 lines should align with the "&" in the
> > same column. Please run "scripts/checkpatch.pl" against the patch.
> 
> I have tested checkpatch.pl before sending, for the current patch there is no
> problem from checkpatch.pl

The line is 138-char long, which seems a little longer to me :-)
IMO we can make it shorter, e.g. be removing the part "hv->hdev as the
"drm_err(&hv->dev," already tells us which device it's.

BTW, if we run the script with --strict, it reports the below:

# scripts/checkpatch.pl --strict 0001-drm-hyperv-Added-error-message-for-fb-size-greater-t.patch
CHECK: Alignment should match open parenthesis
#28: FILE: drivers/gpu/drm/hyperv/hyperv_drm_modeset.c:127:
+               drm_err(&hv->dev, "hv->hdev, fb size requested by process %s for %d X %d (pitch %d) is greater than allocated size %ld\n",
+               current->comm, fb->width, fb->height, fb->pitches[0], hv->fb_size);

Re: [PATCH v3] drm/hyperv: Added error message for fb size greater than allocated

[Saurabh Singh Sengar]

On Mon, Apr 11, 2022 at 07:02:19PM +0000, Dexuan Cui wrote:
> > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> > Sent: Monday, April 11, 2022 12:56 AM
> > > >...
> > > > -	if (fb->pitches[0] * fb->height > hv->fb_size)
> > > > +	if (fb->pitches[0] * fb->height > hv->fb_size) {
> > > > +		drm_err(&hv->dev, "hv->hdev, fb size requested by process %s
> > > > for %d X %d (pitch %d) is greater than allocated size %ld\n",
> > > Should we use drm_err_ratelimited() instead of drm_err()?
> > 
> > The error is not produced in good cases, for every resolution change which is
> > violating this error should print once.
> 
> Thanks for the clarification! Then drm_err() looks good to me.
> 
> > I suggest having it print every time some application tries to violate the policy
> > set at boot time.
> > If we use ratelimit many resolutions error change will be suppressed. Please let
> > me know your thoughts.
>  
> > >
> > > The line exceeds 80 chars.
> > 
> > At first I tried braking the line to respect 80 character boundary, but
> > checkpatch.pl reported it as a problem.
> > And these new lines are suggested by checkpatch.pl itself.
> > Looks the recent rule realted to 80 charachters are changed. Ref :
> > ...
> 
> Good to know! Thanks for sharing the link!
> 
> FYI, the default max_line_length in scripts/checkpatch.pl is 100 now:
>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f
> 
> "80-chars" is still preferred, but isn't a hard limit. I also noticed
> "never break user-visible strings such as printk messages ", so yes you're 
> correct. It's perfectly fine to have a not-too-long string that exceeds 80 chars.
> 

Good information ! thank you for digging this.

> > > > +		current->comm, fb->width, fb->height, fb->pitches[0], hv->fb_size);
> > > >  		return -EINVAL;
> > > > +	}
> > >
> > > Maybe we can use the below:
> > > 	drm_err_ratelimited(&hv->dev, "%s: requested %dX%d (pitch %d) "
> > >                      "exceeds fb_size %ld\n",
> > >                      current->comm, fb->width, fb->height,
> > >                      fb->pitches[0], hv->fb_size);
> > >
> > > Note: the first chars of last 3 lines should align with the "&" in the
> > > same column. Please run "scripts/checkpatch.pl" against the patch.
> > 
> > I have tested checkpatch.pl before sending, for the current patch there is no
> > problem from checkpatch.pl
> 
> The line is 138-char long, which seems a little longer to me :-)
> IMO we can make it shorter, e.g. be removing the part "hv->hdev as the
> "drm_err(&hv->dev," already tells us which device it's.

Ok, will make it shorter.

> 
> BTW, if we run the script with --strict, it reports the below:
> 
> # scripts/checkpatch.pl --strict 0001-drm-hyperv-Added-error-message-for-fb-size-greater-t.patch
> CHECK: Alignment should match open parenthesis
> #28: FILE: drivers/gpu/drm/hyperv/hyperv_drm_modeset.c:127:
> +               drm_err(&hv->dev, "hv->hdev, fb size requested by process %s for %d X %d (pitch %d) is greater than allocated size %ld\n",
> +               current->comm, fb->width, fb->height, fb->pitches[0], hv->fb_size);
Sure, will fix this.