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.
