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@xxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxx>
> > > > 
> > > > Alice