On Mon, May 15, 2017 at 09:01:29AM +0200, Daniel Vetter wrote:

On Fri, May 12, 2017 at 07:55:42PM +0100, Chris Wilson wrote:

...

Constructing the name takes the majority of the time for allocating a sync_file to wrap a fence, and the name is very rarely used (only via the sync_file status user interface). To reduce the impact on the common path (that of creating sync_file to pass around), defer the construction of the name until it is first used.

v2: Update kerneldoc (kbuild test robot) v3: sync_debug.c was peeking at the name

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org

---

drivers/dma-buf/sync_debug.c | 3 ++- drivers/dma-buf/sync_file.c | 34 +++++++++++++++++++++++++++------- include/linux/sync_file.h | 5 +++-- 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 4b1731ee7458..9a93f1085c63 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -134,7 +134,8 @@ static void sync_print_sync_file(struct seq_file *s, { int i;

• seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,

• seq_printf(s, "[%p] %s: %s\n", sync_file,

•    sync_file_get_name(sync_file),
  

  sync_status_str(dma_fence_get_status(sync_file->fence)));

  if (dma_fence_is_array(sync_file->fence)) {

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index c9eb4997cfcc..d105079ec45c 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)

sync_file->fence = dma_fence_get(fence);

• snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",

•  fence->ops->get_driver_name(fence),
  

•  fence->ops->get_timeline_name(fence), fence->context,
  

•  fence->seqno);
  

• return sync_file;

} EXPORT_SYMBOL(sync_file_create); @@ -129,6 +124,31 @@ struct dma_fence *sync_file_get_fence(int fd) } EXPORT_SYMBOL(sync_file_get_fence);

+/**

• • sync_file_get_name - get the name of the sync_file
• • @sync_file: sync_file to get the fence from
• • Each sync_file may have a name assigned either by the user (when merging
• • sync_files together) or created from the fence it contains. However,
• • construction of the name is deferred until first use.
• • Returns: a string representing the name
• */

+char *sync_file_get_name(struct sync_file *sync_file) +{

• if (!sync_file->user_name[0]) {

• scnprintf(sync_file->user_name,
  

• 	  sizeof(sync_file->user_name),
  

• 	  "%s-%s%llu-%d",
  

• 	  sync_file->fence->ops->get_driver_name(sync_file->fence),
  

• 	  sync_file->fence->ops->get_timeline_name(sync_file->fence),
  

• 	  sync_file->fence->context,
  

• 	  sync_file->fence->seqno);
  

• }

This is mildly race. Do we care? Deserves at least a comment that we don't care, with that ack: me.

Hopefully, we don't care. It's not even a given that the fence->ops return the same names at different times. The only sore point is that this is an ioctl, so not the usual debugfs handwave.

We can cut down the race somewhat

if (!name[0]) { char buf[sizeof(sync_file->user_name)]; struct dma_fence *fence; int len;

rcu_read_lock(); fence = sync_file->fence; len = scnprintf(buf, sizeof(buf), "%s-%s%llu-%d", fence->ops->get_driver_name(sync_file->fence), fence->ops->get_timeline_name(sync_file->fence), fence->context, fence->seqno); rcu_read_unlock();

spin_lock(&global_sync_file_name_lock); if (!sync_file->user_name[0]) memcpy(sync_file->user_name[0], buf, len + 1); spin_unlock(&global_sync_file_name_lock); }

Even doing the staging to a local before doing a memcpy will be enough to trim the race to a few cycles -- and leave it under the rcu_read_unlock to keep preempt disabled. -Chris

Hey

On Mon, May 15, 2017 at 12:10 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

Constructing the name takes the majority of the time for allocating a sync_file to wrap a fence, and the name is very rarely used (only via the sync_file status user interface). To reduce the impact on the common path (that of creating sync_file to pass around), defer the construction of the name until it is first used.

v2: Update kerneldoc (kbuild test robot) v3: sync_debug.c was peeking at the name v4: Comment upon the potential race between two users of sync_file_get_name() and claim that such a race is below the level of notice. However, to prevent any future nuisance, use a global spinlock to serialize the assignment of the name.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/dma-buf/sync_debug.c | 3 ++- drivers/dma-buf/sync_file.c | 54 ++++++++++++++++++++++++++++++++++++++------ include/linux/sync_file.h | 5 ++-- 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 4b1731ee7458..9a93f1085c63 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -134,7 +134,8 @@ static void sync_print_sync_file(struct seq_file *s, { int i;

•   seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
  

•   seq_printf(s, "[%p] %s: %s\n", sync_file,
  

•              sync_file_get_name(sync_file),
               sync_status_str(dma_fence_get_status(sync_file->fence)));
  
    if (dma_fence_is_array(sync_file->fence)) {
  

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index c9eb4997cfcc..0d54de79ba76 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)

    sync_file->fence = dma_fence_get(fence);

•   snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
  

•            fence->ops->get_driver_name(fence),
  

•            fence->ops->get_timeline_name(fence), fence->context,
  

•            fence->seqno);
  

•   return sync_file;
  

} EXPORT_SYMBOL(sync_file_create); @@ -129,6 +124,51 @@ struct dma_fence *sync_file_get_fence(int fd) } EXPORT_SYMBOL(sync_file_get_fence);

