* [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc
@ 2016-08-03 4:42 Rodrigo Vivi
2016-08-03 4:42 ` [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function Rodrigo Vivi
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2016-08-03 4:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Rodrigo Vivi
Copy to user return the number of bytes it couldn't write
and zero on success. So any number different than 0 should
be considered a fault, not only when it doesn't write
the full size.
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7c42ec4..7052c47 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3632,7 +3632,6 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
while (n_entries > 0) {
struct intel_pipe_crc_entry *entry =
&pipe_crc->entries[pipe_crc->tail];
- int ret;
if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
INTEL_PIPE_CRC_ENTRIES_NR) < 1)
@@ -3649,8 +3648,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
spin_unlock_irq(&pipe_crc->lock);
- ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
- if (ret == PIPE_CRC_LINE_LEN)
+ if (!copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN))
return -EFAULT;
user_buf += PIPE_CRC_LINE_LEN;
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function.
2016-08-03 4:42 [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc Rodrigo Vivi
@ 2016-08-03 4:42 ` Rodrigo Vivi
2016-08-03 7:31 ` Ville Syrjälä
2016-08-03 7:48 ` Daniel Vetter
2016-08-03 6:49 ` ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Fix copy_to_user usage for pipe_crc Patchwork
` (2 subsequent siblings)
3 siblings, 2 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2016-08-03 4:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Rodrigo Vivi
A read(fd, buf, len) function should return the number
of bytes read. In our case we need to return the
number of bytes we copy to user, instead of returning
the number of bytes we read internally.
It was really strange when I saw i-g-t test case using
len '54' but getting '56' as return. First thought was
how do we read more than we asked? But also I checked
and there was really only 54. Until I realized it
was all our fault. EFAULT!
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7052c47..b7b8d79 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3659,7 +3659,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
spin_unlock_irq(&pipe_crc->lock);
- return bytes_read;
+ return PIPE_CRC_LINE_LEN;
}
static const struct file_operations i915_pipe_crc_fops = {
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Fix copy_to_user usage for pipe_crc
2016-08-03 4:42 [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc Rodrigo Vivi
2016-08-03 4:42 ` [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function Rodrigo Vivi
@ 2016-08-03 6:49 ` Patchwork
2016-08-03 7:29 ` [PATCH 1/2] " Ville Syrjälä
2016-08-03 7:44 ` Daniel Vetter
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2016-08-03 6:49 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Fix copy_to_user usage for pipe_crc
URL : https://patchwork.freedesktop.org/series/10573/
State : failure
== Summary ==
Series 10573v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/10573/revisions/1/mbox
Test kms_cursor_legacy:
Subgroup basic-cursor-vs-flip-legacy:
pass -> FAIL (ro-byt-n2820)
Subgroup basic-flip-vs-cursor-legacy:
pass -> FAIL (fi-skl-i7-6700k)
Test kms_frontbuffer_tracking:
Subgroup basic:
pass -> DMESG-FAIL (ro-bdw-i7-5557U)
skip -> FAIL (ro-ilk1-i5-650)
pass -> DMESG-FAIL (ro-hsw-i3-4010u)
pass -> FAIL (fi-skl-i5-6260u)
pass -> DMESG-FAIL (ro-bdw-i7-5600u)
pass -> FAIL (ro-snb-i7-2620M)
pass -> DMESG-FAIL (ro-bdw-i5-5250u)
pass -> FAIL (ro-hsw-i7-4770r)
pass -> FAIL (fi-hsw-i7-4770k)
pass -> FAIL (fi-snb-i7-2600)
pass -> FAIL (ro-ivb-i7-3770)
pass -> FAIL (ro-ivb2-i7-3770)
pass -> FAIL (ro-byt-n2820)
pass -> FAIL (fi-skl-i7-6700k)
pass -> FAIL (ro-skl3-i5-6260u)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> FAIL (ro-bdw-i7-5557U)
pass -> FAIL (ro-ilk1-i5-650)
pass -> FAIL (ro-hsw-i3-4010u)
pass -> FAIL (fi-skl-i5-6260u)
pass -> FAIL (ro-bdw-i7-5600u)
pass -> FAIL (ro-snb-i7-2620M)
pass -> FAIL (ro-bdw-i5-5250u)
pass -> FAIL (ro-hsw-i7-4770r)
pass -> FAIL (fi-snb-i7-2600)
pass -> FAIL (ro-ivb-i7-3770)
pass -> FAIL (ro-ivb2-i7-3770)
pass -> FAIL (ro-byt-n2820)
pass -> FAIL (ro-skl3-i5-6260u)
Subgroup hang-read-crc-pipe-b:
pass -> FAIL (ro-bdw-i7-5557U)
pass -> FAIL (ro-ilk1-i5-650)
pass -> FAIL (ro-hsw-i3-4010u)
pass -> FAIL (fi-skl-i5-6260u)
pass -> FAIL (ro-bdw-i7-5600u)
pass -> FAIL (ro-snb-i7-2620M)
pass -> FAIL (ro-bdw-i5-5250u)
pass -> FAIL (ro-hsw-i7-4770r)
pass -> FAIL (fi-snb-i7-2600)
pass -> FAIL (ro-ivb-i7-3770)
pass -> FAIL (ro-ivb2-i7-3770)
pass -> FAIL (ro-byt-n2820)
pass -> FAIL (ro-skl3-i5-6260u)
Subgroup hang-read-crc-pipe-c:
pass -> FAIL (ro-bdw-i7-5557U)
pass -> FAIL (ro-hsw-i3-4010u)
pass -> FAIL (fi-skl-i5-6260u)
pass -> FAIL (ro-bdw-i7-5600u)
pass -> FAIL (ro-bdw-i5-5250u)
pass -> FAIL (ro-hsw-i7-4770r)
pass -> FAIL (ro-ivb-i7-3770)
pass -> FAIL (ro-ivb2-i7-3770)
pass -> FAIL (ro-skl3-i5-6260u)
Subgroup nonblocking-crc-pipe-a:
pass -> FAIL (ro-bdw-i7-5557U)
pass -> FAIL (ro-ilk1-i5-650)
pass -> FAIL (ro-hsw-i3-4010u)
pass -> FAIL (fi-skl-i5-6260u)
pass -> FAIL (ro-bdw-i7-5600u)
pass -> FAIL (ro-snb-i7-2620M)
pass -> FAIL (ro-bdw-i5-5250u)
pass -> FAIL (ro-hsw-i7-4770r)
pass -> FAIL (fi-hsw-i7-4770k)
pass -> FAIL (fi-snb-i7-2600)
pass -> FAIL (ro-ivb-i7-3770)
pass -> FAIL (ro-ivb2-i7-3770)
pass -> FAIL (ro-byt-n2820)
pass -> FAIL (fi-skl-i7-6700k)
pass -> FAIL (ro-skl3-i5-6260u)
Subgroup nonblocking-crc-pipe-a-frame-sequence:
pass -> FAIL (ro-bdw-i7-5557U)
pass -> FAIL (ro-ilk1-i5-650)
pass -> FAIL (ro-hsw-i3-4010u)
pass -> FAIL (fi-skl-i5-6260u)
pass -> FAIL (ro-bdw-i7-5600u)
pass -> FAIL (ro-snb-i7-2620M)
pass -> FAIL (ro-bdw-i5-5250u)
pass -> FAIL (ro-hsw-i7-4770r)
pass -> FAIL (fi-hsw-i7-4770k)
pass -> FAIL (fi-snb-i7-2600)
pass -> FAIL (ro-ivb-i7-3770)
pass -> FAIL (ro-ivb2-i7-3770)
pass -> FAIL (ro-byt-n2820)
pass -> FAIL (fi-skl-i7-6700k)
pass -> FAIL (ro-skl3-i5-6260u)
Subgroup nonblocking-crc-pipe-b:
pass -> FAIL (ro-bdw-i7-5557U)
pass -> FAIL (ro-ilk1-i5-650)
pass -> FAIL (ro-hsw-i3-4010u)
WARNING: Long output truncated
Results at /archive/results/CI_IGT_test/RO_Patchwork_1673/
aa8628c drm-intel-nightly: 2016y-08m-02d-14h-10m-12s UTC integration manifest
be0abf9 drm/i915: Fix the return value of pipe crc read function.
e9fa366 drm/i915: Fix copy_to_user usage for pipe_crc
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc
2016-08-03 4:42 [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc Rodrigo Vivi
2016-08-03 4:42 ` [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function Rodrigo Vivi
2016-08-03 6:49 ` ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Fix copy_to_user usage for pipe_crc Patchwork
@ 2016-08-03 7:29 ` Ville Syrjälä
2016-08-03 7:44 ` Daniel Vetter
3 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2016-08-03 7:29 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
On Tue, Aug 02, 2016 at 09:42:06PM -0700, Rodrigo Vivi wrote:
> Copy to user return the number of bytes it couldn't write
> and zero on success. So any number different than 0 should
> be considered a fault, not only when it doesn't write
> the full size.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7c42ec4..7052c47 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3632,7 +3632,6 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
> while (n_entries > 0) {
> struct intel_pipe_crc_entry *entry =
> &pipe_crc->entries[pipe_crc->tail];
> - int ret;
>
> if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
> INTEL_PIPE_CRC_ENTRIES_NR) < 1)
> @@ -3649,8 +3648,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>
> spin_unlock_irq(&pipe_crc->lock);
>
> - ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
> - if (ret == PIPE_CRC_LINE_LEN)
> + if (!copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN))
A bad '!' snuck in.
> return -EFAULT;
>
> user_buf += PIPE_CRC_LINE_LEN;
> --
> 2.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function.
2016-08-03 4:42 ` [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function Rodrigo Vivi
@ 2016-08-03 7:31 ` Ville Syrjälä
2016-08-03 14:44 ` Vivi, Rodrigo
2016-08-03 7:48 ` Daniel Vetter
1 sibling, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2016-08-03 7:31 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
On Tue, Aug 02, 2016 at 09:42:07PM -0700, Rodrigo Vivi wrote:
> A read(fd, buf, len) function should return the number
> of bytes read. In our case we need to return the
> number of bytes we copy to user, instead of returning
> the number of bytes we read internally.
>
> It was really strange when I saw i-g-t test case using
> len '54' but getting '56' as return. First thought was
> how do we read more than we asked? But also I checked
> and there was really only 54. Until I realized it
> was all our fault. EFAULT!
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7052c47..b7b8d79 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3659,7 +3659,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>
> spin_unlock_irq(&pipe_crc->lock);
>
> - return bytes_read;
> + return PIPE_CRC_LINE_LEN;
Nope. We can read multiple entries in one go.
bytes_read += snprintf()
so it's mostly good. The only case where it fails if we don't have
enough space for the snprintf(), as snprintf() will then return the
number of bytes it would have written. That can actually happen on
account of the hex vs. decimal mess with the frame counter.
> }
>
> static const struct file_operations i915_pipe_crc_fops = {
> --
> 2.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc
2016-08-03 4:42 [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc Rodrigo Vivi
` (2 preceding siblings ...)
2016-08-03 7:29 ` [PATCH 1/2] " Ville Syrjälä
@ 2016-08-03 7:44 ` Daniel Vetter
3 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2016-08-03 7:44 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
On Tue, Aug 02, 2016 at 09:42:06PM -0700, Rodrigo Vivi wrote:
> Copy to user return the number of bytes it couldn't write
> and zero on success. So any number different than 0 should
> be considered a fault, not only when it doesn't write
> the full size.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
I audited all the other places we use copy_*_user. We have a bit a mess
between negative errno and unwritten bytes for our own copy functions, but
seems consistent (except this one here).
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7c42ec4..7052c47 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3632,7 +3632,6 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
> while (n_entries > 0) {
> struct intel_pipe_crc_entry *entry =
> &pipe_crc->entries[pipe_crc->tail];
> - int ret;
>
> if (CIRC_CNT(pipe_crc->head, pipe_crc->tail,
> INTEL_PIPE_CRC_ENTRIES_NR) < 1)
> @@ -3649,8 +3648,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>
> spin_unlock_irq(&pipe_crc->lock);
>
> - ret = copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN);
> - if (ret == PIPE_CRC_LINE_LEN)
> + if (!copy_to_user(user_buf, buf, PIPE_CRC_LINE_LEN))
> return -EFAULT;
>
> user_buf += PIPE_CRC_LINE_LEN;
> --
> 2.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function.
2016-08-03 4:42 ` [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function Rodrigo Vivi
2016-08-03 7:31 ` Ville Syrjälä
@ 2016-08-03 7:48 ` Daniel Vetter
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2016-08-03 7:48 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
On Tue, Aug 02, 2016 at 09:42:07PM -0700, Rodrigo Vivi wrote:
> A read(fd, buf, len) function should return the number
> of bytes read. In our case we need to return the
> number of bytes we copy to user, instead of returning
> the number of bytes we read internally.
>
> It was really strange when I saw i-g-t test case using
> len '54' but getting '56' as return. First thought was
> how do we read more than we asked? But also I checked
> and there was really only 54. Until I realized it
> was all our fault. EFAULT!
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7052c47..b7b8d79 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3659,7 +3659,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>
> spin_unlock_irq(&pipe_crc->lock);
>
> - return bytes_read;
> + return PIPE_CRC_LINE_LEN;
This will no loose entries if you have more than 1 crc ready to be read. I
think better to WARN_ON if snprintf doesn't give us the expected length,
and then correct bytes_read to match.
-Daniel
> }
>
> static const struct file_operations i915_pipe_crc_fops = {
> --
> 2.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function.
2016-08-03 7:31 ` Ville Syrjälä
@ 2016-08-03 14:44 ` Vivi, Rodrigo
2016-08-05 21:50 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 9+ messages in thread
From: Vivi, Rodrigo @ 2016-08-03 14:44 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
On Wed, 2016-08-03 at 10:31 +0300, Ville Syrjälä wrote:
> On Tue, Aug 02, 2016 at 09:42:07PM -0700, Rodrigo Vivi wrote:
> >
> > A read(fd, buf, len) function should return the number
> > of bytes read. In our case we need to return the
> > number of bytes we copy to user, instead of returning
> > the number of bytes we read internally.
> >
> > It was really strange when I saw i-g-t test case using
> > len '54' but getting '56' as return. First thought was
> > how do we read more than we asked? But also I checked
> > and there was really only 54. Until I realized it
> > was all our fault. EFAULT!
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 7052c47..b7b8d79 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3659,7 +3659,7 @@ i915_pipe_crc_read(struct file *filep, char
> > __user *user_buf, size_t count,
> >
> > spin_unlock_irq(&pipe_crc->lock);
> >
> > - return bytes_read;
> > + return PIPE_CRC_LINE_LEN;
> Nope. We can read multiple entries in one go.
>
> bytes_read += snprintf()
>
> so it's mostly good. The only case where it fails if we don't have
> enough space for the snprintf(), as snprintf() will then return the
> number of bytes it would have written. That can actually happen on
> account of the hex vs. decimal mess with the frame counter.
Oh, I see.
So what about:
if (bytes_read > count)
return -E-something?
>
> >
> > }
> >
> > static const struct file_operations i915_pipe_crc_fops = {
> > --
> > 2.5.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function.
2016-08-03 14:44 ` Vivi, Rodrigo
@ 2016-08-05 21:50 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 9+ messages in thread
From: Pandiyan, Dhinakaran @ 2016-08-05 21:50 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx
On Wed, 2016-08-03 at 14:44 +0000, Vivi, Rodrigo wrote:
> On Wed, 2016-08-03 at 10:31 +0300, Ville Syrjälä wrote:
> > On Tue, Aug 02, 2016 at 09:42:07PM -0700, Rodrigo Vivi wrote:
> > >
> > > A read(fd, buf, len) function should return the number
> > > of bytes read. In our case we need to return the
> > > number of bytes we copy to user, instead of returning
> > > the number of bytes we read internally.
> > >
> > > It was really strange when I saw i-g-t test case using
> > > len '54' but getting '56' as return. First thought was
> > > how do we read more than we asked? But also I checked
> > > and there was really only 54. Until I realized it
> > > was all our fault. EFAULT!
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 7052c47..b7b8d79 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -3659,7 +3659,7 @@ i915_pipe_crc_read(struct file *filep, char
> > > __user *user_buf, size_t count,
> > >
> > > spin_unlock_irq(&pipe_crc->lock);
> > >
> > > - return bytes_read;
> > > + return PIPE_CRC_LINE_LEN;
> > Nope. We can read multiple entries in one go.
> >
> > bytes_read += snprintf()
> >
> > so it's mostly good. The only case where it fails if we don't have
> > enough space for the snprintf(), as snprintf() will then return the
> > number of bytes it would have written. That can actually happen on
> > account of the hex vs. decimal mess with the frame counter.
>
> Oh, I see.
>
> So what about:
> if (bytes_read > count)
> return -E-something?
We could just return from snprintf() when it does not return
PIPE_CRC_LINE_LEN instead of reading all entries.
Anyway, shouldn't we fix hex to decimal conv.? Or is the value incorrect
when entry->frame in decimal exceeds 8 chars?
> >
> > >
> > > }
> > >
> > > static const struct file_operations i915_pipe_crc_fops = {
> > > --
> > > 2.5.5
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-08-05 21:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-03 4:42 [PATCH 1/2] drm/i915: Fix copy_to_user usage for pipe_crc Rodrigo Vivi
2016-08-03 4:42 ` [PATCH 2/2] drm/i915: Fix the return value of pipe crc read function Rodrigo Vivi
2016-08-03 7:31 ` Ville Syrjälä
2016-08-03 14:44 ` Vivi, Rodrigo
2016-08-05 21:50 ` Pandiyan, Dhinakaran
2016-08-03 7:48 ` Daniel Vetter
2016-08-03 6:49 ` ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Fix copy_to_user usage for pipe_crc Patchwork
2016-08-03 7:29 ` [PATCH 1/2] " Ville Syrjälä
2016-08-03 7:44 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.