Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 11 Apr 2022 00:55:55 -0700
From:   Saurabh Singh Sengar <ssengar@linux.microsoft.com>
To:     Dexuan Cui <decui@microsoft.com>
Cc:     Saurabh Singh Sengar <ssengar@microsoft.com>,
        "drawat.floss@gmail.com" <drawat.floss@gmail.com>,
        "airlied@linux.ie" <airlied@linux.ie>,
        "daniel@ffwll.ch" <daniel@ffwll.ch>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Michael Kelley (LINUX)" <mikelley@microsoft.com>
Subject: Re: [PATCH v3] drm/hyperv: Added error message for fb size greater
 than allocated
Message-ID: <20220411075555.GA15355@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
References: <1649650437-17977-1-git-send-email-ssengar@linux.microsoft.com>
 <BYAPR21MB1270B3CFBE674EB0A7537180BFEA9@BYAPR21MB1270.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BYAPR21MB1270B3CFBE674EB0A7537180BFEA9@BYAPR21MB1270.namprd21.prod.outlook.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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