Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1731122 > unrolled thread

[PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

Back to article view | Back to linux.kernel

Contents

  [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming Colin King <colin.king@canonical.com> - 2017-09-12 19:40 +0200
    Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming Colin Ian King <colin.king@canonical.com> - 2017-09-12 19:50 +0200
      Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming Emil Velikov <emil.l.velikov@gmail.com> - 2017-09-12 20:00 +0200
        Re: [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming Daniel Vetter <daniel@ffwll.ch> - 2017-09-20 20:40 +0200

#1731122 — [PATCH] drm/vmwgfx: remove DRM_ERROR message, stops log spamming

From: Colin Ian King <colin.king@canonical.com>

mmap'ing the device multiple times will spam the kernel log with the
DRM_ERROR message about illegal mmap'ing the old fifo space. Since
the mmap'ing will fail with an -EINVAL there is no need to emit this
message, so just remove it.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index e771091d2cd3..1e633c602fb1 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
 	struct drm_file *file_priv;
 	struct vmw_private *dev_priv;
 
-	if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {
-		DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
+	if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
 		return -EINVAL;
-	}
 
 	file_priv = filp->private_data;
 	dev_priv = vmw_priv(file_priv->minor->dev);
-- 
2.14.1

[toc] | [next] | [standalone]

#1731124

On 12/09/17 18:42, Thomas Hellstrom wrote:
> Hi, Colin,
> 
> On 09/12/2017 07:35 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> mmap'ing the device multiple times will spam the kernel log with the
>> DRM_ERROR message about illegal mmap'ing the old fifo space.
> How are you hitting this? Multiple mappings should be fine as long as
> mapping offsets are correct,
> so hitting this message should indicate that the user-space app is doing
> something seriously wrong, and
> having it present in the log should probably help more than it hurts.
> 
> /Thomas

Good question.  I hit similar issues with the drm qxl driver when
running some kernel regression tests with stress-ng [1]. I realize this
is an artificial test scenario so it is definitely not a typical
use-case, however, sync the illegal mmapping will return -EINVAL the
application will pick up that this is an error without the need of
spotting it in the kernel log. And a user space application can perform
many millions of these invalid mmaps causing kernel log spamming.

[1] http://kernel.ubuntu.com/~cking/stress-ng/

> 
> 
> 
>> Since
>> the mmap'ing will fail with an -EINVAL there is no need to emit this
>> message, so just remove it.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> index e771091d2cd3..1e633c602fb1 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> @@ -33,10 +33,8 @@ int vmw_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>>       struct drm_file *file_priv;
>>       struct vmw_private *dev_priv;
>>   -    if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET)) {
>> -        DRM_ERROR("Illegal attempt to mmap old fifo space.\n");
>> +    if (unlikely(vma->vm_pgoff < VMWGFX_FILE_PAGE_OFFSET))
>>           return -EINVAL;
>> -    }
>>         file_priv = filp->private_data;
>>       dev_priv = vmw_priv(file_priv->minor->dev);
> 
> 

[toc] | [prev] | [next] | [standalone]

#1731129

On 12 September 2017 at 18:47, Colin Ian King <colin.king@canonical.com> wrote:
> On 12/09/17 18:42, Thomas Hellstrom wrote:
>> Hi, Colin,
>>
>> On 09/12/2017 07:35 PM, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> mmap'ing the device multiple times will spam the kernel log with the
>>> DRM_ERROR message about illegal mmap'ing the old fifo space.
>> How are you hitting this? Multiple mappings should be fine as long as
>> mapping offsets are correct,
>> so hitting this message should indicate that the user-space app is doing
>> something seriously wrong, and
>> having it present in the log should probably help more than it hurts.
>>
>> /Thomas
>
> Good question.  I hit similar issues with the drm qxl driver when
> running some kernel regression tests with stress-ng [1]. I realize this
> is an artificial test scenario so it is definitely not a typical
> use-case, however, sync the illegal mmapping will return -EINVAL the
> application will pick up that this is an error without the need of
> spotting it in the kernel log. And a user space application can perform
> many millions of these invalid mmaps causing kernel log spamming.
>
FWIW I'm the one to "blame" here - pointing Colin to drop the message.

Two reasons come to mind:
 - there is a unwritten rule that roughly says "user input should not
cause kernel log spam"
 - out of all the DRM drivers only QXL and VMWGFX print a message,
with a patch addressing the former

HTH
Emil

[toc] | [prev] | [next] | [standalone]

#1735998

On Tue, Sep 12, 2017 at 06:54:45PM +0100, Emil Velikov wrote:
> On 12 September 2017 at 18:47, Colin Ian King <colin.king@canonical.com> wrote:
> > On 12/09/17 18:42, Thomas Hellstrom wrote:
> >> Hi, Colin,
> >>
> >> On 09/12/2017 07:35 PM, Colin King wrote:
> >>> From: Colin Ian King <colin.king@canonical.com>
> >>>
> >>> mmap'ing the device multiple times will spam the kernel log with the
> >>> DRM_ERROR message about illegal mmap'ing the old fifo space.
> >> How are you hitting this? Multiple mappings should be fine as long as
> >> mapping offsets are correct,
> >> so hitting this message should indicate that the user-space app is doing
> >> something seriously wrong, and
> >> having it present in the log should probably help more than it hurts.
> >>
> >> /Thomas
> >
> > Good question.  I hit similar issues with the drm qxl driver when
> > running some kernel regression tests with stress-ng [1]. I realize this
> > is an artificial test scenario so it is definitely not a typical
> > use-case, however, sync the illegal mmapping will return -EINVAL the
> > application will pick up that this is an error without the need of
> > spotting it in the kernel log. And a user space application can perform
> > many millions of these invalid mmaps causing kernel log spamming.
> >
> FWIW I'm the one to "blame" here - pointing Colin to drop the message.
> 
> Two reasons come to mind:
>  - there is a unwritten rule that roughly says "user input should not
> cause kernel log spam"
>  - out of all the DRM drivers only QXL and VMWGFX print a message,
> with a patch addressing the former

Maybe we should make this a written rule by patching
Documentation/drivers/gpu? Would definitely make sense as part of this
patch series.

Thanks, Daniel
> 
> HTH
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web