Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Sima!


> 
> Yeah I'm not sure a partially converted driver where the main driver is
> still C really works, that pretty much has to throw out all the type
> safety in the interfaces.
> 
> What I think might work is if such partial drivers register as full rust
> drivers, and then largely delegate the implementation to their existing C
> code with a big "safety: trust me, the C side is bug free" comment since
> it's all going to be unsafe :-)
> 
> It would still be a big change, since all the driver's callbacks need to
> switch from container_of to upcast to their driver structure to some small
> rust shim (most likely, I didn't try this out) to get at the driver parts
> on the C side. And I think you also need a small function to downcast to
> the drm base class. But that should be all largely mechanical.
> 
> More freely allowing to mix&match is imo going to be endless pains. We
> kinda tried that with the atomic conversion helpers for legacy kms
> drivers, and the impendance mismatch was just endless amounts of very
> subtle pain. Rust will exacerbate this, because it encodes semantics into
> the types and interfaces. And that was with just one set of helpers, for
> rust we'll likely need a custom one for each driver that's partially
> written in rust.
> -Sima
> 

I humbly disagree here.

I know this is a bit tangential, but earlier this year I converted a bunch of codec libraries to Rust in v4l2. That worked just fine with the C codec drivers. There were no regressions as per our test tools.

The main idea is that you isolate all unsafety to a single point: so long as the C code upholds the safety guarantees when calling into Rust, the Rust layer will be safe. This is just the same logic used in unsafe blocks in Rust itself, nothing new really.

This is not unlike what is going on here, for example:


```
+unsafe extern "C" fn open_callback<T: BaseDriverObject<U>, U: BaseObject>(
+ raw_obj: *mut bindings::drm_gem_object,
+ raw_file: *mut bindings::drm_file,
+) -> core::ffi::c_int {
+ // SAFETY: The pointer we got has to be valid.
+ let file = unsafe {
+ file::File::<<<U as IntoGEMObject>::Driver as drv::Driver>::File>::from_raw(raw_file)
+ };
+ let obj =
+ <<<U as IntoGEMObject>::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(
+ raw_obj,
+ );
+
+ // SAFETY: from_gem_obj() returns a valid pointer as long as the type is
+ // correct and the raw_obj we got is valid.
+ match T::open(unsafe { &*obj }, &file) {
+ Err(e) => e.to_errno(),
+ Ok(()) => 0,
+ }
+}
```

We have to trust that the kernel is passing in a valid pointer. By the same token, we can choose to trust drivers if we so desire.

> that pretty much has to throw out all the type
> safety in the interfaces.

Can you expand on that?

In particular, I believe that we should ideally be able to convert from a C "struct Foo * " to a Rust “FooRef" for types whose lifetimes are managed either by the kernel itself or by a C driver. In practical terms, this has run into the issues we’ve been discussing in this thread, but there may be solutions e.g.:

> One thing that comes to my mindis , you could probably create some driver specific
> "dummy" types to satisfy the type generics of the types you want to use. Not sure
> how well this works out though.

I haven’t thought of anything yet - which is why I haven’t replied. OTOH, IIRC, Faith seems to have something in mind that can work with the current abstractions, so I am waiting on her reply.


> What I think might work is if such partial drivers register as full rust
> drivers, and then largely delegate the implementation to their existing C
> code with a big "safety: trust me, the C side is bug free" comment since
> it's all going to be unsafe :-)

> with a big "safety: trust me, the C side is bug free" comment since it's all going to be unsafe :-)

This is what I want too :) but I can’t see how your proposed approach is better, at least at a cursory glance. It is a much bigger change, though, which is a clear drawback.

> And that was with just one set of helpers, for
> rust we'll likely need a custom one for each driver that's partially
> written in rust.

That’s exactly what I am trying to avoid. In other words, I want to find a way to use the same abstractions and the same APIs so that we do not run precisely into that problem.

— Daniel