6 ms·

In the function 'pl061_irq_type' the original code has (line 224): writeb(gpiois, pl061->base + GPIOIS); writeb(gpioibe, pl061->base + GPIOIBE); wr

In the function 'pl061_irq_type' the original code has (line 224): writeb(gpiois, pl061->base + GPIOIS); writeb(gpioibe, pl061->base + GPIOIBE); writeb(gpioiev, pl061->base + GPIOIEV); but the translated code has a differing order: pl061.base.writeb(gpioiev, GPIOIEV); pl061.base.writeb(gpiois, GPIOIS); pl061.base.writeb(gpioibe, GPIOIBE); Isn't the order of operations important when talking to I/O?

uxp100 5y ago
I doubt order of operations is important here, but nice catch, I bet that was not on purpose.
geocar 5y ago
> Isn't the order of operations important when talking to I/O? Sometimes, yes. It’s hard to tell if this is one of those times without a closer look at the documentation or at least some experimentation, but because “Rust is Memory Safe(tm)” I think it is safe assume this is fine simply because it compiled, and without knowing anything about you, would recommend you assume the same, because, well, Rust. Obviously.
- karatinversion 5y ago
  > it is safe assume this is fine simply because it compiled, and without knowing anything about you I think this is sarcasm, but for the record, no compiler can make guarantees about the behaviour of a different computer.
  - geocar 5y ago
    > I think this is sarcasm Yes. Maybe satire as well. It’s hard to tell because most comments about rust’s Memory Safety are utter gibberish as well. > no compiler can make guarantees about the behaviour of a different computer. Honestly, I think it’s worse than that: the computer simply cannot tell you what your program is going to do. This should be obvious, and yet many people are using Rust in prayer of exactly this.
- stefan_ 5y ago
  This is writing to the register space of some peripheral so no, the compiler knows nothing about that. It is also extremely common for such peripherals to have requirements on the order of writes and otherwise silently discard them. They also have all kinds of side effects and don't behave like normal R/W memory at all (e.g. clearing an interrupt is usually done writing a 1 to a register which will read 0 after).
  - db48x 5y ago
    If you want that kind of guarantee in a Rust program, you can encode it into the types. It does make the implementation more complex, however, so it is not always worth doing.
  - tialaramex 5y ago
    The compiler doesn't know what's going on here, but fortunately the programmer does and they can use Rust's type safety to reduce the opportunity for foot guns. Notice how the C code calls readb() with some address it got out of another structure adding to a constant from this file, whereas the Rust has readb() as a function implemented on the structure defining what it can write to. As a result, C code can easily mistakenly readb() something it didn't intend to, while the same mistake in Rust fails to compile. Specifically, when you call that Rust readb() function with a constant like GPIO_SIZE that's not actually a GPIO register, it compares the constant you asked to readb to the size of the GPIO's I/O memory (1 page = 4096 bytes) and check it is smaller - if not the type match fails and your compiler tells you this won't work. No runtime crash, it doesn't compile. If you don't have a constant, you can call a try_readb() function, this time you won't fail to compile if you screw up, but at runtime you get EINVAL instead of crashing.
    - geocar 5y ago
      > fortunately the programmer does and they can use Rust's type safety to reduce the opportunity for foot guns. And what, they just didn’t? This is an ordering bug. You could encode that into types perhaps, but I do not think that would be easy or as valuable as other ways to spend time. > Specifically, when you call that Rust readb() function with a constant like GPIO_SIZE that's not actually a GPIO register, it compares the constant you asked to readb to the size of the GPIO's I/O memory (1 page = 4096 bytes) and check it is smaller - if not the type match fails and your compiler tells you this won't work. No runtime crash, it doesn't compile. I think nearly every c compiler has had the ability to warn on something like this for thirty years. What’s the point? The rust programmer could do things differently but so could the C programmer?
floatingatoll 5y ago
I've read through a chunk of the thread and no one has mentioned that on the list yet. You could, I imagine, submit that as a Pull Request here: https://github.com/wedsonaf/linux/tree/pl061 https://github.com/wedsonaf/linux/tree/pl061 Or you can email a note about it to the author at the email address shown in the thread archives. Either way, I bet they'd like to know about the order swap and either add a code comment or unswap it.
brundolf 5y ago
I bet it would be possible to enforce this with some clever types
tialaramex 5y ago
In this case GPIOIS, GPIOIBE and GPIOIEV are basically three registers that between them determine how interrupts work for each pin. Do you want edges or level triggered? Both edges or just one? High levels or low? Order likely makes no practical difference whatsoever. The data sheet actually suggests a particular course of action if you desire edge triggered interrupts but that recommendation doesn't match what either driver actually does here. Real world experience suggests that "Follow the exact steps from the data sheet" is a worthwhile diagnostic step when the driver doesn't work but probably not worth doing if your driver works. AIUI this driver works. It sounds like the worst thing that can go wrong is you eat one spurious interrupt because something happened to the GPIO pin you cared about while you were meddling with the interrupt handling. Confusing for a tightly coded embedded system, but probably unnoticeable in the Linux ecosystem.

