Hi Eliot,
On Tue May 19, 2026 at 11:33 AM JST, Eliot Courtney wrote:
>>
>> + // With the GSP shut down, reset the GSP so it can be restarted.
>> + if let Some(unload_bundle) = self.unload.as_ref() {
>> + unload_bundle.run(dev, bar, gsp_falcon, sec2_falcon)?;
>> + } else {
>> + dev_warn!(
>> + dev,
>> + "Unload bundle is missing, GSP won't be properly reset.\n"
>> + );
>> + }
>
> It feels a bit odd to me to (conceptually) allow the unload bundle to be
> run multiple times. IMO, ideally we would be able to take() this. Since
> we don't have a mut self though we can't unless we add a Mutex or
> something. If we do, we can update UnloadBundle::run to consume the
> value so it can only be run once, which is nice. WDYT?
I'm a bit on the fence. On the one hand yes, we typically want the
unload bundle to be run only once. On the other hand, we can also
imagine than `unload` fails for some reason (say, the GSP refused to
shut down when asked), and the driver decides to try again sometime
later. It's not what we do now, but doing so would technically not be
incorrect.
I would feel strongly about enforcing single-shot use if this had safety
implications, but here it is just to enforce something that is already
structurally guaranteed by the driver core and that, in the worst case,
returns a runtime error. IIUC `boot` has the same problem as well - it
can be called even after the GSP is booted, it would just fail pretty
early.
With that in mind, I am not sure it is worth introducing a `Mutex` just
for that. We could also use a `Cell` as of now, but since `Gsp` will
probably need to be `Sync` soon that also won't work too well.
But there is another option: store the `UnloadBundle` into `NovaCore`,
and make `NovaCoreDriver::unbind` pass it to `Gpu::unbind` and by
transition to Gsp::unload` as an argument. This would address the issue
partially, as now unloading can only be initiated by the driver. And
here we should be able to use `Cell` as `NovaCore` doesn't need to be
`Sync`, and implement single-shot logic without a `Mutex`.
The drawback being that this complicates `Gpu::new` a bit, as the unload
bundle would now need to be passed as a `& mut` reference to be written
to, i.e. an output argument which is not super idiomatic to do imho. But
I'll give it a try to see how it looks.
I've addressed all the other comments, thanks for the review!