+/**

• • sync_file_get_name - get the name of the sync_file
• • @sync_file: sync_file to get the fence from
• • Each sync_file may have a name assigned either by the user (when merging
• • sync_files together) or created from the fence it contains. However,
• • construction of the name is deferred until first use.
• • Returns: a string representing the name
• */

+char *sync_file_get_name(struct sync_file *sync_file) +{

•   static DEFINE_SPINLOCK(name_lock);
  

•   if (!sync_file->user_name[0]) {
  

•           char buf[sizeof(sync_file->user_name)];
  

•           struct dma_fence *fence;
  

•           int len;
  

•           rcu_read_lock();
  

•           fence = sync_file->fence;
  

•           len = scnprintf(buf, sizeof(buf), "%s-%s%llu-%d",
  

•                           fence->ops->get_driver_name(sync_file->fence),
  

•                           fence->ops->get_timeline_name(sync_file->fence),
  

•                           fence->context,
  

•                           fence->seqno);
  

•           /* This is mildly racy, so we stage the printf into a local
  

•            * buffer and do the copy with preempt disabled to trim the
  

•            * race down to a few cycles. To completely eliminate the
  

•            * race, we do the final assignment underneath a global
  

•            * spinlock. A global lock should be adequate here for this
  

•            * rare path (called from the middle of a user sw_sync status
  

•            * ioctl and from debugfs).
  

•            */
  

•           spin_lock(&name_lock);
  

•           if (!sync_file->user_name[0])
  

•                   memcpy(sync_file->user_name, buf, len + 1);
  

A racing reader might get a truncated string here. Note that a racing reader does not take the lock as soon as the first byte is visible. Not sure what the name is used for, but with that code it is not reliable.

Also, please zero-terminate the string... scnprintf() does not guarantee that, if it follows posix.

•           spin_unlock(&name_lock);
  

•           rcu_read_unlock();
  

•   }
  

•   return sync_file->user_name;
  

+}

static int sync_file_set_fence(struct sync_file *sync_file, struct dma_fence **fences, int num_fences) { @@ -266,7 +306,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, goto err; }

•   strlcpy(sync_file->name, name, sizeof(sync_file->name));
  

•   strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name));
    return sync_file;
  
  

err: @@ -419,7 +459,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, }

no_fences:

•   strlcpy(info.name, sync_file->name, sizeof(info.name));
  

•   strlcpy(info.name, sync_file_get_name(sync_file), sizeof(info.name));
  

Don't use strlcpy(). It requires your source to be 0-terminated. Which is not the case for sync_file->user_name. Either make sure user_name is 0-terminated, or use strcpy().

Thanks David

    info.status = dma_fence_is_signaled(sync_file->fence);
    info.num_fences = num_fences;

diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h index d37beefdfbd5..0f3f05325c84 100644 --- a/include/linux/sync_file.h +++ b/include/linux/sync_file.h @@ -23,7 +23,7 @@ /**

• struct sync_file - sync file to export to the userspace
• @file: file representing this fence

• • @name: name of sync_file. Useful for debugging

• • @user_name: name of sync_file. Useful for debugging
  • @sync_file_list: membership in global file list
  • @wq: wait queue for fence signaling
  • @fence: fence with the fences in the sync_file

@@ -31,7 +31,7 @@ */ struct sync_file { struct file *file;

•   char                    name[32];
  

•   char                    user_name[32];
  

#ifdef CONFIG_DEBUG_FS struct list_head sync_file_list; #endif @@ -46,5 +46,6 @@ struct sync_file {

struct sync_file *sync_file_create(struct dma_fence *fence); struct dma_fence *sync_file_get_fence(int fd); +char *sync_file_get_name(struct sync_file *sync_file);

#endif /* _LINUX_SYNC_H */

2.11.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hey

On Mon, May 15, 2017 at 1:42 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

On Mon, May 15, 2017 at 12:50:04PM +0200, David Herrmann wrote:

...

Hey

On Mon, May 15, 2017 at 12:10 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

...

Constructing the name takes the majority of the time for allocating a sync_file to wrap a fence, and the name is very rarely used (only via the sync_file status user interface). To reduce the impact on the common path (that of creating sync_file to pass around), defer the construction of the name until it is first used.

v2: Update kerneldoc (kbuild test robot) v3: sync_debug.c was peeking at the name v4: Comment upon the potential race between two users of sync_file_get_name() and claim that such a race is below the level of notice. However, to prevent any future nuisance, use a global spinlock to serialize the assignment of the name.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/dma-buf/sync_debug.c | 3 ++- drivers/dma-buf/sync_file.c | 54 ++++++++++++++++++++++++++++++++++++++------ include/linux/sync_file.h | 5 ++-- 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 4b1731ee7458..9a93f1085c63 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -134,7 +134,8 @@ static void sync_print_sync_file(struct seq_file *s, { int i;

•   seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
  

•   seq_printf(s, "[%p] %s: %s\n", sync_file,
  

•              sync_file_get_name(sync_file),
               sync_status_str(dma_fence_get_status(sync_file->fence)));
  
    if (dma_fence_is_array(sync_file->fence)) {
  

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index c9eb4997cfcc..0d54de79ba76 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)

    sync_file->fence = dma_fence_get(fence);

•   snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
  

•            fence->ops->get_driver_name(fence),
  

•            fence->ops->get_timeline_name(fence), fence->context,
  

•            fence->seqno);
  

•   return sync_file;
  

} EXPORT_SYMBOL(sync_file_create); @@ -129,6 +124,51 @@ struct dma_fence *sync_file_get_fence(int fd) } EXPORT_SYMBOL(sync_file_get_fence);

+/**

• • sync_file_get_name - get the name of the sync_file
• • @sync_file: sync_file to get the fence from
• • Each sync_file may have a name assigned either by the user (when merging
• • sync_files together) or created from the fence it contains. However,
• • construction of the name is deferred until first use.
• • Returns: a string representing the name
• */

+char *sync_file_get_name(struct sync_file *sync_file) +{

•   static DEFINE_SPINLOCK(name_lock);
  

•   if (!sync_file->user_name[0]) {
  

•           char buf[sizeof(sync_file->user_name)];
  

•           struct dma_fence *fence;
  

•           int len;
  

•           rcu_read_lock();
  

•           fence = sync_file->fence;
  

•           len = scnprintf(buf, sizeof(buf), "%s-%s%llu-%d",
  

•                           fence->ops->get_driver_name(sync_file->fence),
  

•                           fence->ops->get_timeline_name(sync_file->fence),
  

•                           fence->context,
  

•                           fence->seqno);
  

•           /* This is mildly racy, so we stage the printf into a local
  

•            * buffer and do the copy with preempt disabled to trim the
  

•            * race down to a few cycles. To completely eliminate the
  

•            * race, we do the final assignment underneath a global
  

•            * spinlock. A global lock should be adequate here for this
  

•            * rare path (called from the middle of a user sw_sync status
  

•            * ioctl and from debugfs).
  

•            */
  

•           spin_lock(&name_lock);
  

•           if (!sync_file->user_name[0])
  

•                   memcpy(sync_file->user_name, buf, len + 1);
  

A racing reader might get a truncated string here. Note that a racing reader does not take the lock as soon as the first byte is visible. Not sure what the name is used for, but with that code it is not reliable.

The comment does explain that the use case is purely debug. We may as well forget the spinlock. It is always a valid string, if incomplete, and the race is fairly immaterial. For the value of the name, I'd be quite happy to ignore it entirely and have it only set when the user provides one.

The comment does not make it clear to me which part is protected here. Whether it is just about racing writers, or whether it actually also protects against racing readers. The lock gives the impression that it protects only against racing writers, leaving open the issue with racing readers. If that's not the case, I don't see the point of the lock, given that racing writers would actually write the same string, anyway.

...

Also, please zero-terminate the string... scnprintf() does not guarantee that, if it follows posix.

But the kernel vs[c]nprintf does ensure that the string is NUL-terminated.

You're right, the kernel does indeed differ from posix here. Sorry for the noise.

Thanks David

On Tue, May 16, 2017 at 12:10:42PM +0100, Chris Wilson wrote:

Constructing the name takes the majority of the time for allocating a sync_file to wrap a fence, and the name is very rarely used (only via the sync_file status user interface). To reduce the impact on the common path (that of creating sync_file to pass around), defer the construction of the name until it is first used.

v2: Update kerneldoc (kbuild test robot) v3: sync_debug.c was peeking at the name v4: Comment upon the potential race between two users of sync_file_get_name() and claim that such a race is below the level of notice. However, to prevent any future nuisance, use a global spinlock to serialize the assignment of the name. v5: Completely avoid the read/write race by only storing the name passed in from the user inside sync_file->user_name and passing in a buffer to dynamically construct the name otherwise.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Herrmann dh.herrmann@gmail.com

Any thoughts on this one? It should completely avoid the race, with the presumption that unless the user sets a name when merging the best response is to show the fence name.

It may be interesting to actually show both; the encoded sync_file->fence name and the user's identifier. -Chris

---

drivers/dma-buf/sync_debug.c | 4 +++- drivers/dma-buf/sync_file.c | 39 ++++++++++++++++++++++++++++++++------- include/linux/sync_file.h | 5 +++-- 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 4b1731ee7458..59a3b2f8ee91 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -132,9 +132,11 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) static void sync_print_sync_file(struct seq_file *s, struct sync_file *sync_file) {

• char buf[128]; int i;

• seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,

• seq_printf(s, "[%p] %s: %s\n", sync_file,

•    sync_file_get_name(sync_file, buf, sizeof(buf)),
  

  sync_status_str(dma_fence_get_status(sync_file->fence)));

  if (dma_fence_is_array(sync_file->fence)) {

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index c9eb4997cfcc..d7e219d2669d 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)

sync_file->fence = dma_fence_get(fence);

• snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",

•  fence->ops->get_driver_name(fence),
  

•  fence->ops->get_timeline_name(fence), fence->context,
  

•  fence->seqno);
  

• return sync_file;

} EXPORT_SYMBOL(sync_file_create); @@ -129,6 +124,36 @@ struct dma_fence *sync_file_get_fence(int fd) } EXPORT_SYMBOL(sync_file_get_fence);

