sdio-adma2-refactor #34

astro · 2020-06-10T06:47:51+08:00

astro commented

2020-06-10 06:47:51 +08:00

@pca006132 Every commit compiles for me, so you can review and pick them one by one.

Apart from removing that one global static mut and the attribute.modify(), the changes are not supposed to change the build. They still need to be tested as I don't know what data to expect from the Sdcard.

@pca006132 Every commit compiles for me, so you can review and pick them one by one. Apart from removing that one global `static mut` and the `attribute.modify()`, the changes are not supposed to change the build. They still need to be tested as I don't know what data to expect from the Sdcard.

pca006132 commented

2020-06-10 10:33:04 +08:00

Still thinking about undefined behavior...

https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.as_mut_ptr

Gets a mutable pointer to the contained value. Reading from this pointer or turning it into a reference is undefined behavior unless the MaybeUninit<T> is initialized.

https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#incorrect-usages-of-this-method-1

The third example:

Nor can you use direct field access to do field-by-field gradual initialization:

#![feature(maybe_uninit_ref)]
use std::{mem::MaybeUninit, ptr};

struct Foo {
    a: u32,
    b: u8,
}

let foo: Foo = unsafe {
    let mut foo = MaybeUninit::<Foo>::uninit();
    ptr::write(&mut foo.get_mut().a as *mut u32, 1337);
                // ^^^^^^^^^^^^^
                // (mutable) reference to uninitialized memory!
                // This is undefined behavior.
    ptr::write(&mut foo.get_mut().b as *mut u8, 42);
                // ^^^^^^^^^^^^^
                // (mutable) reference to uninitialized memory!
                // This is undefined behavior.
    foo.assume_init()
};

From what I've seen, it seems that the only safe way is to use write to write to the uninit memory directly without referencing any of its content. However, with bitfield and VolatileCell, we cannot initialize the struct directly without unsafe operations like transmute or something like that...

I'm thinking if it would be easier to just stick with the current implementation... I think the volatile register crate was meant to be used with memory mapped registers but not a generic volatile cell, so it did not implement Copy.

I cannot think of any safer way to circumvent that, and it seems that implementing a custom VolatileCell with Copy would also be weird, as we cannot enforce the Copy to be volatile. (or not, not familiar with this)

@sb10q what do you think?

Still thinking about undefined behavior... https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.as_mut_ptr > Gets a mutable pointer to the contained value. Reading from this pointer or turning it into a reference is undefined behavior unless the `MaybeUninit<T>` is initialized. https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#incorrect-usages-of-this-method-1 > The third example: > > Nor can you use direct field access to do field-by-field gradual initialization: > ```rust > #![feature(maybe_uninit_ref)] > use std::{mem::MaybeUninit, ptr}; > > struct Foo { > a: u32, > b: u8, > } > > let foo: Foo = unsafe { > let mut foo = MaybeUninit::<Foo>::uninit(); > ptr::write(&mut foo.get_mut().a as *mut u32, 1337); > // ^^^^^^^^^^^^^ > // (mutable) reference to uninitialized memory! > // This is undefined behavior. > ptr::write(&mut foo.get_mut().b as *mut u8, 42); > // ^^^^^^^^^^^^^ > // (mutable) reference to uninitialized memory! > // This is undefined behavior. > foo.assume_init() > }; > ``` From what I've seen, it seems that the only safe way is to use `write` to write to the uninit memory directly without referencing any of its content. However, with `bitfield` and `VolatileCell`, we cannot initialize the struct directly without unsafe operations like transmute or something like that... I'm thinking if it would be easier to just stick with the current implementation... I think the volatile register crate was meant to be used with memory mapped registers but not a generic volatile cell, so it did not implement `Copy`. I cannot think of any safer way to circumvent that, and it seems that implementing a custom `VolatileCell` with `Copy` would also be weird, as we cannot enforce the `Copy` to be volatile. (or not, not familiar with this) @sb10q what do you think?

astro commented

2020-06-10 17:14:56 +08:00

Poster

Good point. I'm going to look if we can just 0-initialize instead.

pca006132 commented

2020-06-11 09:47:06 +08:00

I wonder if align(4) would be correct... from the SD Specification Part A2, the ADMA2 descriptor table format is:

I wonder if `align(4)` would be correct... from the SD Specification Part A2, the ADMA2 descriptor table format is:

Screenshot from 2020-06-11 09-44-55.png

37 KiB

pca006132 commented

2020-06-11 10:06:24 +08:00

OK nevermind, I think I misunderstood the alignment attribute, it is for the whole struct instead of individual fields.

I've tested it and it works, I think we can merge it.

OK nevermind, I think I misunderstood the alignment attribute, it is for the whole struct instead of individual fields. I've tested it and it works, I think we can merge it.

sb10q closed this pull request

2020-06-11 10:07:19 +08:00

astro deleted branch sdio-adma2-refactor

2020-06-11 19:16:28 +08:00

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/zynq-rs#34