Re: [PATCH 2/5] rust: device: Add a minimal RawDevice trait
From: Robin Murphy
Date: Fri Feb 24 2023 - 10:15:29 EST
On 2023-02-24 14:48, Asahi Lina wrote:
On 2023/02/24 23:32, Robin Murphy wrote:
On 2023-02-24 14:11, Greg Kroah-Hartman wrote:
Thanks for the detailed rust explainations, I'd like to just highlight
one thing:
On Fri, Feb 24, 2023 at 10:15:12PM +0900, Asahi Lina wrote:
On 24/02/2023 20.23, Greg Kroah-Hartman wrote:
And again, why are bindings needed for a "raw" struct device at all?
Shouldn't the bus-specific wrappings work better?
Because lots of kernel subsystems need to be able to accept "any" device
and don't care about the bus! That's what this is for.
That's great, but:
All the bus
wrappers would implement this so they can be used as an argument for all
those subsystems (plus a generic one when you just need to pass around
an actual owned generic reference and no longer need bus-specific
operations - you can materialize that out of a RawDevice impl, which is
when get_device() would be called). That's why I'm introducing this now,
because both io_pgtable and rtkit need to take `struct device` pointers
on the C side so we need some "generic struct device" view on the
Rust side.
In looking at both ftkit and io_pgtable, those seem to be good examples
of how "not to use a struct device", so trying to make safe bindings
from Rust to these frameworks is very ironic :)
rtkit takes a struct device pointer and then never increments it,
despite saving it off, which is unsafe. It then only uses it to print
out messages if things go wrong (or right in some cases), which is odd.
So it can get away from using a device pointer entirely, except for the
devm_apple_rtkit_init() call, which I doubt you want to call from rust
code, right?
for io_pgtable, that's a bit messier, you want to pass in a device that
io_pgtable treats as a "device" but again, it is NEVER properly
reference counted, AND, it is only needed to try to figure out the bus
operations that dma memory should be allocated from for this device. So
what would be better to save off there would be a pointer to the bus,
which is constant and soon will be read-only so there are no lifetime
rules needed at all (see the major struct bus_type changes going into
6.3-rc1 that will enable that to happen).
FWIW the DMA API *has* to know which specific device it's operating
with, since the relevant properties can and do vary even between
different devices within a single bus_type (e.g. DMA masks).
In the case of io-pgtable at least, there's no explicit refcounting
since the struct device must be the one representing the physical
platform/PCI/etc. device consuming the pagetable, so if that were to
disappear from underneath its driver while the pagetable is still in
use, things would already have gone very very wrong indeed :)
There's no terribly good way to encode this relationship in safe Rust as
far as I know. So although it might be "obvious" (and I think my driver
can never violate it as it is currently designed), this means the Rust
abstraction will have to take the device reference if the C side does
not, because safe rust abstractions have to actually make these bugs
impossible and nothing stops a Rust driver from, say, stashing an
io_pgtable reference into a global and letting the device go away.
If someone did that, then simply holding a struct device reference
wouldn't guarantee much, since it only prevents the pointer itself from
becoming invalid - it still doesn't say any of the data *in* the
structure is still valid and "safe" for what a DMA API call might do
with it.
At the very least you'd probably have to somehow also guarantee that the
device has a driver bound (which is the closest thing to a general
indication of valid DMA ops across all architectures) and block it from
unbinding for the lifetime of the reference, but that would then mean a
simple driver which expects to tear down its io-pgtable from its .remove
callback could never be unbound due to the circular dependency :/
Cheers,
Robin.