+/**

• • sync_file_get_name - get the name of the sync_file
• • @sync_file: sync_file to get the fence from
• • @buf: destination buffer to copy sync_file name into
• • @len: available size of destination buffer.
• • Each sync_file may have a name assigned either by the user (when merging
• • sync_files together) or created from the fence it contains. In the latter
• • case construction of the name is deferred until use, and so requires
• • sync_file_get_name().
• • Returns: a string representing the name.
• */

+char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) +{

• if (sync_file->user_name[0]) {

• strlcpy(buf, sync_file->user_name, len);
  

• } else {

• struct dma_fence *fence = sync_file->fence;
  

• snprintf(buf, len, "%s-%s%llu-%d",
  

• 	 fence->ops->get_driver_name(fence),
  

• 	 fence->ops->get_timeline_name(fence),
  

• 	 fence->context,
  

• 	 fence->seqno);
  

• }
• return buf;

+}

static int sync_file_set_fence(struct sync_file *sync_file, struct dma_fence **fences, int num_fences) { @@ -266,7 +291,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, goto err; }

• strlcpy(sync_file->name, name, sizeof(sync_file->name));

• strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); return sync_file;

err: @@ -419,7 +444,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, }

no_fences:

• strlcpy(info.name, sync_file->name, sizeof(info.name));

• sync_file_get_name(sync_file, info.name, sizeof(info.name)); info.status = dma_fence_is_signaled(sync_file->fence); info.num_fences = num_fences;

diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h index d37beefdfbd5..5226d62ae91b 100644 --- a/include/linux/sync_file.h +++ b/include/linux/sync_file.h @@ -23,7 +23,7 @@ /**

• struct sync_file - sync file to export to the userspace
• @file: file representing this fence

• • @name: name of sync_file. Useful for debugging

• • @user_name: name of sync_file. Useful for debugging
  • @sync_file_list: membership in global file list
  • @wq: wait queue for fence signaling
  • @fence: fence with the fences in the sync_file

@@ -31,7 +31,7 @@ */ struct sync_file { struct file *file;

• char name[32];

• char user_name[32];

#ifdef CONFIG_DEBUG_FS struct list_head sync_file_list; #endif @@ -46,5 +46,6 @@ struct sync_file {

struct sync_file *sync_file_create(struct dma_fence *fence); struct dma_fence *sync_file_get_fence(int fd); +char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len);

#endif /* _LINUX_SYNC_H */

2.11.0

