dma_fence's backend_ops get_driver_name() and get_timeline_name() have an unnecessary and complicated RCU requirement.
Usage in sync_file, however, already shows how it can be done significantly simpler: Copy the name strings to the user. All that needs to be ensured for that is that the signaled-state is checked and that it cannot change while the callback runs, i.e., the lock must be held. Adjust the two callbacks so that they deep-copy name parameters under lock-protection. Signed-off-by: Philipp Stanner <[email protected]> --- drivers/dma-buf/dma-fence.c | 73 ++++++++++----------- drivers/dma-buf/sync_file.c | 29 ++++---- drivers/gpu/drm/i915/gt/intel_gt_requests.c | 16 ++--- drivers/gpu/drm/i915/i915_request.c | 9 ++- drivers/gpu/drm/i915/i915_sw_fence.c | 15 ++--- include/linux/dma-fence.h | 22 ++----- include/trace/events/dma_fence.h | 12 ++-- 7 files changed, 77 insertions(+), 99 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index e4350bce9cf9..d473d4ea601a 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -555,23 +555,22 @@ void dma_fence_release(struct kref *kref) struct dma_fence *fence = container_of(kref, struct dma_fence, refcount); const struct dma_fence_ops *ops = fence->ops; + const unsigned short len = 64; + char timeline[len]; + char driver[len]; - rcu_read_lock(); trace_dma_fence_destroy(fence); if (!list_empty(&fence->cb_list) && !dma_fence_test_signaled_flag(fence)) { - const char __rcu *timeline; - const char __rcu *driver; unsigned long flags; - driver = dma_fence_driver_name(fence); - timeline = dma_fence_timeline_name(fence); + dma_fence_driver_name(fence, driver, len); + dma_fence_timeline_name(fence, timeline, len); WARN(1, "Fence %s:%s:%llx:%llx released with pending signals!\n", - rcu_dereference(driver), rcu_dereference(timeline), - fence->context, fence->seqno); + driver, timeline, fence->context, fence->seqno); /* * Failed to signal before release, likely a refcounting issue. @@ -590,7 +589,6 @@ void dma_fence_release(struct kref *kref) ops->release(fence); else dma_fence_free(fence); - rcu_read_unlock(); } EXPORT_SYMBOL(dma_fence_release); @@ -1017,23 +1015,20 @@ EXPORT_SYMBOL(dma_fence_set_deadline); */ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) { - const char __rcu *timeline = (const char __rcu *)""; - const char __rcu *driver = (const char __rcu *)""; - const char *signaled = ""; - - rcu_read_lock(); + const unsigned short len = 64; + char timeline[len]; + char driver[len]; + char *signaled; if (!dma_fence_is_signaled(fence)) { - timeline = dma_fence_timeline_name(fence); - driver = dma_fence_driver_name(fence); + dma_fence_timeline_name(fence, timeline, len); + dma_fence_driver_name(fence, driver, len); signaled = "un"; } seq_printf(seq, "%llu:%llu %s %s %ssignalled\n", fence->context, fence->seqno, timeline, driver, signaled); - - rcu_read_unlock(); } EXPORT_SYMBOL(dma_fence_describe); @@ -1129,20 +1124,22 @@ EXPORT_SYMBOL(dma_fence_init64); * memory potentially accessed by the &dma_fence.ops functions, is forbidden * after the fence has been signalled. Drivers are allowed to free that data, * and some do. - * - * To allow safe access drivers are mandated to guarantee a RCU grace period - * between signalling the fence and freeing said data. - * - * As such access to the driver name is only valid inside a RCU locked section. - * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded - * by the &rcu_read_lock and &rcu_read_unlock pair. */ -const char __rcu *dma_fence_driver_name(struct dma_fence *fence) +void dma_fence_driver_name(struct dma_fence *fence, char *s, unsigned short len) { + unsigned long flags; + const char *tmp; + + dma_fence_lock_irqsave(fence, flags); + if (!dma_fence_test_signaled_flag(fence)) - return (const char __rcu *)fence->ops->get_driver_name(fence); + tmp = fence->ops->get_driver_name(fence); else - return (const char __rcu *)"detached-driver"; + tmp = "detached-driver"; + + strscpy(s, tmp, len); + + dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_driver_name); @@ -1158,19 +1155,21 @@ EXPORT_SYMBOL(dma_fence_driver_name); * memory potentially accessed by the &dma_fence.ops functions, is forbidden * after the fence has been signalled. Drivers are allowed to free that data, * and some do. - * - * To allow safe access drivers are mandated to guarantee a RCU grace period - * between signalling the fence and freeing said data. - * - * As such access to the driver name is only valid inside a RCU locked section. - * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded - * by the &rcu_read_lock and &rcu_read_unlock pair. */ -const char __rcu *dma_fence_timeline_name(struct dma_fence *fence) +void dma_fence_timeline_name(struct dma_fence *fence, char *s, unsigned short len) { + unsigned long flags; + const char *tmp; + + dma_fence_lock_irqsave(fence, flags); + if (!dma_fence_test_signaled_flag(fence)) - return (const char __rcu *)fence->ops->get_timeline_name(fence); + tmp = fence->ops->get_timeline_name(fence); else - return (const char __rcu *)"signaled-timeline"; + tmp = "signaled-timeline"; + + strscpy(s, tmp, len); + + dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_timeline_name); diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 2166bbdf7e4a..0f08713b3b5e 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -18,6 +18,8 @@ #include <linux/sync_file.h> #include <uapi/linux/sync_file.h> +#define FENCE_NAME_LEN 64 + static const struct file_operations sync_file_fops; static struct sync_file *sync_file_alloc(void) @@ -131,22 +133,21 @@ EXPORT_SYMBOL(sync_file_get_fence); */ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) { + char timeline[FENCE_NAME_LEN]; + char driver[FENCE_NAME_LEN]; + if (sync_file->user_name[0]) { strscpy(buf, sync_file->user_name, len); } else { struct dma_fence *fence = sync_file->fence; - const char __rcu *timeline; - const char __rcu *driver; - rcu_read_lock(); - driver = dma_fence_driver_name(fence); - timeline = dma_fence_timeline_name(fence); + dma_fence_driver_name(fence, driver, FENCE_NAME_LEN); + dma_fence_timeline_name(fence, timeline, FENCE_NAME_LEN); snprintf(buf, len, "%s-%s%llu-%lld", - rcu_dereference(driver), - rcu_dereference(timeline), + driver, + timeline, fence->context, fence->seqno); - rcu_read_unlock(); } return buf; @@ -268,19 +269,11 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file, static int sync_fill_fence_info(struct dma_fence *fence, struct sync_fence_info *info) { - const char __rcu *timeline; - const char __rcu *driver; + dma_fence_timeline_name(fence, info->obj_name, sizeof(info->obj_name)); + dma_fence_driver_name(fence, info->driver_name, sizeof(info->driver_name)); rcu_read_lock(); - driver = dma_fence_driver_name(fence); - timeline = dma_fence_timeline_name(fence); - - strscpy(info->obj_name, rcu_dereference(timeline), - sizeof(info->obj_name)); - strscpy(info->driver_name, rcu_dereference(driver), - sizeof(info->driver_name)); - info->status = dma_fence_get_status(fence); info->timestamp_ns = dma_fence_is_signaled(fence) ? diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c index 93298820bee2..a37069ca6d2a 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c @@ -240,8 +240,12 @@ void intel_gt_watchdog_work(struct work_struct *work) { struct intel_gt *gt = container_of(work, typeof(*gt), watchdog.work); + const unsigned short len = 64; struct i915_request *rq, *rn; struct llist_node *first; + char timeline[len]; + char driver[len]; + first = llist_del_all(>->watchdog.list); if (!first) @@ -250,17 +254,13 @@ void intel_gt_watchdog_work(struct work_struct *work) llist_for_each_entry_safe(rq, rn, first, watchdog.link) { if (!i915_request_completed(rq)) { struct dma_fence *f = &rq->fence; - const char __rcu *timeline; - const char __rcu *driver; - rcu_read_lock(); - driver = dma_fence_driver_name(f); - timeline = dma_fence_timeline_name(f); + dma_fence_driver_name(f, driver, len); + dma_fence_timeline_name(f, timeline, len); pr_notice("Fence expiration time out i915-%s:%s:%llx!\n", - rcu_dereference(driver), - rcu_dereference(timeline), + driver, + timeline, f->seqno); - rcu_read_unlock(); i915_request_cancel(rq, -EINTR); } i915_request_put(rq); diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index d2c7b1090df0..c81902d8a89a 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -2185,7 +2185,8 @@ void i915_request_show(struct drm_printer *m, const char *prefix, int indent) { - const char __rcu *timeline; + const unsigned short len = 64; + char timeline[len]; char buf[80] = ""; int x = 0; @@ -2221,8 +2222,7 @@ void i915_request_show(struct drm_printer *m, x = print_sched_attr(&rq->sched.attr, buf, x, sizeof(buf)); - rcu_read_lock(); - timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence); + dma_fence_timeline_name((struct dma_fence *)&rq->fence, timeline, len); drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n", prefix, indent, " ", queue_status(rq), @@ -2231,8 +2231,7 @@ void i915_request_show(struct drm_printer *m, fence_status(rq), buf, jiffies_to_msecs(jiffies - rq->emitted_jiffies), - rcu_dereference(timeline)); - rcu_read_unlock(); + timeline); } static bool engine_match_ring(struct intel_engine_cs *engine, struct i915_request *rq) diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index f24f616e23ee..08b5f96ae2a5 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -429,23 +429,22 @@ static void timer_i915_sw_fence_wake(struct timer_list *t) { struct i915_sw_dma_fence_cb_timer *cb = timer_container_of(cb, t, timer); + const unsigned short len = 64; struct i915_sw_fence *fence; - const char __rcu *timeline; - const char __rcu *driver; + char timeline[len]; + char driver[len]; fence = xchg(&cb->base.fence, NULL); if (!fence) return; - rcu_read_lock(); - driver = dma_fence_driver_name(cb->dma); - timeline = dma_fence_timeline_name(cb->dma); + dma_fence_driver_name(cb->dma, driver, len); + dma_fence_timeline_name(cb->dma, timeline, len); pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n", - rcu_dereference(driver), - rcu_dereference(timeline), + driver, + timeline, cb->dma->seqno, i915_sw_fence_debug_hint(fence)); - rcu_read_unlock(); i915_sw_fence_set_error_once(fence, -ETIMEDOUT); i915_sw_fence_complete(fence); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 2223d75c5a49..08ecbc101f4c 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -459,25 +459,11 @@ void dma_fence_enable_signaling(struct dma_fence *fence); /** * DOC: Safe external access to driver provided object members * - * All data not stored directly in the dma-fence object, such as the - * &dma_fence.lock and memory potentially accessed by functions in the - * &dma_fence.ops table, MUST NOT be accessed after the fence has been signalled - * because after that point drivers are allowed to free it. - * - * All code accessing that data via the dma-fence API (or directly, which is - * discouraged), MUST make sure to contain the complete access within a - * &rcu_read_lock and &rcu_read_unlock pair. - * - * Some dma-fence API handles this automatically, while other, as for example - * &dma_fence_driver_name and &dma_fence_timeline_name, leave that - * responsibility to the caller. - * - * To enable this scheme to work drivers MUST ensure a RCU grace period elapses - * between signalling the fence and freeing the said data. - * + * Provides naming information strings about the fence if it is not yet signaled. + * Otherwise, provides default strings. */ -const char __rcu *dma_fence_driver_name(struct dma_fence *fence); -const char __rcu *dma_fence_timeline_name(struct dma_fence *fence); +void dma_fence_driver_name(struct dma_fence *fence, char *s, unsigned short len); +void dma_fence_timeline_name(struct dma_fence *fence, char *s, unsigned short len); /* * dma_fence_test_signaled_flag - Only check whether a fence is signaled yet. diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index 5b10a9e06fb4..f3b929b98cf4 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -9,6 +9,8 @@ struct dma_fence; +#define FENCE_NAME_LEN 64 + DECLARE_EVENT_CLASS(dma_fence, TP_PROTO(struct dma_fence *fence), @@ -16,21 +18,21 @@ DECLARE_EVENT_CLASS(dma_fence, TP_ARGS(fence), TP_STRUCT__entry( - __string(driver, dma_fence_driver_name(fence)) - __string(timeline, dma_fence_timeline_name(fence)) + __array(char, driver, FENCE_NAME_LEN) + __array(char, timeline, FENCE_NAME_LEN) __field(unsigned int, context) __field(unsigned int, seqno) ), TP_fast_assign( - __assign_str(driver); - __assign_str(timeline); + dma_fence_driver_name(fence, __entry->driver, FENCE_NAME_LEN); + dma_fence_timeline_name(fence, __entry->timeline, FENCE_NAME_LEN); __entry->context = fence->context; __entry->seqno = fence->seqno; ), TP_printk("driver=%s timeline=%s context=%u seqno=%u", - __get_str(driver), __get_str(timeline), __entry->context, + __entry->driver, __entry->timeline, __entry->context, __entry->seqno) ); -- 2.56.0