uxp100 5y ago

I doubt order of operations is important here, but nice catch, I bet that was not on purpose.

geocar 5y ago

> Isn't the order of operations important when talking to I/O? Sometimes, yes. It’s hard to tell if this is one of those times without a closer look at the documentation or at least some experimentation, but because “Rust is Memory Safe(tm)” I think it is safe assume this is fine simply because it compiled, and without knowing anything about you, would recommend you assume the same, because, well, Rust. Obviously.

karatinversion 5y ago
> it is safe assume this is fine simply because it compiled, and without knowing anything about you I think this is sarcasm, but for the record, no compiler can make guarantees about the behaviour of a different computer.
- geocar 5y ago
  > I think this is sarcasm Yes. Maybe satire as well. It’s hard to tell because most comments about rust’s Memory Safety are utter gibberish as well. > no compiler can make guarantees about the behaviour of a different computer. Honestly, I think it’s worse than that: the computer simply cannot tell you what your program is going to do. This should be obvious, and yet many people are using Rust in prayer of exactly this.
stefan_ 5y ago
This is writing to the register space of some peripheral so no, the compiler knows nothing about that. It is also extremely common for such peripherals to have requirements on the order of writes and otherwise silently discard them. They also have all kinds of side effects and don't behave like normal R/W memory at all (e.g. clearing an interrupt is usually done writing a 1 to a register which will read 0 after).
- db48x 5y ago
  If you want that kind of guarantee in a Rust program, you can encode it into the types. It does make the implementation more complex, however, so it is not always worth doing.
- tialaramex 5y ago
  The compiler doesn't know what's going on here, but fortunately the programmer does and they can use Rust's type safety to reduce the opportunity for foot guns. Notice how the C code calls readb() with some address it got out of another structure adding to a constant from this file, whereas the Rust has readb() as a function implemented on the structure defining what it can write to. As a result, C code can easily mistakenly readb() something it didn't intend to, while the same mistake in Rust fails to compile. Specifically, when you call that Rust readb() function with a constant like GPIO_SIZE that's not actually a GPIO register, it compares the constant you asked to readb to the size of the GPIO's I/O memory (1 page = 4096 bytes) and check it is smaller - if not the type match fails and your compiler tells you this won't work. No runtime crash, it doesn't compile. If you don't have a constant, you can call a try_readb() function, this time you won't fail to compile if you screw up, but at runtime you get EINVAL instead of crashing.
  - geocar 5y ago
    > fortunately the programmer does and they can use Rust's type safety to reduce the opportunity for foot guns. And what, they just didn’t? This is an ordering bug. You could encode that into types perhaps, but I do not think that would be easy or as valuable as other ways to spend time. > Specifically, when you call that Rust readb() function with a constant like GPIO_SIZE that's not actually a GPIO register, it compares the constant you asked to readb to the size of the GPIO's I/O memory (1 page = 4096 bytes) and check it is smaller - if not the type match fails and your compiler tells you this won't work. No runtime crash, it doesn't compile. I think nearly every c compiler has had the ability to warn on something like this for thirty years. What’s the point? The rust programmer could do things differently but so could the C programmer?

floatingatoll 5y ago

I've read through a chunk of the thread and no one has mentioned that on the list yet. You could, I imagine, submit that as a Pull Request here: https://github.com/wedsonaf/linux/tree/pl061 https://github.com/wedsonaf/linux/tree/pl061 Or you can email a note about it to the author at the email address shown in the thread archives. Either way, I bet they'd like to know about the order swap and either add a code comment or unswap it.

brundolf 5y ago

I bet it would be possible to enforce this with some clever types

tialaramex 5y ago

In this case GPIOIS, GPIOIBE and GPIOIEV are basically three registers that between them determine how interrupts work for each pin. Do you want edges or level triggered? Both edges or just one? High levels or low? Order likely makes no practical difference whatsoever. The data sheet actually suggests a particular course of action if you desire edge triggered interrupts but that recommendation doesn't match what either driver actually does here. Real world experience suggests that "Follow the exact steps from the data sheet" is a worthwhile diagnostic step when the driver doesn't work but probably not worth doing if your driver works. AIUI this driver works. It sounds like the worst thing that can go wrong is you eat one spurious interrupt because something happened to the GPIO pin you cared about while you were meddling with the interrupt handling. Confusing for a tightly coded embedded system, but probably unnoticeable in the Linux ecosystem.