Linaro-mm-sig June 2025

linaro-mm-sig@lists.linaro.org

13 participants
42 discussions

Re: [PATCH v6 1/4] sync_file: Protect access to driver and timeline name

by Christian König

On 6/10/25 18:42, Tvrtko Ursulin wrote: > Protect the access to driver and timeline name which otherwise could be > freed as dma-fence exported is signalling fences. > > This prepares the code for incoming dma-fence API changes which will start > asserting these accesses are done from a RCU locked section. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/sync_file.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 212df4b849fe..747e377fb954 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -135,12 +135,18 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) > 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); > snprintf(buf, len, "%s-%s%llu-%lld", > - dma_fence_driver_name(fence), > - dma_fence_timeline_name(fence), > + rcu_dereference(driver), > + rcu_dereference(timeline), > fence->context, > fence->seqno); > + rcu_read_unlock(); > } > > return buf; > @@ -262,9 +268,17 @@ 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) > { > - strscpy(info->obj_name, dma_fence_timeline_name(fence), > + const char __rcu *timeline; > + const char __rcu *driver; > + > + 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, dma_fence_driver_name(fence), > + strscpy(info->driver_name, rcu_dereference(driver), > sizeof(info->driver_name)); > > info->status = dma_fence_get_status(fence); > @@ -273,6 +287,8 @@ static int sync_fill_fence_info(struct dma_fence *fence, > ktime_to_ns(dma_fence_timestamp(fence)) : > ktime_set(0, 0); > > + rcu_read_unlock(); > + > return info->status; > } >

1 week, 4 days

Re: [PATCH v6 3/4] dma-fence: Add safe access helpers and document the rules

by Christian König

On 6/10/25 18:42, Tvrtko Ursulin wrote: > Dma-fence objects currently suffer from a potential use after free problem > where fences exported to userspace and other drivers can outlive the > exporting driver, or the associated data structures. > > The discussion on how to address this concluded that adding reference > counting to all the involved objects is not desirable, since it would need > to be very wide reaching and could cause unloadable drivers if another > entity would be holding onto a signaled fence reference potentially > indefinitely. > > This patch enables the safe access by introducing and documenting a > contract between fence exporters and users. It documents a set of > contraints and adds helpers which a) drivers with potential to suffer from > the use after free must use and b) users of the dma-fence API must use as > well. > > Premise of the design has multiple sides: > > 1. Drivers (fence exporters) MUST ensure a RCU grace period between > signalling a fence and freeing the driver private data associated with it. > > The grace period does not have to follow the signalling immediately but > HAS to happen before data is freed. > > 2. Users of the dma-fence API marked with such requirement MUST contain > the complete access to the data within a single code block guarded by > rcu_read_lock() and rcu_read_unlock(). > > The combination of the two ensures that whoever sees the > DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a > valid fence->lock and valid data potentially accessed by the fence->ops > virtual functions, until the call to rcu_read_unlock(). > > 3. Module unload (fence->ops) disappearing is for now explicitly not > handled. That would required a more complex protection, possibly needing > SRCU instead of RCU to handle callers such as dma_fence_release() and > dma_fence_wait_timeout(), where race between > dma_fence_enable_sw_signaling, signalling, and dereference of > fence->ops->wait() would need a sleeping SRCU context. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-fence.c | 111 ++++++++++++++++++++++++++++--- > include/linux/dma-fence.h | 31 ++++++--- > include/trace/events/dma_fence.h | 38 +++++++++-- > 3 files changed, 157 insertions(+), 23 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 74f9e4b665e3..3f78c56b58dc 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -511,12 +511,20 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) > > dma_fence_enable_sw_signaling(fence); > > - trace_dma_fence_wait_start(fence); > + if (trace_dma_fence_wait_start_enabled()) { > + rcu_read_lock(); > + trace_dma_fence_wait_start(fence); > + rcu_read_unlock(); > + } > if (fence->ops->wait) > ret = fence->ops->wait(fence, intr, timeout); > else > ret = dma_fence_default_wait(fence, intr, timeout); > - trace_dma_fence_wait_end(fence); > + if (trace_dma_fence_wait_end_enabled()) { > + rcu_read_lock(); > + trace_dma_fence_wait_end(fence); > + rcu_read_unlock(); > + } > return ret; > } > EXPORT_SYMBOL(dma_fence_wait_timeout); > @@ -533,16 +541,23 @@ void dma_fence_release(struct kref *kref) > struct dma_fence *fence = > container_of(kref, struct dma_fence, refcount); > > + rcu_read_lock(); > trace_dma_fence_destroy(fence); > > - if (WARN(!list_empty(&fence->cb_list) && > - !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags), > - "Fence %s:%s:%llx:%llx released with pending signals!\n", > - dma_fence_driver_name(fence), > - dma_fence_timeline_name(fence), > - fence->context, fence->seqno)) { > + if (!list_empty(&fence->cb_list) && > + !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > + const char __rcu *timeline; > + const char __rcu *driver; > unsigned long flags; > > + driver = dma_fence_driver_name(fence); > + timeline = dma_fence_timeline_name(fence); > + > + WARN(1, > + "Fence %s:%s:%llx:%llx released with pending signals!\n", > + rcu_dereference(driver), rcu_dereference(timeline), > + fence->context, fence->seqno); > + > /* > * Failed to signal before release, likely a refcounting issue. > * > @@ -556,6 +571,8 @@ void dma_fence_release(struct kref *kref) > spin_unlock_irqrestore(fence->lock, flags); > } > > + rcu_read_unlock(); > + > if (fence->ops->release) > fence->ops->release(fence); > else > @@ -982,11 +999,21 @@ 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 *driver; > + > + rcu_read_lock(); > + > + timeline = dma_fence_timeline_name(fence); > + driver = dma_fence_driver_name(fence); > + > seq_printf(seq, "%s %s seq %llu %ssignalled\n", > - dma_fence_driver_name(fence), > - dma_fence_timeline_name(fence), > + rcu_dereference(driver), > + rcu_dereference(timeline), > fence->seqno, > dma_fence_is_signaled(fence) ? "" : "un"); > + > + rcu_read_unlock(); > } > EXPORT_SYMBOL(dma_fence_describe); > > @@ -1055,3 +1082,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, > BIT(DMA_FENCE_FLAG_SEQNO64_BIT)); > } > EXPORT_SYMBOL(dma_fence_init64); > + > +/** > + * dma_fence_driver_name - Access the driver name > + * @fence: the fence to query > + * > + * Returns a driver name backing the dma-fence implementation. > + * > + * IMPORTANT CONSIDERATION: > + * Dma-fence contract stipulates that access to driver provided data (data not > + * directly embedded into the object itself), such as the &dma_fence.lock and > + * 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) > +{ > + RCU_LOCKDEP_WARN(!rcu_read_lock_held(), > + "RCU protection is required for safe access to returned string"); > + > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return fence->ops->get_driver_name(fence); > + else > + return "detached-driver"; > +} > +EXPORT_SYMBOL(dma_fence_driver_name); > + > +/** > + * dma_fence_timeline_name - Access the timeline name > + * @fence: the fence to query > + * > + * Returns a timeline name provided by the dma-fence implementation. > + * > + * IMPORTANT CONSIDERATION: > + * Dma-fence contract stipulates that access to driver provided data (data not > + * directly embedded into the object itself), such as the &dma_fence.lock and > + * 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) > +{ > + RCU_LOCKDEP_WARN(!rcu_read_lock_held(), > + "RCU protection is required for safe access to returned string"); > + > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return fence->ops->get_driver_name(fence); > + else > + return "signaled-timeline"; > +} > +EXPORT_SYMBOL(dma_fence_timeline_name); > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 10a849cb2d3f..64639e104110 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -378,15 +378,28 @@ bool dma_fence_remove_callback(struct dma_fence *fence, > struct dma_fence_cb *cb); > void dma_fence_enable_sw_signaling(struct dma_fence *fence); > > -static inline const char *dma_fence_driver_name(struct dma_fence *fence) > -{ > - return fence->ops->get_driver_name(fence); > -} > - > -static inline const char *dma_fence_timeline_name(struct dma_fence *fence) > -{ > - return fence->ops->get_timeline_name(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. > + * > + */ > +const char __rcu *dma_fence_driver_name(struct dma_fence *fence); > +const char __rcu *dma_fence_timeline_name(struct dma_fence *fence); > > /** > * dma_fence_is_signaled_locked - Return an indication if the fence > diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h > index 84c83074ee81..4814a65b68dc 100644 > --- a/include/trace/events/dma_fence.h > +++ b/include/trace/events/dma_fence.h > @@ -34,14 +34,44 @@ DECLARE_EVENT_CLASS(dma_fence, > __entry->seqno) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_emit, > +/* > + * Safe only for call sites which are guaranteed to not race with fence > + * signaling,holding the fence->lock and having checked for not signaled, or the > + * signaling path itself. > + */ > +DECLARE_EVENT_CLASS(dma_fence_unsignaled, > + > + TP_PROTO(struct dma_fence *fence), > + > + TP_ARGS(fence), > + > + TP_STRUCT__entry( > + __string(driver, fence->ops->get_driver_name(fence)) > + __string(timeline, fence->ops->get_timeline_name(fence)) > + __field(unsigned int, context) > + __field(unsigned int, seqno) > + ), > + > + TP_fast_assign( > + __assign_str(driver); > + __assign_str(timeline); > + __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->seqno) > +); > + > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_emit, > > TP_PROTO(struct dma_fence *fence), > > TP_ARGS(fence) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_init, > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_init, > > TP_PROTO(struct dma_fence *fence), > > @@ -55,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, > TP_ARGS(fence) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_enable_signal, > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_enable_signal, > > TP_PROTO(struct dma_fence *fence), > > TP_ARGS(fence) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_signaled, > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_signaled, > > TP_PROTO(struct dma_fence *fence), >

1 week, 4 days

[PATCH v3 0/3] media: fix incorrect use of dma_sync_sg_*() calls

by Marek Szyprowski

Dear All, This patchset fixes the incorrect use of dma_sync_sg_*() calls in media and related drivers. They are replaced with much safer dma_sync_sgtable_*() variants, which take care of passing the proper number of elements for the sync operation. Best regards Marek Szyprowski, PhD Samsung R&D Institute Poland Change log: v3: added cc: stable to tags v2: fixes typos and added cc: stable Patch summary: Marek Szyprowski (3): media: videobuf2: use sgtable-based scatterlist wrappers udmabuf: use sgtable-based scatterlist wrappers media: omap3isp: use sgtable-based scatterlist wrappers drivers/dma-buf/udmabuf.c | 5 ++--- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 4 ++-- drivers/media/platform/ti/omap3isp/ispccdc.c | 8 ++++---- drivers/media/platform/ti/omap3isp/ispstat.c | 6 ++---- 4 files changed, 10 insertions(+), 13 deletions(-) -- 2.34.1

1 week, 4 days

[PATCH] dma-buf: fix compare in WARN_ON_ONCE

by Christian König

Smatch pointed out this trivial typo: drivers/dma-buf/dma-buf.c:1123 dma_buf_map_attachment() warn: passing positive error code '16' to 'ERR_PTR' drivers/dma-buf/dma-buf.c 1113 dma_resv_assert_held(attach->dmabuf->resv); 1114 1115 if (dma_buf_pin_on_map(attach)) { 1116 ret = attach->dmabuf->ops->pin(attach); 1117 /* 1118 * Catch exporters making buffers inaccessible even when 1119 * attachments preventing that exist. 1120 */ 1121 WARN_ON_ONCE(ret == EBUSY); ^^^^^ This was probably intended to be -EBUSY? 1122 if (ret) --> 1123 return ERR_PTR(ret); ^^^ Otherwise we will eventually crash. 1124 } 1125 1126 sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); 1127 if (!sg_table) 1128 sg_table = ERR_PTR(-ENOMEM); 1129 if (IS_ERR(sg_table)) 1130 goto error_unpin; 1131 Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 0c48d41dd5eb..451714008e8a 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1060,7 +1060,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, * Catch exporters making buffers inaccessible even when * attachments preventing that exist. */ - WARN_ON_ONCE(ret == EBUSY); + WARN_ON_ONCE(ret == -EBUSY); if (ret) return ERR_PTR(ret); } -- 2.43.0

