| Message ID | 20250509153352.7187-5-tvrtko.ursulin@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Some (drm_sched_|dma_)fence lifetime issues | expand |
On 12/05/2025 09:19, Christian König wrote: > On 5/9/25 17:33, Tvrtko Ursulin wrote: >> With the goal of reducing the need for drivers to touch fence->ops, we >> add explicit flags for struct dma_fence_array and struct dma_fence_chain >> and make the respective helpers (dma_fence_is_array() and >> dma_fence_is_chain()) use them. >> >> This also allows us to remove the exported symbols for the respective >> operation tables. > > That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared. > > Since the array and chain are always build in that should be completely unproblematic for driver unload. You are right this is not strictly needed. Idea was just to reduce any access to ops as much as we can and this fell under that scope. Another benefit one could perhaps argue is two fewer EXPORT_SYMBOLs, which is perhaps a little bit cleaner design (less exporting of implementation details to the outside), but it is not a super strong argument. If we will not be going for this one then I would be taking 1/13 via drm-intel-gt-next. Regards, Tvrtko >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> --- >> drivers/dma-buf/dma-fence-array.c | 2 +- >> drivers/dma-buf/dma-fence-chain.c | 2 +- >> include/linux/dma-fence.h | 9 ++++----- >> 3 files changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c >> index 6657d4b30af9..daf444f5d228 100644 >> --- a/drivers/dma-buf/dma-fence-array.c >> +++ b/drivers/dma-buf/dma-fence-array.c >> @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = { >> .release = dma_fence_array_release, >> .set_deadline = dma_fence_array_set_deadline, >> }; >> -EXPORT_SYMBOL(dma_fence_array_ops); >> >> /** >> * dma_fence_array_alloc - Allocate a custom fence array >> @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array, >> spin_lock_init(&array->lock); >> dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, >> context, seqno); >> + __set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags); >> init_irq_work(&array->work, irq_dma_fence_array_work); >> >> atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); >> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c >> index a8a90acf4f34..f4abe41fb092 100644 >> --- a/drivers/dma-buf/dma-fence-chain.c >> +++ b/drivers/dma-buf/dma-fence-chain.c >> @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = { >> .release = dma_fence_chain_release, >> .set_deadline = dma_fence_chain_set_deadline, >> }; >> -EXPORT_SYMBOL(dma_fence_chain_ops); >> >> /** >> * dma_fence_chain_init - initialize a fence chain >> @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, >> >> dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, >> context, seqno); >> + __set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags); >> >> /* >> * Chaining dma_fence_chain container together is only allowed through >> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >> index ac6535716dbe..5bafd0a5f1f1 100644 >> --- a/include/linux/dma-fence.h >> +++ b/include/linux/dma-fence.h >> @@ -98,6 +98,8 @@ struct dma_fence { >> >> enum dma_fence_flag_bits { >> DMA_FENCE_FLAG_SEQNO64_BIT, >> + DMA_FENCE_FLAG_ARRAY_BIT, >> + DMA_FENCE_FLAG_CHAIN_BIT, >> DMA_FENCE_FLAG_SIGNALED_BIT, >> DMA_FENCE_FLAG_TIMESTAMP_BIT, >> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >> @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void); >> struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); >> u64 dma_fence_context_alloc(unsigned num); >> >> -extern const struct dma_fence_ops dma_fence_array_ops; >> -extern const struct dma_fence_ops dma_fence_chain_ops; >> - >> /** >> * dma_fence_is_array - check if a fence is from the array subclass >> * @fence: the fence to test >> @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops; >> */ >> static inline bool dma_fence_is_array(struct dma_fence *fence) >> { >> - return fence->ops == &dma_fence_array_ops; >> + return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags); >> } >> >> /** >> @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence) >> */ >> static inline bool dma_fence_is_chain(struct dma_fence *fence) >> { >> - return fence->ops == &dma_fence_chain_ops; >> + return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags); >> } >> >> /** >
On 5/12/25 11:14, Tvrtko Ursulin wrote: > > On 12/05/2025 09:19, Christian König wrote: >> On 5/9/25 17:33, Tvrtko Ursulin wrote: >>> With the goal of reducing the need for drivers to touch fence->ops, we >>> add explicit flags for struct dma_fence_array and struct dma_fence_chain >>> and make the respective helpers (dma_fence_is_array() and >>> dma_fence_is_chain()) use them. >>> >>> This also allows us to remove the exported symbols for the respective >>> operation tables. >> >> That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared. >> >> Since the array and chain are always build in that should be completely unproblematic for driver unload. > > You are right this is not strictly needed. Idea was just to reduce any access to ops as much as we can and this fell under that scope. > > Another benefit one could perhaps argue is two fewer EXPORT_SYMBOLs, which is perhaps a little bit cleaner design (less exporting of implementation details to the outside), but it is not a super strong argument. I would rather say that using the symbols improves things. Background is that otherwise every driver could accidentally or because of malicious intend set this flag. The symbol is not so easily changeable. Regards, Christian. > > If we will not be going for this one then I would be taking 1/13 via drm-intel-gt-next. > > Regards, > > Tvrtko > >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >>> --- >>> drivers/dma-buf/dma-fence-array.c | 2 +- >>> drivers/dma-buf/dma-fence-chain.c | 2 +- >>> include/linux/dma-fence.h | 9 ++++----- >>> 3 files changed, 6 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c >>> index 6657d4b30af9..daf444f5d228 100644 >>> --- a/drivers/dma-buf/dma-fence-array.c >>> +++ b/drivers/dma-buf/dma-fence-array.c >>> @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = { >>> .release = dma_fence_array_release, >>> .set_deadline = dma_fence_array_set_deadline, >>> }; >>> -EXPORT_SYMBOL(dma_fence_array_ops); >>> /** >>> * dma_fence_array_alloc - Allocate a custom fence array >>> @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array, >>> spin_lock_init(&array->lock); >>> dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, >>> context, seqno); >>> + __set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags); >>> init_irq_work(&array->work, irq_dma_fence_array_work); >>> atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); >>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c >>> index a8a90acf4f34..f4abe41fb092 100644 >>> --- a/drivers/dma-buf/dma-fence-chain.c >>> +++ b/drivers/dma-buf/dma-fence-chain.c >>> @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = { >>> .release = dma_fence_chain_release, >>> .set_deadline = dma_fence_chain_set_deadline, >>> }; >>> -EXPORT_SYMBOL(dma_fence_chain_ops); >>> /** >>> * dma_fence_chain_init - initialize a fence chain >>> @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, >>> dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, >>> context, seqno); >>> + __set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags); >>> /* >>> * Chaining dma_fence_chain container together is only allowed through >>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >>> index ac6535716dbe..5bafd0a5f1f1 100644 >>> --- a/include/linux/dma-fence.h >>> +++ b/include/linux/dma-fence.h >>> @@ -98,6 +98,8 @@ struct dma_fence { >>> enum dma_fence_flag_bits { >>> DMA_FENCE_FLAG_SEQNO64_BIT, >>> + DMA_FENCE_FLAG_ARRAY_BIT, >>> + DMA_FENCE_FLAG_CHAIN_BIT, >>> DMA_FENCE_FLAG_SIGNALED_BIT, >>> DMA_FENCE_FLAG_TIMESTAMP_BIT, >>> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >>> @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void); >>> struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); >>> u64 dma_fence_context_alloc(unsigned num); >>> -extern const struct dma_fence_ops dma_fence_array_ops; >>> -extern const struct dma_fence_ops dma_fence_chain_ops; >>> - >>> /** >>> * dma_fence_is_array - check if a fence is from the array subclass >>> * @fence: the fence to test >>> @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops; >>> */ >>> static inline bool dma_fence_is_array(struct dma_fence *fence) >>> { >>> - return fence->ops == &dma_fence_array_ops; >>> + return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags); >>> } >>> /** >>> @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence) >>> */ >>> static inline bool dma_fence_is_chain(struct dma_fence *fence) >>> { >>> - return fence->ops == &dma_fence_chain_ops; >>> + return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags); >>> } >>> /** >> >
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 6657d4b30af9..daf444f5d228 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = { .release = dma_fence_array_release, .set_deadline = dma_fence_array_set_deadline, }; -EXPORT_SYMBOL(dma_fence_array_ops); /** * dma_fence_array_alloc - Allocate a custom fence array @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array, spin_lock_init(&array->lock); dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, context, seqno); + __set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags); init_irq_work(&array->work, irq_dma_fence_array_work); atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c index a8a90acf4f34..f4abe41fb092 100644 --- a/drivers/dma-buf/dma-fence-chain.c +++ b/drivers/dma-buf/dma-fence-chain.c @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = { .release = dma_fence_chain_release, .set_deadline = dma_fence_chain_set_deadline, }; -EXPORT_SYMBOL(dma_fence_chain_ops); /** * dma_fence_chain_init - initialize a fence chain @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, context, seqno); + __set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags); /* * Chaining dma_fence_chain container together is only allowed through diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index ac6535716dbe..5bafd0a5f1f1 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -98,6 +98,8 @@ struct dma_fence { enum dma_fence_flag_bits { DMA_FENCE_FLAG_SEQNO64_BIT, + DMA_FENCE_FLAG_ARRAY_BIT, + DMA_FENCE_FLAG_CHAIN_BIT, DMA_FENCE_FLAG_SIGNALED_BIT, DMA_FENCE_FLAG_TIMESTAMP_BIT, DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void); struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); u64 dma_fence_context_alloc(unsigned num); -extern const struct dma_fence_ops dma_fence_array_ops; -extern const struct dma_fence_ops dma_fence_chain_ops; - /** * dma_fence_is_array - check if a fence is from the array subclass * @fence: the fence to test @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops; */ static inline bool dma_fence_is_array(struct dma_fence *fence) { - return fence->ops == &dma_fence_array_ops; + return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags); } /** @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence) */ static inline bool dma_fence_is_chain(struct dma_fence *fence) { - return fence->ops == &dma_fence_chain_ops; + return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags); } /**
With the goal of reducing the need for drivers to touch fence->ops, we add explicit flags for struct dma_fence_array and struct dma_fence_chain and make the respective helpers (dma_fence_is_array() and dma_fence_is_chain()) use them. This also allows us to remove the exported symbols for the respective operation tables. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> --- drivers/dma-buf/dma-fence-array.c | 2 +- drivers/dma-buf/dma-fence-chain.c | 2 +- include/linux/dma-fence.h | 9 ++++----- 3 files changed, 6 insertions(+), 7 deletions(-)