On Thu May 21, 2026 at 8:03 PM JST, Alexandre Courbot wrote:
> 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.
Yerp I think that is very fair, I was hoping you would know some better
way to do this than what I could come up with. If not I think the
current way is okay.
>
> 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!
