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

[Dexuan Cui]

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

> > > +		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);