| Message ID | 20230928-rust-send-trait-v1-3-30b4f59d13cb@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | thread-safety doc + Rust modeling | expand |
On 28-09-23, 16:37, Erik Schilling wrote: > The thread-safety rules of libgpiod allow individual object instances to > be used from different threads. So far, this was not actually possible > with the Rust bindings. Not being `Send` disallowed the user to transfer > the ownership to different threads. > > Rust also has a `Sync` marker. That one would even allow sending > references of objects to other threads. Since we wrap a lot of C > functions with `fn foo(&self)` signatures, that would not be safe. > libgpiod does not allow concurrent API calls to the same object instance > - which Rust would allow for read-only references. Thus, we do not > define that one. > > Chip was already modeled correctly. > > line::Info is not marked as Send since it may either be owning or non- > owning. That problem is fixed as part of a separate pull request [1]. > > [1] https://lore.kernel.org/r/20230927-rust-line-info-soundness-v1-0-990dce6f18ab@linaro.org > > Link: https://lore.kernel.org/r/CVHO091CC80Y.3KUOSLSOBVL0T@ablu-work > Signed-off-by: Erik Schilling <erik.schilling@linaro.org> > --- > bindings/rust/libgpiod/src/edge_event.rs | 4 ++++ > bindings/rust/libgpiod/src/event_buffer.rs | 8 ++++++++ > bindings/rust/libgpiod/src/info_event.rs | 4 ++++ > bindings/rust/libgpiod/src/line_config.rs | 4 ++++ > bindings/rust/libgpiod/src/line_request.rs | 4 ++++ > bindings/rust/libgpiod/src/line_settings.rs | 4 ++++ > bindings/rust/libgpiod/src/request_config.rs | 4 ++++ > 7 files changed, 32 insertions(+) Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Fri, Sep 29, 2023 at 2:47 PM Erik Schilling <erik.schilling@linaro.org> wrote: > > On Fri Sep 29, 2023 at 2:45 PM CEST, Bartosz Golaszewski wrote: > > On Fri, Sep 29, 2023 at 12:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 28-09-23, 16:37, Erik Schilling wrote: > > > > The thread-safety rules of libgpiod allow individual object instances to > > > > be used from different threads. So far, this was not actually possible > > > > with the Rust bindings. Not being `Send` disallowed the user to transfer > > > > the ownership to different threads. > > > > > > > > Rust also has a `Sync` marker. That one would even allow sending > > > > references of objects to other threads. Since we wrap a lot of C > > > > functions with `fn foo(&self)` signatures, that would not be safe. > > > > libgpiod does not allow concurrent API calls to the same object instance > > > > - which Rust would allow for read-only references. Thus, we do not > > > > define that one. > > > > > > > > Chip was already modeled correctly. > > > > > > > > line::Info is not marked as Send since it may either be owning or non- > > > > owning. That problem is fixed as part of a separate pull request [1]. > > > > > > > > [1] https://lore.kernel.org/r/20230927-rust-line-info-soundness-v1-0-990dce6f18ab@linaro.org > > > > > > > > Link: https://lore.kernel.org/r/CVHO091CC80Y.3KUOSLSOBVL0T@ablu-work > > > > Signed-off-by: Erik Schilling <erik.schilling@linaro.org> > > > > --- > > > > bindings/rust/libgpiod/src/edge_event.rs | 4 ++++ > > > > bindings/rust/libgpiod/src/event_buffer.rs | 8 ++++++++ > > > > bindings/rust/libgpiod/src/info_event.rs | 4 ++++ > > > > bindings/rust/libgpiod/src/line_config.rs | 4 ++++ > > > > bindings/rust/libgpiod/src/line_request.rs | 4 ++++ > > > > bindings/rust/libgpiod/src/line_settings.rs | 4 ++++ > > > > bindings/rust/libgpiod/src/request_config.rs | 4 ++++ > > > > 7 files changed, 32 insertions(+) > > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > > > Thanks, do you have any comments about the other patches in this series? > > Hm. The others are not Rust-related and mostly try to document our > discussion about thread-safety of the C lib. It would probably be good > if you could double-check that I summarized everything correctly. > > - Erik > Of course they aren't and I should have read them before assuming the entire series is for rust bindings. :) Thanks. Bart
diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs index 0c0cfbc..639f033 100644 --- a/bindings/rust/libgpiod/src/edge_event.rs +++ b/bindings/rust/libgpiod/src/edge_event.rs @@ -24,6 +24,10 @@ use super::{ #[derive(Debug, Eq, PartialEq)] pub struct Event(*mut gpiod::gpiod_edge_event); +// SAFETY: Event models a wrapper around an owned gpiod_edge_event and may +// be safely sent to other threads. +unsafe impl Send for Event {} + impl Event { pub fn event_clone(event: &Event) -> Result<Event> { // SAFETY: `gpiod_edge_event` is guaranteed to be valid here. diff --git a/bindings/rust/libgpiod/src/event_buffer.rs b/bindings/rust/libgpiod/src/event_buffer.rs index 2e4bfd3..68d6e2f 100644 --- a/bindings/rust/libgpiod/src/event_buffer.rs +++ b/bindings/rust/libgpiod/src/event_buffer.rs @@ -68,6 +68,14 @@ pub struct Buffer { events: Vec<*mut gpiod::gpiod_edge_event>, } +// SAFETY: Buffer models an owned gpiod_edge_event_buffer. However, there may +// be events tied to it. Concurrent access from multiple threads to a buffer +// and its associated events is not allowed by the C lib. +// In Rust, those events will always be borrowed from a buffer instance. Thus, +// either Rust prevents the user to move the Buffer while there are still +// borrowed events, or we can safely send the the Buffer. +unsafe impl Send for Buffer {} + impl Buffer { /// Create a new edge event buffer. /// diff --git a/bindings/rust/libgpiod/src/info_event.rs b/bindings/rust/libgpiod/src/info_event.rs index db60600..1e26b37 100644 --- a/bindings/rust/libgpiod/src/info_event.rs +++ b/bindings/rust/libgpiod/src/info_event.rs @@ -25,6 +25,10 @@ pub struct Event { pub(crate) event: *mut gpiod::gpiod_info_event, } +// SAFETY: Event models a wrapper around an owned gpiod_info_event and may be +// safely sent to other threads. +unsafe impl Send for Event {} + impl Event { /// Get a single chip's line's status change event. pub(crate) fn new(event: *mut gpiod::gpiod_info_event) -> Self { diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs index f4f03f1..d0a4aba 100644 --- a/bindings/rust/libgpiod/src/line_config.rs +++ b/bindings/rust/libgpiod/src/line_config.rs @@ -28,6 +28,10 @@ pub struct Config { pub(crate) config: *mut gpiod::gpiod_line_config, } +// SAFETY: Config models a wrapper around an owned gpiod_line_config and may be +// safely sent to other threads. +unsafe impl Send for Config {} + impl Config { /// Create a new line config object. pub fn new() -> Result<Self> { diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs index a5697d6..64ef05d 100644 --- a/bindings/rust/libgpiod/src/line_request.rs +++ b/bindings/rust/libgpiod/src/line_request.rs @@ -20,6 +20,10 @@ pub struct Request { pub(crate) request: *mut gpiod::gpiod_line_request, } +// SAFETY: Request models a wrapper around an owned gpiod_line_request and may +// be safely sent to other threads. +unsafe impl Send for Request {} + impl Request { /// Request a set of lines for exclusive usage. pub(crate) fn new(request: *mut gpiod::gpiod_line_request) -> Result<Self> { diff --git a/bindings/rust/libgpiod/src/line_settings.rs b/bindings/rust/libgpiod/src/line_settings.rs index f0b3e9c..c81d118 100644 --- a/bindings/rust/libgpiod/src/line_settings.rs +++ b/bindings/rust/libgpiod/src/line_settings.rs @@ -25,6 +25,10 @@ pub struct Settings { pub(crate) settings: *mut gpiod::gpiod_line_settings, } +// SAFETY: Settings models a wrapper around an owned gpiod_line_settings and may +// be safely sent to other threads. +unsafe impl Send for Settings {} + impl Settings { /// Create a new line settings object. pub fn new() -> Result<Self> { diff --git a/bindings/rust/libgpiod/src/request_config.rs b/bindings/rust/libgpiod/src/request_config.rs index 5bde7c6..9b66cc9 100644 --- a/bindings/rust/libgpiod/src/request_config.rs +++ b/bindings/rust/libgpiod/src/request_config.rs @@ -20,6 +20,10 @@ pub struct Config { pub(crate) config: *mut gpiod::gpiod_request_config, } +// SAFETY: Config models a wrapper around an owned gpiod_request_config and may +// be safely sent to other threads. +unsafe impl Send for Config {} + impl Config { /// Create a new request config object. pub fn new() -> Result<Self> {
The thread-safety rules of libgpiod allow individual object instances to be used from different threads. So far, this was not actually possible with the Rust bindings. Not being `Send` disallowed the user to transfer the ownership to different threads. Rust also has a `Sync` marker. That one would even allow sending references of objects to other threads. Since we wrap a lot of C functions with `fn foo(&self)` signatures, that would not be safe. libgpiod does not allow concurrent API calls to the same object instance - which Rust would allow for read-only references. Thus, we do not define that one. Chip was already modeled correctly. line::Info is not marked as Send since it may either be owning or non- owning. That problem is fixed as part of a separate pull request [1]. [1] https://lore.kernel.org/r/20230927-rust-line-info-soundness-v1-0-990dce6f18ab@linaro.org Link: https://lore.kernel.org/r/CVHO091CC80Y.3KUOSLSOBVL0T@ablu-work Signed-off-by: Erik Schilling <erik.schilling@linaro.org> --- bindings/rust/libgpiod/src/edge_event.rs | 4 ++++ bindings/rust/libgpiod/src/event_buffer.rs | 8 ++++++++ bindings/rust/libgpiod/src/info_event.rs | 4 ++++ bindings/rust/libgpiod/src/line_config.rs | 4 ++++ bindings/rust/libgpiod/src/line_request.rs | 4 ++++ bindings/rust/libgpiod/src/line_settings.rs | 4 ++++ bindings/rust/libgpiod/src/request_config.rs | 4 ++++ 7 files changed, 32 insertions(+)