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