[PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Danilo Krummrich]

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
-- 
2.48.1

[PATCH 2/2] rust: platform: impl Send + Sync for platform::Device

[Danilo Krummrich]

Commit 4d320e30ee04 ("rust: platform: fix unrestricted &mut
platform::Device") changed the definition of platform::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/platform.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index c77c9f2e9aea..2811ca53d8b6 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -233,3 +233,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 {}
-- 
2.48.1

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Andreas Hindborg]

"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

Re: [PATCH 2/2] rust: platform: impl Send + Sync for platform::Device

[Andreas Hindborg]

"Danilo Krummrich" <dakr@kernel.org> writes:

> Commit 4d320e30ee04 ("rust: platform: fix unrestricted &mut
> platform::Device") changed the definition of platform::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/platform.rs | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index c77c9f2e9aea..2811ca53d8b6 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -233,3 +233,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 {}

Same as for 1/2:

  # git am --show-current-patch=diff | patch -p1
  patching file rust/kernel/platform.rs
  Hunk #1 succeeded at 198 with fuzz 1 (offset -35 lines).


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Danilo Krummrich]

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
> 
> 

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Alice Ryhl]

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

Re: [PATCH 2/2] rust: platform: impl Send + Sync for platform::Device

[Alice Ryhl]

On Tue, Mar 18, 2025 at 10:29:22PM +0100, Danilo Krummrich wrote:
> Commit 4d320e30ee04 ("rust: platform: fix unrestricted &mut
> platform::Device") changed the definition of platform::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>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Danilo Krummrich]

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

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Alice Ryhl]

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

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Danilo Krummrich]

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

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Danilo Krummrich]

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

Re: [PATCH 1/2] rust: pci: impl Send + Sync for pci::Device

[Danilo Krummrich]

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