Nouveau - Sep 2025 - [PATCH] gpu: nova-core: take advantage of pci::Device::unbind()

On Sun Aug 31, 2025 at 3:50 PM CEST, Alexandre Courbot
wrote:
>> +    pub(crate) fn unbind(&self, pdev:
&pci::Device<device::Bound>) {
>> +        // Unregister the sysmem flush page before we release it.
>> +        kernel::warn_on!(self.bar.access(pdev.as_ref()).map_or(true,
|bar| {
>> +            self.sysmem_flush.unregister(bar);
>> +
>> +            false
>> +        }));
>> +    }
>
> Maybe I'm overtly cautious, but this method can be called from a large
> part of the driver, leaving the Gpu device in a half-unbound state. The
> `PinnedDrop` approach had the benefit of not allowing this.
>
> One way to solve the problem would be to make this method `pub(in
> crate::driver)`, so other modules cannot call it.
pub(in crate::driver) doesn't work, since hierarchically it's a sibling.
:(

However, I can add a doc-comment to make it a bit more obvious.

On Mon Sep 1, 2025 at 7:41 PM JST, Danilo Krummrich
wrote:
> On Sun Aug 31, 2025 at 3:50 PM CEST, Alexandre Courbot wrote:
>>> +    pub(crate) fn unbind(&self, pdev:
&pci::Device<device::Bound>) {
>>> +        // Unregister the sysmem flush page before we release it.
>>> +       
kernel::warn_on!(self.bar.access(pdev.as_ref()).map_or(true, |bar| {
>>> +            self.sysmem_flush.unregister(bar);
>>> +
>>> +            false
>>> +        }));
>>> +    }
>>
>> Maybe I'm overtly cautious, but this method can be called from a
large
>> part of the driver, leaving the Gpu device in a half-unbound state. The
>> `PinnedDrop` approach had the benefit of not allowing this.
>>
>> One way to solve the problem would be to make this method `pub(in
>> crate::driver)`, so other modules cannot call it.
>
> pub(in crate::driver) doesn't work, since hierarchically it's a
sibling. :(
Argh. TIL.
>
> However, I can add a doc-comment to make it a bit more obvious.
Would it also help if we made `Gpu::unbind` take a
`pci::Device<device::Core>`? That way, driver functions that only have a
bound device could not invoke it.

(also, should we make the argument a `device::Device` instead of a
`pci::Device`?)