[PATCH v8 0/2] Add dma coherent allocator abstraction

Abdiel Janulgue posted 2 patches 11 months, 2 weeks ago
There is a newer version of this series
rust/bindings/bindings_helper.h |   1 +
rust/kernel/dma.rs              | 271 ++++++++++++++++++++++++++++++++
rust/kernel/error.rs            |   1 +
rust/kernel/lib.rs              |   1 +
4 files changed, 274 insertions(+)
create mode 100644 rust/kernel/dma.rs
[PATCH v8 0/2] Add dma coherent allocator abstraction
Posted by Abdiel Janulgue 11 months, 2 weeks ago
Changes since v7:
- Remove cpu_buf() and cpu_buf_mut() as exporting a r/w interface via
  a slice is undefined behaviour due to slice's requirement that the
  underlying pointer should not be modified (Alice Ryhl, Robin Murphy).
- Reintroduce r/w helpers instead which includes proper safety
  invariants (Daniel Almeida).
- Link to v7: https://lore.kernel.org/lkml/20241210221603.3174929-1-abdiel.janulgue@gmail.com/

Changes since v6:
- Include the dma_attrs in the constructor, use alloc::Flags as inpiration

Changes since v5:
- Remove unnecessary lifetime annotation when returning the CPU buffer.

Changes since v4:
- Documentation and example fixes, use Markdown formatting (Miguel Ojeda).
- Discard read()/write() helpers to remove bound on Copy and fix overhead
  (Daniel Almeida).
- Improve error-handling in the constructor block (Andreas Hindborg).

Changes since v3:
- Reject ZST types by checking the type size in the constructor in
  addition to requiring FromBytes/AsBytes traits for the type (Alice Ryhl).

Changes since v2:
- Fixed missing header for generating the bindings.

Changes since v1:
- Fix missing info in commit log where EOVERFLOW is used.
- Restrict the dma coherent allocator to numeric types for now for valid
  behaviour (Daniel Almeida).
- Build slice dynamically.

Abdiel Janulgue (2):
  rust: error: Add EOVERFLOW
  rust: add dma coherent allocator abstraction.

 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/dma.rs              | 271 ++++++++++++++++++++++++++++++++
 rust/kernel/error.rs            |   1 +
 rust/kernel/lib.rs              |   1 +
 4 files changed, 274 insertions(+)
 create mode 100644 rust/kernel/dma.rs


base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
-- 
2.43.0
Re: [PATCH v8 0/2] Add dma coherent allocator abstraction
Posted by Pyrex 10 months, 1 week ago
I'm nervy about the Rust code here.

- read()'s comment says it takes a snapshot, but it doesn't

- read()'s name implies it does a read, but it doesn't. It returns a 
live, dangerous view

- into_parts()'s comment claims to decrement the refcount. One, it 
doesn't. Two, it probably shouldn't, if it's supposed to transfer ownership.

- write() shouldn't take an immutable receiver without unsafe

- write() is unsound if used with the slice from read()

- the mutation in write() breaks read() without contradicting its 
`Safety` requirements

- write() memcpys T, which isn't explicitly Copy

This doesn't have to be this unsafe. AsBytes + FromBytes implies Copy 
(or at least it _should_) -- so the view could be of Cell.
Re: [PATCH v8 0/2] Add dma coherent allocator abstraction
Posted by Danilo Krummrich 10 months, 1 week ago
Hi,

On Mon, Feb 10, 2025 at 12:54:25AM -0800, Pyrex wrote:
> I'm nervy about the Rust code here.

Code reviews are always welcome!

However, please make sure to consider the following.

When sending feedback, please try to keep up with the latest version [1] of the
patchset.

Please send your comments inline keeping the relevant context and removing the
irrelevant parts in your reply. Otherwise it is impossible to have a discussion
that everyone can follow.

Thanks!

[1] https://lore.kernel.org/rust-for-linux/20250123104333.1340512-1-abdiel.janulgue@gmail.com/