6 ms·

> I have no idea what this is supposed to mean. Drivers obviously interact with each other and with the rest of the system via all kinds of in-kernel interfaces

> I have no idea what this is supposed to mean. Drivers obviously interact with each other and with the rest of the system via all kinds of in-kernel interfaces. But the total footprint of the interfaces that typical drivers use is fairly small. When a rusty shim is provided that provides access to those interfaces, you can write pure rust drivers.

megous 5y ago
Typical device drivers with limited interface footprint also barely do anything interesting. For example look at typical audio codec drivers. They barely have any procedural code (and when they do it's just some form of register value mapping anyway), locking/concurency and almost all resource management is handled at the higher level and the driver is not exposed to it. Another kind of a driver that features a very limited dependency on Linux interfaces is a SoC clock/reset driver. And clock drivers are even more boring in this aspect. Code vs data ratio is probably < 0.1 in many of those. I guess some drivers that do more complicated stuff, and deal with concurrency issues or more complicated resource management may benefit, but they'll likely need to touch much larger API surface, too. And I dunno, even the sample driver in the patchset has things like this: let mut data = kernel::new_device_data!( gpio::RegistrationWithIrqChip::new(), PL061Resources { // SAFETY: This device doesn't support DMA. base: unsafe { IoMem::try_new(res)? }, parent_irq: irq, }, // SAFETY: We call `spinlock_init` below. unsafe { SpinLock::new(PL061Data::default()) }, "PL061::Registrations" )?; // SAFETY: General part of the data is pinned when `data` is. let gen = unsafe { data.as_mut().map_unchecked_mut(|d| &mut **d) }; kernel::spinlock_init!(gen, "PL061::General"); let data = Ref::<DeviceData>::from(data); It's barely readable. I don't know what type `data` is, what `gen` is or how it's different from `data`. Variable names don't really help. `data.as_mut().map_unchecked_mut(|d| &mut *d)` also looks like some kind of WTF to the uninitiated, that is somehow retrieving some 'general part of data' whatever that is. When I look at the kernel's spinlock type https://elixir.bootlin.com/linux/latest/source/include/linux/spinlock_types.h#L17 https://elixir.bootlin.com/linux/latest/source/include/linux... that doesn't really help me determine why whatever this `gen` [variable?] is needs to be passed to `spinlock_init`. And what's with the two `let data`. Are they the same type or is the type now different and the first data variable now hidden? Because it looks like second `let data` gets somehow transformed from the first `data` into something else. So now we have two identically named `data` variables with different types in the same function? Wasn't this supposed to be some simpler language to draw more people to the kernel development? At least with C each variable has a type that's quickly identifiable. I guess after 15 minutes I see what this is trying to do. It passes some newly created spinlock object to `new_device_data` function and then calls `spinlock_init` on some part of the data returned from `new_device_data` that needs to be extracted from it via some really obtuse transformation, just to initialize the spinlock later instead of directly in `SpinLock::new` for some obscure reason, using two unsafe {} blocks in the process. But oh my god, if this kind of obfuscation is going to be the new standard for the kernel,... At least C is the devil I know. :)
- johnisgood 5y ago
  Yeah, I hardly understand Rust code, and I did not start learning programming yesterday. If you want to practice, check out https://github.com/immunant/ioq3/blob/transpiled/quake3-rs/qagamex86_64/src/qcommon/q_math.rs https://github.com/immunant/ioq3/blob/transpiled/quake3-rs/q... or whatever.
  - estebank 5y ago
    That is not idiomatic code: it is an auto-translated code straight from C. You are judging Rust by looking at what idiomatic C code looks like in Rust.
    - johnisgood 5y ago
      No no no, I know that it is not idiomatic code, it says in the URL: "transpiled". I am just telling him to practice using it. I was not judging Rust at all. That said, idiomatic Rust is difficult for me to read/understand as well. I do not expect device drivers in Rust to be readable (by me), and if they have lots of unsafe blocks then we are back to square one I would say.
    - ggrrhh_ta 5y ago
      how would the idiomatic code for that example look like? (seriously)
      - johnisgood 5y ago
        Here is the refactored code: https://github.com/immunant/ioq3/blob/refactored/quake3-rs/qagamex86_64/src/qcommon/q_math.rs https://github.com/immunant/ioq3/blob/refactored/quake3-rs/q...
        estebank 5y ago
        I wouldn't call that code idiomatic either: it is cleaned up from the automatic translation, but it still a far cry from what Rust written by hand would look like pub static mut g_color_table: [crate::src::qcommon::q_shared::vec4_t; 8] = [ [0f32, 0f32, 0f32, 1f32], [1f32, 0f32, 0f32, 1f32], [0f32, 1f32, 0f32, 1f32], [1f32, 1f32, 0f32, 1f32], [0f32, 0f32, 1f32, 1f32], [0f32, 1f32, 1f32, 1f32], [1f32, 0f32, 1f32, 1f32], [1f32, 1f32, 1f32, 1f32], ]; would maybe look like pub static mut g_color_table: [[f32; 4]; 8] = [ [0, 0, 0, 1], [1, 0, 0, 1], [0, 1, 0, 1], [1, 1, 0, 1], [0, 0, 1, 1], [0, 1, 1, 1], [1, 0, 1, 1], [1, 1, 1, 1], ]; but it still looks weird to me (a static mut would be uncommon in a design, and not having a new-type to represent these would also be uncommon). If you are interested in looking what Rust written from the ground up in the same domain looks like, you can maybe look at Bevy[1]. The biggest source of "noise" in Rust is explicit type conversions, FP constructs and longer types (`.into()`/`.as_mut_ref()`/`.map()`/`Result<Option<T>, E>`). [1]: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/mesh/mesh.rs https://github.com/bevyengine/bevy/blob/main/crates/bevy_ren...

megous 5y ago

Typical device drivers with limited interface footprint also barely do anything interesting. For example look at typical audio codec drivers. They barely have any procedural code (and when they do it's just some form of register value mapping anyway), locking/concurency and almost all resource management is handled at the higher level and the driver is not exposed to it. Another kind of a driver that features a very limited dependency on Linux interfaces is a SoC clock/reset driver. And clock drivers are even more boring in this aspect. Code vs data ratio is probably < 0.1 in many of those. I guess some drivers that do more complicated stuff, and deal with concurrency issues or more complicated resource management may benefit, but they'll likely need to touch much larger API surface, too. And I dunno, even the sample driver in the patchset has things like this: let mut data = kernel::new_device_data!( gpio::RegistrationWithIrqChip::new(), PL061Resources { // SAFETY: This device doesn't support DMA. base: unsafe { IoMem::try_new(res)? }, parent_irq: irq, }, // SAFETY: We call `spinlock_init` below. unsafe { SpinLock::new(PL061Data::default()) }, "PL061::Registrations" )?; // SAFETY: General part of the data is pinned when `data` is. let gen = unsafe { data.as_mut().map_unchecked_mut(|d| &mut **d) }; kernel::spinlock_init!(gen, "PL061::General"); let data = Ref::<DeviceData>::from(data); It's barely readable. I don't know what type `data` is, what `gen` is or how it's different from `data`. Variable names don't really help. `data.as_mut().map_unchecked_mut(|d| &mut *d)` also looks like some kind of WTF to the uninitiated, that is somehow retrieving some 'general part of data' whatever that is. When I look at the kernel's spinlock type https://elixir.bootlin.com/linux/latest/source/include/linux/spinlock_types.h#L17 https://elixir.bootlin.com/linux/latest/source/include/linux... that doesn't really help me determine why whatever this `gen` [variable?] is needs to be passed to `spinlock_init`. And what's with the two `let data`. Are they the same type or is the type now different and the first data variable now hidden? Because it looks like second `let data` gets somehow transformed from the first `data` into something else. So now we have two identically named `data` variables with different types in the same function? Wasn't this supposed to be some simpler language to draw more people to the kernel development? At least with C each variable has a type that's quickly identifiable. I guess after 15 minutes I see what this is trying to do. It passes some newly created spinlock object to `new_device_data` function and then calls `spinlock_init` on some part of the data returned from `new_device_data` that needs to be extracted from it via some really obtuse transformation, just to initialize the spinlock later instead of directly in `SpinLock::new` for some obscure reason, using two unsafe {} blocks in the process. But oh my god, if this kind of obfuscation is going to be the new standard for the kernel,... At least C is the devil I know. :)

johnisgood 5y ago
Yeah, I hardly understand Rust code, and I did not start learning programming yesterday. If you want to practice, check out https://github.com/immunant/ioq3/blob/transpiled/quake3-rs/qagamex86_64/src/qcommon/q_math.rs https://github.com/immunant/ioq3/blob/transpiled/quake3-rs/q... or whatever.
- estebank 5y ago
  That is not idiomatic code: it is an auto-translated code straight from C. You are judging Rust by looking at what idiomatic C code looks like in Rust.
  - johnisgood 5y ago
    No no no, I know that it is not idiomatic code, it says in the URL: "transpiled". I am just telling him to practice using it. I was not judging Rust at all. That said, idiomatic Rust is difficult for me to read/understand as well. I do not expect device drivers in Rust to be readable (by me), and if they have lots of unsafe blocks then we are back to square one I would say.
  - ggrrhh_ta 5y ago
    how would the idiomatic code for that example look like? (seriously)
    - johnisgood 5y ago
      Here is the refactored code: https://github.com/immunant/ioq3/blob/refactored/quake3-rs/qagamex86_64/src/qcommon/q_math.rs https://github.com/immunant/ioq3/blob/refactored/quake3-rs/q...
      - estebank 5y ago
        I wouldn't call that code idiomatic either: it is cleaned up from the automatic translation, but it still a far cry from what Rust written by hand would look like pub static mut g_color_table: [crate::src::qcommon::q_shared::vec4_t; 8] = [ [0f32, 0f32, 0f32, 1f32], [1f32, 0f32, 0f32, 1f32], [0f32, 1f32, 0f32, 1f32], [1f32, 1f32, 0f32, 1f32], [0f32, 0f32, 1f32, 1f32], [0f32, 1f32, 1f32, 1f32], [1f32, 0f32, 1f32, 1f32], [1f32, 1f32, 1f32, 1f32], ]; would maybe look like pub static mut g_color_table: [[f32; 4]; 8] = [ [0, 0, 0, 1], [1, 0, 0, 1], [0, 1, 0, 1], [1, 1, 0, 1], [0, 0, 1, 1], [0, 1, 1, 1], [1, 0, 1, 1], [1, 1, 1, 1], ]; but it still looks weird to me (a static mut would be uncommon in a design, and not having a new-type to represent these would also be uncommon). If you are interested in looking what Rust written from the ground up in the same domain looks like, you can maybe look at Bevy[1]. The biggest source of "noise" in Rust is explicit type conversions, FP constructs and longer types (`.into()`/`.as_mut_ref()`/`.map()`/`Result<Option<T>, E>`). [1]: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/mesh/mesh.rs https://github.com/bevyengine/bevy/blob/main/crates/bevy_ren...