| Message ID | 1470199327-5833-2-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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 = {
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(-)