Add SPI Abstraction #264
Add SPI Abstraction #264
Conversation
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
Nice work !!
To review this properly, I believe that we will probably want to see a skeleton spi client driver as part of this PR. The out-of-tree driver you linked to might be difficult to build. And how would we know we're checking out the right version? For example, the logfile of your crash contains log messages
[ 36.267087] mfrc522: [MFRC522-RS] SPI probed
that I can't find in your linked-to mfrc522 driver.
| probe: Option<SpiMethod>, | ||
| remove: Option<SpiMethod>, | ||
| shutdown: Option<SpiMethodVoid>, | ||
| ) -> Self { |
There was a problem hiding this comment.
Won't your driver require driver data, i.e. the private memory you usually kzalloc() in probe(), and attach to your driver using spi_set_drvdata()? I noticed that in your driver you've sidestepped this issue by storing the driver data as a static. But this will stop working once you instantiate multiple instances (devices) of the same class (driver). E.g. you could have two spi memory chips of the same type. On different SPI busses, or even on the same SPI bus, using a different chip-select.
We currently have a PR in-flight that's trying to solve the problem of driver data, you could help review?
There was a problem hiding this comment.
This is a problem we are indeed facing. While it wasn't necessary to complete on the project we have, it's definitely something that we have to improve on later on. Supporting multiple devices is indeed an application that makes sense, and a shortcoming of our driver as well as this PR.
We could allow the user to pass their own data as an Option<T> and pass a pointer to this instance to the function spi_set_drvdata. As long as the DriverRegistration lives, this pointer will be valid and therefore safe. Another solution would be to add a wrapper around spi_set_drvdata and to let the user use it as they wish.
We currently have a PR in-flight that's trying to solve the problem of driver data, you could help review?
Of course!
You are right. We will provide a sample to accompany this PR. Should it simply contain an example of SPI driver registration or should it contain some more functionality? How would you view it? Since we cannot get probed simply without the necessary hardware resources.
Sorry about that, we edited the backtrace to reflect the actual message in the code. We updated the code in the meantime and forgot to update the backtrace. Skallwar, |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review of the v2 changes you pushed.
I am not convinced that safety is correct here. Or that macros are the best way to call SPI functions.
Speaking more generally, I suspect that you are running into the same problem I encountered myself with #254 : our first instinct is to do a straightforward conversion from C to Rust APIs. While that may be great as a proof-of-concept ("look, we can get this to work in Rust. cool!") it's probably not good enough to convince the upstream kernel community that Rust-as-a-second-language is a worthwhile thing to have. For that to happen, we will probably need to create Rust APIs/abstractions which are significantly better than C for driver writers: safe(r), more intuitive, easier to use. This will likely be the hard part.
(This is only my personal hunch as a hobby contributor - the people in charge @ojeda @wedsonaf may be more aware of the actual situation, and wish to proceed differently)
Can I invite you to help review the PR series starting with #276? It would be great if you could think how this could apply to your spi driver. For example, will it need to bind to devicetree? acpi? How could this be done in Rust in a way that is easier, safer, more intuitive than C?
| pub struct SpiDevice(*mut bindings::spi_device); | ||
|
|
||
| impl SpiDevice { | ||
| pub unsafe fn from_ptr(dev: *mut bindings::spi_device) -> Self { |
There was a problem hiding this comment.
I am not sure if I understand the use of unsafe here. Or the // SAFETY: guarantee in the macros below.
There was a problem hiding this comment.
This was done because of bjorn3's comment
There was a problem hiding this comment.
I am not sure if I understand the use of
unsafehere. Or the// SAFETY:guarantee in the macros below.
What bjorn said is that to the user, this function is unsafe as it uses a raw pointer which could be NULL or invalid. If we mark it as unsafe then we show the user that it holds no guarantees. Regarding the SAFETY comment: When our Spi driver gets probed (or removed, or shut down), the spi_device pointer is provided by the kernel. It is a pointer to the SPI client probing interacting with our device. Therefore, it is safe to pass to this function as it's just a valid pointer that the kernel gives us.
This is a bit similar to the PointerWrapper trait defined here.
There was a problem hiding this comment.
What bjorn said is that to the user, this function is unsafe as it uses a raw pointer which could be NULL or invalid. If we mark it as unsafe then we show the user that it holds no guarantees.
Not exactly -- to clarify a bit: the function does not use the pointer, and so it could potentially be a safe one. The problem is that other functions later on actually use it, and they are not unsafe. Since it is best to require that the pointer is valid just once, it is best to do it here, and then SpiDevice gains a type invariant, which then Spi can use to not need to be unsafe.
Speaking of which: you are missing the documentation and the # Safety section on the function, etc. The SpiDevice should also have documentation, including this type invariant. See other examples in rust/kernel (you can also wait a bit if you do not mind, since I have a PR pending with more docs explaining this).
rust/kernel/spi.rs
Outdated
| impl Drop for DriverRegistration { | ||
| fn drop(&mut self) { | ||
| unsafe { bindings::driver_unregister(&mut self.spi_driver.as_mut().unwrap().driver) } | ||
| // FIXME: No unwrap? But it's safe? |
There was a problem hiding this comment.
Does bindings::spi_driver need to live inside an Option? If you could get rid of the Option, you would not need unwrap() here (which could theoretically panic).
There was a problem hiding this comment.
Does bindings::spi_driver need to live inside an Option
I think not. I made a PR but I need to see if it works correctly before merging it
Definitely agree with this. We implemented something similar to the FileOperations's virtual table that is going to land soon! It's here and being reviewed/rebased
What parts do you have in mind specifically? But this is definitely something that we might be running into, we did go from our C driver to write this abstraction.
For sure! We'll review it all three of us in the morning in a few hours. Is there anything you'd like to see in a small driver example to showcase the usage of our SPI abstraction? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi. I have rebase on top of the |
rust/kernel/spi.rs
Outdated
| this.registered = true; | ||
| Ok(()) | ||
| } | ||
| _ => Err(Error::from_kernel_errno(res)), |
There was a problem hiding this comment.
I think it's better just to have a if for the error case, and the fallthrough to the Ok case.
There was a problem hiding this comment.
Indeed, please always try to keep the happy path non-indented.
This comment has been minimized.
This comment has been minimized.
CohenArthur
force-pushed
the
feature/spi
branch
from
258762b to
063fbcd
Compare
|
I rebased this. The only notable changes are the use of |
| use core::pin::Pin; | ||
|
|
||
| /// Wrapper struct around the kernel's `spi_device`. | ||
| #[derive(Clone, Copy)] |
There was a problem hiding this comment.
Wouldn't this allow me to clone a valid SpiDevice only to have it being deleted eventually while I am still holding a SpiDevice to a dangling pointer which would result in unsafe behaviour when calling the safe write functions below?
There was a problem hiding this comment.
Yes I think that's a problem. I will try to find some time to fix this PR as Rust for Linux has evolved quite a lot and I'm not sure if this is still even compiling
|
I rebased this. The only notable changes is the use of |
This add support for the SPI API for Rust modules. Features: - Register/Unregister - Probe, Remove and Shutdown - write_then_read, write, read Co-developed-by: Martin Schmidt <[email protected]> Signed-off-by: Martin Schmidt <[email protected]> Co-developed-by: Arthur Cohen <[email protected]> Signed-off-by: Arthur Cohen <[email protected]> Signed-off-by: Esteban Blanc <[email protected]>
Signed-off-by: Esteban Blanc <[email protected]>
Signed-off-by: Esteban Blanc <[email protected]>
Signed-off-by: Esteban Blanc <[email protected]>
Skallwar
force-pushed
the
feature/spi
branch
2 times, most recently
from
af38138 to
8b1321d
Compare
Signed-off-by: Esteban Blanc <[email protected]>
ojeda
force-pushed
the
rust-next
branch
3 times, most recently
from
c9b5ce6 to
ce1c54f
Compare
ojeda
force-pushed
the
rust-next
branch
3 times, most recently
from
9ee7197 to
6ce162a
Compare
Hi,
@CohenArthur @n1tram1 and myself are working on a Rust chrdev module for the MFRC522 RFID reader for a class. In order to do so, we needed a new SPI abstraction so we created it.
It is not finished yet but we would like to have some feedback on our work and we are hoping we can merge it in the future.
Currently our driver is compiling correctly using our abstraction
but we have are having some Kernel OOPS when our SPI probe function returns(fixed by e55c21a)Here is the code to our driver if you want to see how we are using it: https://github.com/ks0n/mfrc522-linux