1 week, 5 days

Re: [PATCH v4 0/4] Implement dmabuf direct I/O via copy_file_range

by Christian König

On 6/9/25 11:32, wangtao wrote: >> -----Original Message----- >> From: Christoph Hellwig <hch(a)infradead.org> >> Sent: Monday, June 9, 2025 12:35 PM >> To: Christian König <christian.koenig(a)amd.com> >> Cc: wangtao <tao.wangtao(a)honor.com>; Christoph Hellwig >> <hch(a)infradead.org>; sumit.semwal(a)linaro.org; kraxel(a)redhat.com; >> vivek.kasireddy(a)intel.com; viro(a)zeniv.linux.org.uk; brauner(a)kernel.org; >> hughd(a)google.com; akpm(a)linux-foundation.org; amir73il(a)gmail.com; >> benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; >> jstultz(a)google.com; tjmercier(a)google.com; jack(a)suse.cz; >> baolin.wang(a)linux.alibaba.com; linux-media(a)vger.kernel.org; dri- >> devel(a)lists.freedesktop.org; linaro-mm-sig(a)lists.linaro.org; linux- >> kernel(a)vger.kernel.org; linux-fsdevel(a)vger.kernel.org; linux- >> mm(a)kvack.org; wangbintian(BintianWang) <bintian.wang(a)honor.com>; >> yipengxiang <yipengxiang(a)honor.com>; liulu 00013167 >> <liulu.liu(a)honor.com>; hanfeng 00012985 <feng.han(a)honor.com> >> Subject: Re: [PATCH v4 0/4] Implement dmabuf direct I/O via >> copy_file_range >> >> On Fri, Jun 06, 2025 at 01:20:48PM +0200, Christian König wrote: >>>> dmabuf acts as a driver and shouldn't be handled by VFS, so I made >>>> dmabuf implement copy_file_range callbacks to support direct I/O >>>> zero-copy. I'm open to both approaches. What's the preference of VFS >>>> experts? >>> >>> That would probably be illegal. Using the sg_table in the DMA-buf >>> implementation turned out to be a mistake. >> >> Two thing here that should not be directly conflated. Using the sg_table was >> a huge mistake, and we should try to move dmabuf to switch that to a pure > I'm a bit confused: don't dmabuf importers need to traverse sg_table to > access folios or dma_addr/len? Do you mean restricting sg_table access > (e.g., only via iov_iter) or proposing alternative approaches? No, accessing pages folios inside the sg_table of a DMA-buf is strictly forbidden. We have removed most use cases of that over the years and push back on generating new ones. > >> dma_addr_t/len array now that the new DMA API supporting that has been >> merged. Is there any chance the dma-buf maintainers could start to kick this >> off? I'm of course happy to assist. Work on that is already underway for some time. Most GPU drivers already do sg_table -> DMA array conversion, I need to push on the remaining to clean up. But there are also tons of other users of dma_buf_map_attachment() which needs to be converted. >> But that notwithstanding, dma-buf is THE buffer sharing mechanism in the >> kernel, and we should promote it instead of reinventing it badly. >> And there is a use case for having a fully DMA mapped buffer in the block >> layer and I/O path, especially on systems with an IOMMU. >> So having an iov_iter backed by a dma-buf would be extremely helpful. >> That's mostly lib/iov_iter.c code, not VFS, though. > Are you suggesting adding an ITER_DMABUF type to iov_iter, or > implementing dmabuf-to-iov_bvec conversion within iov_iter? That would be rather nice to have, yeah. > >> >>> The question Christoph raised was rather why is your CPU so slow that >>> walking the page tables has a significant overhead compared to the >>> actual I/O? >> >> Yes, that's really puzzling and should be addressed first. > With high CPU performance (e.g., 3GHz), GUP (get_user_pages) overhead > is relatively low (observed in 3GHz tests). Even on a low end CPU walking the page tables and grabbing references shouldn't be that much of an overhead. There must be some reason why you see so much CPU overhead. E.g. compound pages are broken up or similar which should not happen in the first place. Regards, Christian. > | 32x32MB Read 1024MB |Creat-ms|Close-ms| I/O-ms|I/O-MB/s| I/O% > |---------------------------|--------|--------|--------|--------|----- > | 1) memfd direct R/W| 1 | 118 | 312 | 3448 | 100% > | 2) u+memfd direct R/W| 196 | 123 | 295 | 3651 | 105% > | 3) u+memfd direct sendfile| 175 | 102 | 976 | 1100 | 31% > | 4) u+memfd direct splice| 173 | 103 | 443 | 2428 | 70% > | 5) udmabuf buffer R/W| 183 | 100 | 453 | 2375 | 68% > | 6) dmabuf buffer R/W| 34 | 4 | 427 | 2519 | 73% > | 7) udmabuf direct c_f_r| 200 | 102 | 278 | 3874 | 112% > | 8) dmabuf direct c_f_r| 36 | 5 | 269 | 4002 | 116% > > With lower CPU performance (e.g., 1GHz), GUP overhead becomes more > significant (as seen in 1GHz tests). > | 32x32MB Read 1024MB |Creat-ms|Close-ms| I/O-ms|I/O-MB/s| I/O% > |---------------------------|--------|--------|--------|--------|----- > | 1) memfd direct R/W| 2 | 393 | 969 | 1109 | 100% > | 2) u+memfd direct R/W| 592 | 424 | 570 | 1884 | 169% > | 3) u+memfd direct sendfile| 587 | 356 | 2229 | 481 | 43% > | 4) u+memfd direct splice| 568 | 352 | 795 | 1350 | 121% > | 5) udmabuf buffer R/W| 597 | 343 | 1238 | 867 | 78% > | 6) dmabuf buffer R/W| 69 | 13 | 1128 | 952 | 85% > | 7) udmabuf direct c_f_r| 595 | 345 | 372 | 2889 | 260% > | 8) dmabuf direct c_f_r| 80 | 13 | 274 | 3929 | 354% > > Regards, > Wangtao.

