| Message ID | 20250318212940.137577-1-dakr@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/2] rust: pci: impl Send + Sync for pci::Device | expand |
"Danilo Krummrich" <dakr@kernel.org> writes: > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > changed the definition of pci::Device and discarded the implicitly > derived Send and Sync traits. > > This isn't required by upstream code yet, and hence did not cause any > issues. However, it is relied on by upcoming drivers, hence add it back > in. > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > rust/kernel/pci.rs | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > index 0ac6cef74f81..0d09ae34a64d 100644 > --- a/rust/kernel/pci.rs > +++ b/rust/kernel/pci.rs > @@ -465,3 +465,10 @@ fn as_ref(&self) -> &device::Device { > unsafe { device::Device::as_ref(dev) } > } > } > + > +// SAFETY: A `Device` is always reference-counted and can be released from any thread. > +unsafe impl Send for Device {} > + > +// SAFETY: `Device` can be shared among threads because all methods of `Device` > +// (i.e. `Device<Normal>) are thread safe. > +unsafe impl Sync for Device {} > > base-commit: 4d320e30ee04c25c660eca2bb33e846ebb71a79a I can't find the base-commit and the patch does not apply clean on v6.14-rc7: patching file rust/kernel/pci.rs Hunk #1 succeeded at 432 with fuzz 1 (offset -33 lines). Otherwise looks good. You might want to rebase? Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org> Best regards, Andreas Hindborg
On Wed, Mar 19, 2025 at 12:18:35PM +0100, Andreas Hindborg wrote: > "Danilo Krummrich" <dakr@kernel.org> writes: > > > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > > changed the definition of pci::Device and discarded the implicitly > > derived Send and Sync traits. > > > > This isn't required by upstream code yet, and hence did not cause any > > issues. However, it is relied on by upcoming drivers, hence add it back > > in. > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > --- > > rust/kernel/pci.rs | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > > index 0ac6cef74f81..0d09ae34a64d 100644 > > --- a/rust/kernel/pci.rs > > +++ b/rust/kernel/pci.rs > > @@ -465,3 +465,10 @@ fn as_ref(&self) -> &device::Device { > > unsafe { device::Device::as_ref(dev) } > > } > > } > > + > > +// SAFETY: A `Device` is always reference-counted and can be released from any thread. > > +unsafe impl Send for Device {} > > + > > +// SAFETY: `Device` can be shared among threads because all methods of `Device` > > +// (i.e. `Device<Normal>) are thread safe. > > +unsafe impl Sync for Device {} > > > > base-commit: 4d320e30ee04c25c660eca2bb33e846ebb71a79a > > I can't find the base-commit and the patch does not apply clean on v6.14-rc7: > > patching file rust/kernel/pci.rs > Hunk #1 succeeded at 432 with fuzz 1 (offset -33 lines). > > Otherwise looks good. You might want to rebase? This is intentional, the base commit is from the driver-core tree, where also commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") landed. > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org> > > > Best regards, > Andreas Hindborg > >
On Tue, Mar 18, 2025 at 10:29:21PM +0100, Danilo Krummrich wrote: > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > changed the definition of pci::Device and discarded the implicitly > derived Send and Sync traits. > > This isn't required by upstream code yet, and hence did not cause any > issues. However, it is relied on by upcoming drivers, hence add it back > in. > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> I have a question related to this ... does the Driver trait need to require T: Send? The change itself LGTM, so: Reviewed-by: Alice Ryhl <aliceryhl@google.com> Alice
On Wed, Mar 19, 2025 at 12:05:13PM +0000, Alice Ryhl wrote: > On Tue, Mar 18, 2025 at 10:29:21PM +0100, Danilo Krummrich wrote: > > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > > changed the definition of pci::Device and discarded the implicitly > > derived Send and Sync traits. > > > > This isn't required by upstream code yet, and hence did not cause any > > issues. However, it is relied on by upcoming drivers, hence add it back > > in. > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > I have a question related to this ... does the Driver trait need to > require T: Send? The driver trait does not have a generic, it doesn't need one. But I think I still get what you're asking. The driver trait never owns a shared reference of the device, it only ever gives out a reference that the driver core guarantees to be valid. > The change itself LGTM, so: > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > Alice
On Wed, Mar 19, 2025 at 01:47:01PM +0100, Danilo Krummrich wrote: > On Wed, Mar 19, 2025 at 12:05:13PM +0000, Alice Ryhl wrote: > > On Tue, Mar 18, 2025 at 10:29:21PM +0100, Danilo Krummrich wrote: > > > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > > > changed the definition of pci::Device and discarded the implicitly > > > derived Send and Sync traits. > > > > > > This isn't required by upstream code yet, and hence did not cause any > > > issues. However, it is relied on by upcoming drivers, hence add it back > > > in. > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > > I have a question related to this ... does the Driver trait need to > > require T: Send? > > The driver trait does not have a generic, it doesn't need one. But I think I > still get what you're asking. Right I mean, should it be: trait Driver: Send + Sync { ... } > The driver trait never owns a shared reference of the device, it only ever gives > out a reference that the driver core guarantees to be valid. > > > The change itself LGTM, so: > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > > Alice
On Wed, Mar 19, 2025 at 01:16:52PM +0000, Alice Ryhl wrote: > On Wed, Mar 19, 2025 at 01:47:01PM +0100, Danilo Krummrich wrote: > > On Wed, Mar 19, 2025 at 12:05:13PM +0000, Alice Ryhl wrote: > > > On Tue, Mar 18, 2025 at 10:29:21PM +0100, Danilo Krummrich wrote: > > > > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > > > > changed the definition of pci::Device and discarded the implicitly > > > > derived Send and Sync traits. > > > > > > > > This isn't required by upstream code yet, and hence did not cause any > > > > issues. However, it is relied on by upcoming drivers, hence add it back > > > > in. > > > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > > > > I have a question related to this ... does the Driver trait need to > > > require T: Send? > > > > The driver trait does not have a generic, it doesn't need one. But I think I > > still get what you're asking. Turns out I did not. :) > Right I mean, should it be: > > trait Driver: Send + Sync { > ... > } Yes, you're absolutely right with this, thanks for pointing this out. > > > The driver trait never owns a shared reference of the device, it only ever gives > > out a reference that the driver core guarantees to be valid. > > > > > The change itself LGTM, so: > > > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > > > > Alice
On Wed, Mar 19, 2025 at 02:30:31PM +0100, Danilo Krummrich wrote: > On Wed, Mar 19, 2025 at 01:16:52PM +0000, Alice Ryhl wrote: > > On Wed, Mar 19, 2025 at 01:47:01PM +0100, Danilo Krummrich wrote: > > > On Wed, Mar 19, 2025 at 12:05:13PM +0000, Alice Ryhl wrote: > > > > On Tue, Mar 18, 2025 at 10:29:21PM +0100, Danilo Krummrich wrote: > > > > > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > > > > > changed the definition of pci::Device and discarded the implicitly > > > > > derived Send and Sync traits. > > > > > > > > > > This isn't required by upstream code yet, and hence did not cause any > > > > > issues. However, it is relied on by upcoming drivers, hence add it back > > > > > in. > > > > > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > > > > > > I have a question related to this ... does the Driver trait need to > > > > require T: Send? > > > > > > The driver trait does not have a generic, it doesn't need one. But I think I > > > still get what you're asking. > > Turns out I did not. :) > > > Right I mean, should it be: > > > > trait Driver: Send + Sync { > > ... > > } > > Yes, you're absolutely right with this, thanks for pointing this out. Just to clarify, the reason we need Sync is that we want to be able to access the driver's private data from an IRQ handler. Otherwise, we can only ever safely access the driver's private data from bus callbacks, which should be synchronized by the device' mutex. > > > > > > The driver trait never owns a shared reference of the device, it only ever gives > > > out a reference that the driver core guarantees to be valid. > > > > > > > The change itself LGTM, so: > > > > > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > > > > > > Alice
On Wed, Mar 19, 2025 at 02:50:49PM +0100, Danilo Krummrich wrote: > On Wed, Mar 19, 2025 at 02:30:31PM +0100, Danilo Krummrich wrote: > > On Wed, Mar 19, 2025 at 01:16:52PM +0000, Alice Ryhl wrote: > > > On Wed, Mar 19, 2025 at 01:47:01PM +0100, Danilo Krummrich wrote: > > > > On Wed, Mar 19, 2025 at 12:05:13PM +0000, Alice Ryhl wrote: > > > > > On Tue, Mar 18, 2025 at 10:29:21PM +0100, Danilo Krummrich wrote: > > > > > > Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") > > > > > > changed the definition of pci::Device and discarded the implicitly > > > > > > derived Send and Sync traits. > > > > > > > > > > > > This isn't required by upstream code yet, and hence did not cause any > > > > > > issues. However, it is relied on by upcoming drivers, hence add it back > > > > > > in. > > > > > > > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > > > > > > > > I have a question related to this ... does the Driver trait need to > > > > > require T: Send? > > > > > > > > The driver trait does not have a generic, it doesn't need one. But I think I > > > > still get what you're asking. > > > > Turns out I did not. :) > > > > > Right I mean, should it be: > > > > > > trait Driver: Send + Sync { > > > ... > > > } > > > > Yes, you're absolutely right with this, thanks for pointing this out. > > Just to clarify, the reason we need Sync is that we want to be able to access > the driver's private data from an IRQ handler. Otherwise, we can only ever > safely access the driver's private data from bus callbacks, which should be > synchronized by the device' mutex. On the other hand, that's up to the IRQ handler abstraction. So, we really should be good with only requiring Send. > > > > > > > > > > The driver trait never owns a shared reference of the device, it only ever gives > > > > out a reference that the driver core guarantees to be valid. > > > > > > > > > The change itself LGTM, so: > > > > > > > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > > > > > > > > Alice
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 0ac6cef74f81..0d09ae34a64d 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -465,3 +465,10 @@ fn as_ref(&self) -> &device::Device { unsafe { device::Device::as_ref(dev) } } } + +// SAFETY: A `Device` is always reference-counted and can be released from any thread. +unsafe impl Send for Device {} + +// SAFETY: `Device` can be shared among threads because all methods of `Device` +// (i.e. `Device<Normal>) are thread safe. +unsafe impl Sync for Device {}
Commit 7b948a2af6b5 ("rust: pci: fix unrestricted &mut pci::Device") changed the definition of pci::Device and discarded the implicitly derived Send and Sync traits. This isn't required by upstream code yet, and hence did not cause any issues. However, it is relied on by upcoming drivers, hence add it back in. Signed-off-by: Danilo Krummrich <dakr@kernel.org> --- rust/kernel/pci.rs | 7 +++++++ 1 file changed, 7 insertions(+) base-commit: 4d320e30ee04c25c660eca2bb33e846ebb71a79a