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@xxxxxxxxxxxxxxxxxxx>
> > 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.