Re: [RFC PATCH 2/2] drm/i915: Adjust to RCU-less fence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Leftover `rcu_dereference()` calls on local stack arrays after removing `rcu_read_lock()`, causing compile errors and lockdep warnings.
--

commit 19905da799945ee74ece58c7907776bf754a26d6
Author: Philipp Stanner <phasta@kernel.org>

drm/i915: Adjust to RCU-less fence

This commit adjusts the i915 driver to accommodate changes in dma_fence 
that allow for handling fences without relying on RCU functionality.

> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index 93298820bee2c..724c4359f6884 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -238,6 +238,8 @@ void intel_gt_fini_requests(struct intel_gt *gt)
>  
>  void intel_gt_watchdog_work(struct work_struct *work)
>  {
> +	char driver[64] = "";
> +	char timeline[64] = "";
>  	struct intel_gt *gt =
>  		container_of(work, typeof(*gt), watchdog.work);
>  	struct i915_request *rq, *rn;
> @@ -250,17 +252,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, 64);
> +			dma_fence_timeline_name(f, timeline, 64);
>  			pr_notice("Fence expiration time out i915-%s:%s:%llx!\n",
>  				  rcu_dereference(driver),
>  				  rcu_dereference(timeline),

[Severity: Low]
Does this cause a compilation error? The patch changes the driver and timeline
variables to be local stack arrays, but rcu_dereference() expects a scalar
pointer, not a character array.

Additionally, if it does compile, could this trigger a lockdep warning since 
rcu_read_lock() was removed from this function?

>  				  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 d2c7b1090df08..2a3df13217b94 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -2185,7 +2185,7 @@ void i915_request_show(struct drm_printer *m,
>  		       const char *prefix,
>  		       int indent)
>  {
> -	const char __rcu *timeline;
> +	char timeline[64] = "";
>  	char buf[80] = "";
>  	int x = 0;
>  
> @@ -2221,8 +2221,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, 64);
>  	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
>  		   prefix, indent, "                ",
>  		   queue_status(rq),
> @@ -2232,7 +2231,6 @@ void i915_request_show(struct drm_printer *m,
>  		   buf,
>  		   jiffies_to_msecs(jiffies - rq->emitted_jiffies),
>  		   rcu_dereference(timeline));

[Severity: Low]
Similar to intel_gt_watchdog_work() above, is it safe to keep the
rcu_dereference() call wrapped around the timeline character array here now
that it is stack allocated and rcu_read_lock() is no longer held?

> -	rcu_read_unlock();
>  }
>  
>  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 f24f616e23ee6..d0e733beff863 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -427,25 +427,23 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma,
>  
>  static void timer_i915_sw_fence_wake(struct timer_list *t)
>  {
> +	char driver[64] = "";
> +	char timeline[64] = "";
>  	struct i915_sw_dma_fence_cb_timer *cb = timer_container_of(cb, t,
>  								   timer);
>  	struct i915_sw_fence *fence;
> -	const char __rcu *timeline;
> -	const char __rcu *driver;
>  
>  	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, 64);
> +	dma_fence_timeline_name(cb->dma, timeline, 64);
>  	pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n",
>  		  rcu_dereference(driver),
>  		  rcu_dereference(timeline),

[Severity: Low]
Does this code also suffer from the same issue? The rcu_dereference() macro is 
still being applied to the driver and timeline stack arrays after 
rcu_read_lock() was removed.

>  		  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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908105926.1120378-2-phasta@kernel.org?part=2