1 week, 5 days

Re: [PATCH v4 0/4] Implement dmabuf direct I/O via copy_file_range

by Christoph Hellwig

On Mon, Jun 09, 2025 at 09:32:20AM +0000, wangtao wrote: > Are you suggesting adding an ITER_DMABUF type to iov_iter, Yes.

1 week, 5 days

Re: [PATCH v5 1/4] dma-fence: Add safe access helpers and document the rules

by Christian König

On 6/9/25 13:03, Tvrtko Ursulin wrote: > Dma-fence objects currently suffer from a potential use after free problem > where fences exported to userspace and other drivers can outlive the > exporting driver, or the associated data structures. > > The discussion on how to address this concluded that adding reference > counting to all the involved objects is not desirable, since it would need > to be very wide reaching and could cause unloadable drivers if another > entity would be holding onto a signaled fence reference potentially > indefinitely. > > This patch enables the safe access by introducing and documenting a > contract between fence exporters and users. It documents a set of > contraints and adds helpers which a) drivers with potential to suffer from > the use after free must use and b) users of the dma-fence API must use as > well. > > Premise of the design has multiple sides: > > 1. Drivers (fence exporters) MUST ensure a RCU grace period between > signalling a fence and freeing the driver private data associated with it. > > The grace period does not have to follow the signalling immediately but > HAS to happen before data is freed. > > 2. Users of the dma-fence API marked with such requirement MUST contain > the complete access to the data within a single code block guarded by the > new dma_fence_access_begin() and dma_fence_access_end() helpers. > > The combination of the two ensures that whoever sees the > DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a > valid fence->lock and valid data potentially accessed by the fence->ops > virtual functions, until the call to dma_fence_access_end(). > > 3. Module unload (fence->ops) disappearing is for now explicitly not > handled. That would required a more complex protection, possibly needing > SRCU instead of RCU to handle callers such as dma_fence_release() and > dma_fence_wait_timeout(), where race between > dma_fence_enable_sw_signaling, signalling, and dereference of > fence->ops->wait() would need a sleeping SRCU context. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> > --- > drivers/dma-buf/dma-fence.c | 82 +++++++++++++++++++++++++++++++- > include/linux/dma-fence.h | 32 +++++++++---- > include/trace/events/dma_fence.h | 38 +++++++++++++-- > 3 files changed, 138 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 74f9e4b665e3..36604d68bdc8 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -511,12 +511,20 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) > > dma_fence_enable_sw_signaling(fence); > > - trace_dma_fence_wait_start(fence); > + if (trace_dma_fence_wait_start_enabled()) { > + dma_fence_access_begin(); > + trace_dma_fence_wait_start(fence); > + dma_fence_access_end(); > + } > if (fence->ops->wait) > ret = fence->ops->wait(fence, intr, timeout); > else > ret = dma_fence_default_wait(fence, intr, timeout); > - trace_dma_fence_wait_end(fence); > + if (trace_dma_fence_wait_end_enabled()) { > + dma_fence_access_begin(); > + trace_dma_fence_wait_end(fence); > + dma_fence_access_end(); > + } > return ret; > } > EXPORT_SYMBOL(dma_fence_wait_timeout); > @@ -533,6 +541,7 @@ void dma_fence_release(struct kref *kref) > struct dma_fence *fence = > container_of(kref, struct dma_fence, refcount); > > + dma_fence_access_begin(); > trace_dma_fence_destroy(fence); > > if (WARN(!list_empty(&fence->cb_list) && > @@ -556,10 +565,13 @@ void dma_fence_release(struct kref *kref) > spin_unlock_irqrestore(fence->lock, flags); > } > > + dma_fence_access_end(); > + > if (fence->ops->release) > fence->ops->release(fence); > else > dma_fence_free(fence); > + > } > EXPORT_SYMBOL(dma_fence_release); > > @@ -982,11 +994,13 @@ EXPORT_SYMBOL(dma_fence_set_deadline); > */ > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) > { > + dma_fence_access_begin(); > seq_printf(seq, "%s %s seq %llu %ssignalled\n", > dma_fence_driver_name(fence), > dma_fence_timeline_name(fence), > fence->seqno, > dma_fence_is_signaled(fence) ? "" : "un"); > + dma_fence_access_end(); > } > EXPORT_SYMBOL(dma_fence_describe); > > @@ -1055,3 +1069,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, > BIT(DMA_FENCE_FLAG_SEQNO64_BIT)); > } > EXPORT_SYMBOL(dma_fence_init64); > + > +/** > + * dma_fence_driver_name - Access the driver name > + * @fence: the fence to query > + * > + * Returns a driver name backing the dma-fence implementation. > + * > + * IMPORTANT CONSIDERATION: > + * Dma-fence contract stipulates that access to driver provided data (data not > + * directly embedded into the object itself), such as the &dma_fence.lock and > + * 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 &dma_fence_access_being and &dma_fence_access_end pair. > + */ > +const char *dma_fence_driver_name(struct dma_fence *fence) > +{ > + RCU_LOCKDEP_WARN(!rcu_read_lock_held(), > + "dma_fence_access_begin/end() are required for safe access to returned string"); > + > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return fence->ops->get_driver_name(fence); > + else > + return "detached-driver"; > +} > +EXPORT_SYMBOL(dma_fence_driver_name); > + > +/** > + * dma_fence_timeline_name - Access the timeline name > + * @fence: the fence to query > + * > + * Returns a timeline name provided by the dma-fence implementation. > + * > + * IMPORTANT CONSIDERATION: > + * Dma-fence contract stipulates that access to driver provided data (data not > + * directly embedded into the object itself), such as the &dma_fence.lock and > + * 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 &dma_fence_access_being and &dma_fence_access_end pair. > + */ > +const char *dma_fence_timeline_name(struct dma_fence *fence) > +{ > + RCU_LOCKDEP_WARN(!rcu_read_lock_held(), > + "dma_fence_access_begin/end() are required for safe access to returned string"); > + > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return fence->ops->get_driver_name(fence); > + else > + return "signaled-timeline"; > +} > +EXPORT_SYMBOL(dma_fence_timeline_name); > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 10a849cb2d3f..366da956fb85 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -378,15 +378,31 @@ bool dma_fence_remove_callback(struct dma_fence *fence, > struct dma_fence_cb *cb); > void dma_fence_enable_sw_signaling(struct dma_fence *fence); > > -static inline const char *dma_fence_driver_name(struct dma_fence *fence) > -{ > - return fence->ops->get_driver_name(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 > + * &dma_fence_access_begin and &dma_fence_access_end 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. > + * > + */ > +#define dma_fence_access_begin rcu_read_lock > +#define dma_fence_access_end rcu_read_unlock Please drop those in a favor of another change and use rcu_read_lock/unlock in the code directly. But see below. > > -static inline const char *dma_fence_timeline_name(struct dma_fence *fence) > -{ > - return fence->ops->get_timeline_name(fence); > -} > +const char *dma_fence_driver_name(struct dma_fence *fence); > +const char *dma_fence_timeline_name(struct dma_fence *fence); Please declare those as: const char __rcu *dma_fence_driver_name(struct dma_fence *fence); const char __rcu *dma_fence_timeline_name(struct dma_fence *fence); And then use rcu_dereference() on the return value when calling those functions. This allows the automated checker to complain when the functions/return value isn't used correctly. Regards, Christian. > > /** > * dma_fence_is_signaled_locked - Return an indication if the fence > diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h > index 84c83074ee81..4814a65b68dc 100644 > --- a/include/trace/events/dma_fence.h > +++ b/include/trace/events/dma_fence.h > @@ -34,14 +34,44 @@ DECLARE_EVENT_CLASS(dma_fence, > __entry->seqno) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_emit, > +/* > + * Safe only for call sites which are guaranteed to not race with fence > + * signaling,holding the fence->lock and having checked for not signaled, or the > + * signaling path itself. > + */ > +DECLARE_EVENT_CLASS(dma_fence_unsignaled, > + > + TP_PROTO(struct dma_fence *fence), > + > + TP_ARGS(fence), > + > + TP_STRUCT__entry( > + __string(driver, fence->ops->get_driver_name(fence)) > + __string(timeline, fence->ops->get_timeline_name(fence)) > + __field(unsigned int, context) > + __field(unsigned int, seqno) > + ), > + > + TP_fast_assign( > + __assign_str(driver); > + __assign_str(timeline); > + __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->seqno) > +); > + > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_emit, > > TP_PROTO(struct dma_fence *fence), > > TP_ARGS(fence) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_init, > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_init, > > TP_PROTO(struct dma_fence *fence), > > @@ -55,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy, > TP_ARGS(fence) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_enable_signal, > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_enable_signal, > > TP_PROTO(struct dma_fence *fence), > > TP_ARGS(fence) > ); > > -DEFINE_EVENT(dma_fence, dma_fence_signaled, > +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_signaled, > > TP_PROTO(struct dma_fence *fence), >

1 week, 5 days

[PATCH v10 0/9] TEE subsystem for protected dma-buf allocations

by Jens Wiklander

Hi, This patch set allocates the protected DMA-bufs from a DMA-heap instantiated from the TEE subsystem. The TEE subsystem handles the DMA-buf allocations since it is the TEE (OP-TEE, AMD-TEE, TS-TEE, or perhaps a future QTEE) which sets up the protection for the memory used for the DMA-bufs. The DMA-heap uses a protected memory pool provided by the backend TEE driver, allowing it to choose how to allocate the protected physical memory. The allocated DMA-bufs must be imported with a new TEE_IOC_SHM_REGISTER_FD before they can be passed as arguments when requesting services from the secure world. Three use-cases (Secure Video Playback, Trusted UI, and Secure Video Recording) have been identified so far to serve as examples of what can be expected. The use-cases have predefined DMA-heap names, "protected,secure-video", "protected,trusted-ui", and "protected,secure-video-record". The backend driver registers protected memory pools for the use-cases it supports. Each use-case has its own protected memory pool since different use-cases require isolation from different parts of the system. A protected memory pool can be based on a static carveout instantiated while probing the TEE backend driver, or dynamically allocated from CMA (dma_alloc_pages()) and made protected as needed by the TEE. This can be tested on a RockPi 4B+ with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m rockpi4.xml \ -b prototype/sdp-v10 repo sync -j8 cd build make toolchains -j$(nproc) make all -j$(nproc) # Copy ../out/rockpi4.img to an SD card and boot the RockPi from that # Connect a monitor to the RockPi # login and at the prompt: gst-launch-1.0 videotestsrc ! \ aesenc key=1f9423681beb9a79215820f6bda73d0f \ iv=e9aa8e834d8d70b7e0d254ff670dd718 serialize-iv=true ! \ aesdec key=1f9423681beb9a79215820f6bda73d0f ! \ kmssink The aesdec module has been hacked to use an OP-TEE TA to decrypt the stream into protected DMA-bufs which are consumed by the kmssink. The primitive QEMU tests from previous patch sets can be tested on RockPi in the same way using: xtest --sdp-basic The primitive tests are tested on QEMU with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \ -b prototype/sdp-v10 repo sync -j8 cd build make toolchains -j$(nproc) make SPMC_AT_EL=1 all -j$(nproc) make SPMC_AT_EL=1 run-only # login and at the prompt: xtest --sdp-basic The SPMC_AT_EL=1 parameter configures the build with FF-A and an SPMC at S-EL1 inside OP-TEE. The parameter can be changed to SPMC_AT_EL=n to test without FF-A using the original SMC ABI instead. Please remember to do %make arm-tf-clean for TF-A to be rebuilt properly using the new configuration. https://optee.readthedocs.io/en/latest/building/prerequisites.html list dependencies required to build the above. The primitive tests are pretty basic, mostly checking that a Trusted Application in the secure world can access and manipulate the memory. There are also some negative tests for out of bounds buffers, etc. Thanks, Jens Changes since V9: * Adding Sumit's R-B to "optee: sync secure world ABI headers" * Update commit message as requested for "dma-buf: dma-heap: export declared functions". * In "tee: implement protected DMA-heap": - add the hidden config option TEE_DMABUF_HEAPS to tell if the TEE subsystem can support DMA heaps - add a pfn_valid() to check that the passed physical address can be used by __pfn_to_page() and friends - remove the memremap() call, the caller is should do that instead if needed * In "tee: add tee_shm_alloc_dma_mem()" guard the calls to dma_alloc_pages() and dma_free_pages() with TEE_DMABUF_HEAPS to avoid linking errors in some configurations * In "optee: support protected memory allocation": - add the hidden config option OPTEE_STATIC_PROTMEM_POOL to tell if the driver can support a static protected memory pool - optee_protmem_pool_init() is slightly refactored to make the patches that follow easier - Call devm_memremap() before calling tee_protmem_static_pool_alloc() Changes since V8: * Using dma_alloc_pages() instead of cma_alloc() so the direct dependency on CMA can be removed together with the patches "cma: export cma_alloc() and cma_release()" and "dma-contiguous: export dma_contiguous_default_area". The patch * Renaming the patch "tee: add tee_shm_alloc_cma_phys_mem()" to "tee: add tee_shm_alloc_dma_mem()" * Setting DMA mask for the OP-TEE TEE device based on input from the secure world instead of relying on the parent device so following patches are removed: "tee: tee_device_alloc(): copy dma_mask from parent device" and "optee: pass parent device to tee_device_alloc()". * Adding Sumit Garg's R-B to "tee: refactor params_from_user()" * In the patch "tee: implement protected DMA-heap", map the physical memory passed to tee_protmem_static_pool_alloc(). Changes since V7: * Adding "dma-buf: dma-heap: export declared functions", "cma: export cma_alloc() and cma_release()", and "dma-contiguous: export dma_contiguous_default_area" to export the symbols needed to keep the TEE subsystem as a load module. * Removing CONFIG_TEE_DMABUF_HEAP and CONFIG_TEE_CMA since they aren't needed any longer. * Addressing review comments in "optee: sync secure world ABI headers" * Better align protected memory pool initialization between the smc-abi and ffa-abi parts of the optee driver. * Removing the patch "optee: account for direction while converting parameters" Changes since V6: * Restricted memory is now known as protected memory since to use the same term as https://docs.vulkan.org/guide/latest/protected.html. Update all patches to consistently use protected memory. * In "tee: implement protected DMA-heap" add the hidden config option TEE_DMABUF_HEAP to tell if the DMABUF_HEAPS functions are available for the TEE subsystem * Adding "tee: refactor params_from_user()", broken out from the patch "tee: new ioctl to a register tee_shm from a dmabuf file descriptor" * For "tee: new ioctl to a register tee_shm from a dmabuf file descriptor": - Update commit message to mention protected memory - Remove and open code tee_shm_get_parent_shm() in param_from_user_memref() * In "tee: add tee_shm_alloc_cma_phys_mem" add the hidden config option TEE_CMA to tell if the CMA functions are available for the TEE subsystem * For "tee: tee_device_alloc(): copy dma_mask from parent device" and "optee: pass parent device to tee_device_alloc", added Reviewed-by: Sumit Garg <sumit.garg(a)kernel.org> Changes since V5: * Removing "tee: add restricted memory allocation" and "tee: add TEE_IOC_RSTMEM_FD_INFO" * Adding "tee: implement restricted DMA-heap", "tee: new ioctl to a register tee_shm from a dmabuf file descriptor", "tee: add tee_shm_alloc_cma_phys_mem()", "optee: pass parent device to tee_device_alloc()", and "tee: tee_device_alloc(): copy dma_mask from parent device" * The two TEE driver OPs "rstmem_alloc()" and "rstmem_free()" are replaced with a struct tee_rstmem_pool abstraction. * Replaced the the TEE_IOC_RSTMEM_ALLOC user space API with the DMA-heap API Changes since V4: * Adding the patch "tee: add TEE_IOC_RSTMEM_FD_INFO" needed by the GStreamer demo * Removing the dummy CPU access and mmap functions from the dma_buf_ops * Fixing a compile error in "optee: FF-A: dynamic restricted memory allocation" reported by kernel test robot <lkp(a)intel.com> Changes since V3: * Make the use_case and flags field in struct tee_shm u32's instead of u16's * Add more description for TEE_IOC_RSTMEM_ALLOC in the header file * Import namespace DMA_BUF in module tee, reported by lkp(a)intel.com * Added a note in the commit message for "optee: account for direction while converting parameters" why it's needed * Factor out dynamic restricted memory allocation from "optee: support restricted memory allocation" into two new commits "optee: FF-A: dynamic restricted memory allocation" and "optee: smc abi: dynamic restricted memory allocation" * Guard CMA usage with #ifdef CONFIG_CMA, effectively disabling dynamic restricted memory allocate if CMA isn't configured Changes since the V2 RFC: * Based on v6.12 * Replaced the flags for SVP and Trusted UID memory with a u32 field with unique id for each use case * Added dynamic allocation of restricted memory pools * Added OP-TEE ABI both with and without FF-A for dynamic restricted memory * Added support for FF-A with FFA_LEND Changes since the V1 RFC: * Based on v6.11 * Complete rewrite, replacing the restricted heap with TEE_IOC_RSTMEM_ALLOC Changes since Olivier's post [2]: * Based on Yong Wu's post [1] where much of dma-buf handling is done in the generic restricted heap * Simplifications and cleanup * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap support" * Replaced the word "secure" with "restricted" where applicable Etienne Carriere (1): tee: new ioctl to a register tee_shm from a dmabuf file descriptor Jens Wiklander (8): optee: sync secure world ABI headers dma-buf: dma-heap: export declared functions tee: implement protected DMA-heap tee: refactor params_from_user() tee: add tee_shm_alloc_dma_mem() optee: support protected memory allocation optee: FF-A: dynamic protected memory allocation optee: smc abi: dynamic protected memory allocation drivers/dma-buf/dma-heap.c | 3 + drivers/tee/Kconfig | 5 + drivers/tee/Makefile | 1 + drivers/tee/optee/Kconfig | 5 + drivers/tee/optee/Makefile | 1 + drivers/tee/optee/core.c | 10 + drivers/tee/optee/ffa_abi.c | 147 +++++++++- drivers/tee/optee/optee_ffa.h | 27 +- drivers/tee/optee/optee_msg.h | 84 +++++- drivers/tee/optee/optee_private.h | 15 +- drivers/tee/optee/optee_smc.h | 37 ++- drivers/tee/optee/protmem.c | 335 +++++++++++++++++++++ drivers/tee/optee/smc_abi.c | 142 ++++++++- drivers/tee/tee_core.c | 155 +++++++--- drivers/tee/tee_heap.c | 472 ++++++++++++++++++++++++++++++ drivers/tee/tee_private.h | 16 + drivers/tee/tee_shm.c | 189 +++++++++++- include/linux/tee_core.h | 71 +++++ include/linux/tee_drv.h | 10 + include/uapi/linux/tee.h | 31 ++ 20 files changed, 1689 insertions(+), 67 deletions(-) create mode 100644 drivers/tee/optee/protmem.c create mode 100644 drivers/tee/tee_heap.c base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca -- 2.43.0

1 week, 5 days

Re: [PATCH v4 0/4] Implement dmabuf direct I/O via copy_file_range

by Christian König

On 6/6/25 11:52, wangtao wrote: > > >> -----Original Message----- >> From: Christoph Hellwig <hch(a)infradead.org> >> Sent: Tuesday, June 3, 2025 9:20 PM >> To: Christian König <christian.koenig(a)amd.com> >> Cc: Christoph Hellwig <hch(a)infradead.org>; wangtao >> <tao.wangtao(a)honor.com>; sumit.semwal(a)linaro.org; kraxel(a)redhat.com; >> vivek.kasireddy(a)intel.com; viro(a)zeniv.linux.org.uk; brauner(a)kernel.org; >> hughd(a)google.com; akpm(a)linux-foundation.org; amir73il(a)gmail.com; >> benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; >> jstultz(a)google.com; tjmercier(a)google.com; jack(a)suse.cz; >> baolin.wang(a)linux.alibaba.com; linux-media(a)vger.kernel.org; dri- >> devel(a)lists.freedesktop.org; linaro-mm-sig(a)lists.linaro.org; linux- >> kernel(a)vger.kernel.org; linux-fsdevel(a)vger.kernel.org; linux- >> mm(a)kvack.org; wangbintian(BintianWang) <bintian.wang(a)honor.com>; >> yipengxiang <yipengxiang(a)honor.com>; liulu 00013167 >> <liulu.liu(a)honor.com>; hanfeng 00012985 <feng.han(a)honor.com> >> Subject: Re: [PATCH v4 0/4] Implement dmabuf direct I/O via >> copy_file_range >> >> On Tue, Jun 03, 2025 at 03:14:20PM +0200, Christian König wrote: >>> On 6/3/25 15:00, Christoph Hellwig wrote: >>>> This is a really weird interface. No one has yet to explain why >>>> dmabuf is so special that we can't support direct I/O to it when we >>>> can support it to otherwise exotic mappings like PCI P2P ones. >>> >>> With udmabuf you can do direct I/O, it's just inefficient to walk the >>> page tables for it when you already have an array of all the folios. >> >> Does it matter compared to the I/O in this case? >> >> Either way there has been talk (in case of networking implementations) that >> use a dmabuf as a first class container for lower level I/O. >> I'd much rather do that than adding odd side interfaces. I.e. have a version >> of splice that doesn't bother with the pipe, but instead just uses in-kernel >> direct I/O on one side and dmabuf-provided folios on the other. > If the VFS layer recognizes dmabuf type and acquires its sg_table > and folios, zero-copy could also be achieved. I initially thought > dmabuf acts as a driver and shouldn't be handled by VFS, so I made > dmabuf implement copy_file_range callbacks to support direct I/O > zero-copy. I'm open to both approaches. What's the preference of > VFS experts? That would probably be illegal. Using the sg_table in the DMA-buf implementation turned out to be a mistake. The question Christoph raised was rather why is your CPU so slow that walking the page tables has a significant overhead compared to the actual I/O? Regards, Christian. > > Regards, > Wangtao. >

1 week, 6 days

Re: [PATCH v7 10/10] arm64: dts: rockchip: enable NPU on ROCK 5B

by Heiko Stübner

Am Freitag, 6. Juni 2025, 11:20:32 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli: > Hi Tomeu, > > On Friday, 6 June 2025 08:28:30 Central European Summer Time Tomeu Vizoso wrote: > > From: Nicolas Frattaroli <nicolas.frattaroli(a)collabora.com> > > > > The NPU on the ROCK5B uses the same regulator for both the sram-supply > > and the npu's supply. Add this regulator, and enable all the NPU bits. > > Also add the regulator as a domain-supply to the pd_npu power domain. > > > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli(a)collabora.com> > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > > --- > > arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 56 +++++++++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > > index d22068475c5dc6cb885f878f3f527a66edf1ba70..49500f7cbcb14af4919a6c1997e9e53a01d84973 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > > @@ -316,6 +316,28 @@ regulator-state-mem { > > }; > > }; > > > > +&i2c1 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2c1m2_xfer>; > > + status = "okay"; > > + > > + vdd_npu_s0: regulator@42 { > > + compatible = "rockchip,rk8602"; > > + reg = <0x42>; > > + fcs,suspend-voltage-selector = <1>; > > + regulator-name = "vdd_npu_s0"; > > + regulator-boot-on; > > + regulator-min-microvolt = <550000>; > > + regulator-max-microvolt = <950000>; > > + regulator-ramp-delay = <2300>; > > + vin-supply = <&vcc5v0_sys>; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > + }; > > +}; > > + > > &i2c6 { > > status = "okay"; > > > > @@ -440,6 +462,10 @@ &pd_gpu { > > domain-supply = <&vdd_gpu_s0>; > > }; > > > > +&pd_npu { > > + domain-supply = <&vdd_npu_s0>; > > +}; > > + > > &pinctrl { > > hdmirx { > > hdmirx_hpd: hdmirx-5v-detection { > > @@ -500,6 +526,36 @@ &pwm1 { > > status = "okay"; > > }; > > > > +&rknn_core_top { > > + npu-supply = <&vdd_npu_s0>; > > + sram-supply = <&vdd_npu_s0>; > > + status = "okay"; > > +}; > > + > > +&rknn_core_1 { > > + npu-supply = <&vdd_npu_s0>; > > + sram-supply = <&vdd_npu_s0>; > > + status = "okay"; > > +}; > > + > > +&rknn_core_2 { > > + npu-supply = <&vdd_npu_s0>; > > + sram-supply = <&vdd_npu_s0>; > > + status = "okay"; > > +}; > > + > > +&rknn_mmu_top { > > + status = "okay"; > > +}; > > + > > +&rknn_mmu_1 { > > + status = "okay"; > > +}; > > + > > +&rknn_mmu_2 { > > + status = "okay"; > > +}; > > + > > &saradc { > > vref-supply = <&avcc_1v8_s0>; > > status = "okay"; > > > > > > Feel free to replace this patch with the following, if your series is > based on linux-next or v6.16. It moves the enablement into the new > shared ROCK 5B/5B+ dtsi, and I've added a regulator-enable-ramp-delay > while I was at it because I've run into hard-to-reproduce problems > relating to it before that Heiko quickly identified and fixed in his > recent series[1] for basically all already present regulators. Remains > to be seen if the final patch lands in that form but this should make > it easier for people to try out as it means a bad luck roll for the > day won't make them run into as many weird issues. > > [1]: https://lore.kernel.org/all/20250605185001.377055-1-heiko@sntech.de/ Reading that just now reminds me to point to https://lore.kernel.org/lkml/20250606190418.478633-1-heiko@sntech.de/ As Chen Yu pointed out in the reply to v2, this is more a property of the regulator IC itself, so likely should go into the driver. So with a bit of luck after 6.16-rc1 all the fan53555 clones should use somewhat hardware-accurate enable-times. > From ff1c370a158f4340aa5dfa4ed5034e815e5371be Mon Sep 17 00:00:00 2001 > From: Nicolas Frattaroli <nicolas.frattaroli(a)collabora.com> > Date: Tue, 3 Jun 2025 17:03:10 +0200 > Subject: [PATCH] arm64: dts: rockchip: enable NPU on ROCK 5B/+ > > The NPU on the ROCK5B uses the same regulator for both the sram-supply > and the npu's supply. Add this regulator, and enable all the NPU bits. > Also add the regulator as a domain-supply to the pd_npu power domain. > > The 5B+'s regulator setup is identical to the 5B in this regard, so it > goes in the shared .dtsi. > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli(a)collabora.com> > --- > .../boot/dts/rockchip/rk3588-rock-5b.dtsi | 57 +++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi > index 51e83f0ed809..5a20cc2555fb 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi > @@ -332,6 +332,29 @@ regulator-state-mem { > }; > }; > > +&i2c1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c1m2_xfer>; > + status = "okay"; > + > + vdd_npu_s0: regulator@42 { > + compatible = "rockchip,rk8602"; > + reg = <0x42>; > + fcs,suspend-voltage-selector = <1>; > + regulator-name = "vdd_npu_s0"; > + regulator-boot-on; > + regulator-enable-ramp-delay = <500>; > + regulator-min-microvolt = <550000>; > + regulator-max-microvolt = <950000>; > + regulator-ramp-delay = <2300>; > + vin-supply = <&vcc5v0_sys>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > +}; > + > &i2c3 { I think your patch here has some other dependency still? Because in the rk3588-rock-5b.dtsi there is no i2c3 yet In torvalds' branch from _just now_ ;-) . Heiko > status = "okay"; > }; > @@ -521,6 +544,10 @@ &pd_gpu { > domain-supply = <&vdd_gpu_s0>; > }; > > +&pd_npu { > + domain-supply = <&vdd_npu_s0>; > +}; > + > &pinctrl { > hdmirx { > hdmirx_hpd: hdmirx-5v-detection { > @@ -585,6 +612,36 @@ &pwm1 { > status = "okay"; > }; > > +&rknn_core_top { > + npu-supply = <&vdd_npu_s0>; > + sram-supply = <&vdd_npu_s0>; > + status = "okay"; > +}; > + > +&rknn_core_1 { > + npu-supply = <&vdd_npu_s0>; > + sram-supply = <&vdd_npu_s0>; > + status = "okay"; > +}; > + > +&rknn_core_2 { > + npu-supply = <&vdd_npu_s0>; > + sram-supply = <&vdd_npu_s0>; > + status = "okay"; > +}; > + > +&rknn_mmu_top { > + status = "okay"; > +}; > + > +&rknn_mmu_1 { > + status = "okay"; > +}; > + > +&rknn_mmu_2 { > + status = "okay"; > +}; > + > &saradc { > vref-supply = <&avcc_1v8_s0>; > status = "okay"; >

2 weeks, 1 day

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig June 2025