2017-05-22 Daniel Vetter daniel@ffwll.ch:

On Tue, May 16, 2017 at 12:10:42PM +0100, Chris Wilson wrote:

...

Constructing the name takes the majority of the time for allocating a sync_file to wrap a fence, and the name is very rarely used (only via the sync_file status user interface). To reduce the impact on the common path (that of creating sync_file to pass around), defer the construction of the name until it is first used.

v2: Update kerneldoc (kbuild test robot) v3: sync_debug.c was peeking at the name v4: Comment upon the potential race between two users of sync_file_get_name() and claim that such a race is below the level of notice. However, to prevent any future nuisance, use a global spinlock to serialize the assignment of the name. v5: Completely avoid the read/write race by only storing the name passed in from the user inside sync_file->user_name and passing in a buffer to dynamically construct the name otherwise.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Herrmann dh.herrmann@gmail.com

---

drivers/dma-buf/sync_debug.c | 4 +++- drivers/dma-buf/sync_file.c | 39 ++++++++++++++++++++++++++++++++------- include/linux/sync_file.h | 5 +++-- 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 4b1731ee7458..59a3b2f8ee91 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -132,9 +132,11 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) static void sync_print_sync_file(struct seq_file *s, struct sync_file *sync_file) {

• char buf[128]; int i;

• seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,

• seq_printf(s, "[%p] %s: %s\n", sync_file,

•    sync_file_get_name(sync_file, buf, sizeof(buf)),
  

  sync_status_str(dma_fence_get_status(sync_file->fence)));

  if (dma_fence_is_array(sync_file->fence)) {

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index c9eb4997cfcc..d7e219d2669d 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)

sync_file->fence = dma_fence_get(fence);

• snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",

•  fence->ops->get_driver_name(fence),
  

•  fence->ops->get_timeline_name(fence), fence->context,
  

•  fence->seqno);
  

• return sync_file;

} EXPORT_SYMBOL(sync_file_create); @@ -129,6 +124,36 @@ struct dma_fence *sync_file_get_fence(int fd) } EXPORT_SYMBOL(sync_file_get_fence);

+/**

• • sync_file_get_name - get the name of the sync_file
• • @sync_file: sync_file to get the fence from
• • @buf: destination buffer to copy sync_file name into
• • @len: available size of destination buffer.
• • Each sync_file may have a name assigned either by the user (when merging
• • sync_files together) or created from the fence it contains. In the latter
• • case construction of the name is deferred until use, and so requires
• • sync_file_get_name().
• • Returns: a string representing the name.
• */

+char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) +{

• if (sync_file->user_name[0]) {

• strlcpy(buf, sync_file->user_name, len);
  

• } else {

• struct dma_fence *fence = sync_file->fence;
  

• snprintf(buf, len, "%s-%s%llu-%d",
  

• 	 fence->ops->get_driver_name(fence),
  

• 	 fence->ops->get_timeline_name(fence),
  

• 	 fence->context,
  

• 	 fence->seqno);
  

• }
• return buf;

+}

static int sync_file_set_fence(struct sync_file *sync_file, struct dma_fence **fences, int num_fences) { @@ -266,7 +291,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, goto err; }

• strlcpy(sync_file->name, name, sizeof(sync_file->name));

• strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); return sync_file;

err: @@ -419,7 +444,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, }

no_fences:

• strlcpy(info.name, sync_file->name, sizeof(info.name));

• sync_file_get_name(sync_file, info.name, sizeof(info.name)); info.status = dma_fence_is_signaled(sync_file->fence); info.num_fences = num_fences;

diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h index d37beefdfbd5..5226d62ae91b 100644 --- a/include/linux/sync_file.h +++ b/include/linux/sync_file.h @@ -23,7 +23,7 @@ /**

• struct sync_file - sync file to export to the userspace
• @file: file representing this fence

• • @name: name of sync_file. Useful for debugging

• • @user_name: name of sync_file. Useful for debugging
  • @sync_file_list: membership in global file list
  • @wq: wait queue for fence signaling
  • @fence: fence with the fences in the sync_file

@@ -31,7 +31,7 @@ */ struct sync_file { struct file *file;

• char name[32];

Maybe inline kerneldoc an explain a bit better what's going on:

/** * @user_name: * * Name of the sync file provided by userspace, for merged fences. * Otherwise generated through driver callbacks (in which case the * entire array is 0). */

Might be overkill, so not going to insist at all. Either way:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

I fixed it. Applied with the modification proposed by Daniel. Thanks for the patch.

Gustavo