Re: [PATCH 1/7] rust: pci: pass driver data by value to `unbind`

["Alexandre Courbot" <acourbot@nvidia.com>]

On Tue Dec 16, 2025 at 9:14 PM JST, Danilo Krummrich wrote:
> On Tue Dec 16, 2025 at 6:13 AM CET, Alexandre Courbot wrote:
>> When unbinding a PCI driver, the `T::unbind` callback is invoked by the
>> driver framework, passing the driver data as a `Pin<&T>`.
>>
>> This artificially restricts what the driver can do, as it cannot mutate
>> any state on the data. This becomes a problem in e.g. Nova, which needs
>> to invoke mutable methods when unbinding.
>>
>> `remove_callback` retrieves the driver data by value, and drops it right
>> after the call to `T::unbind`, meaning it is the only reference to the
>> driver data by the time `T::unbind` is called.
>>
>> There is thus no reason for not granting full ownership of the data to
>> `T::unbind`, so do it.
>
> There are multiple reasons I did avoid this for:
>
> (1) Race conditions
>
> A driver can call Device::drvdata() and obtain a reference to the driver's
> device private data as long as it has a &Device<Bound> and asserts the correct
> type of the driver's device private data [1].
>
> Assume you have an IRQ registration, for instance, that lives within this device
> private data.  Within the IRQ handler, nothing prevents us from calling
> Device::drvdata() given that the IRQ handler has a Device<Bound> reference.
>
> Consequently, with passing the device private data by value to unbind() it can
> happen that we have both a mutable and immutable reference at of the device
> private data at the same time.
>
> The same is true for a lot of other cases, such as work items or workqueues that
> are scoped to the Device being bound living within the device private data.
>
> More generally, you can't take full ownership of the device private data as long
> as the device is not yet fully unbound (which is not yet the case in unbind()).

Ah, I completely ignored the fact that we can indeed have other
references to the private data! The fact that `unbind` works with
`Device<Bounded>` should have given me a hint, but somehow I blissfully
ignored that. ^_^;

I will implement some basic locking on the command queue so we can work
with a non-mutable reference.