Re: nvme driver: possible missing `unregister_irq`

[Andreas Hindborg]

Dennis Dai <dzy.0424thu@xxxxxxxxx> writes:

> I was inspecting the rust nvme driver [1] and would like know if the following
> code contains a missing unregister or I missed anything
>
> // nvme.rs:180, in NvmeDevice::setup_io_queues
>         admin_queue.register_irq(pci_dev)?;
> // nvme.rs:186, in NvmeDevice::setup_io_queues
>         let q_depth = core::cmp::min(...).try_into()?;
> // nvme.rs:190, in NvmeDevice::setup_io_queues
>         let tagset = mq::TagSet::try_new(...)?; //TODO: 1 or 3 on
> demand, depending on polling enabled
>
> Line 186 and 190 could abort the execution of
> NvmeDevice::setup_io_queues without calling `unregister_irq`.
> In the end this could result in an `request_threaded_irq` without a
> pairing `free_irq` on failure.
> Or is the job done by Rust by auto dropping?

In line with my reply to the other potential sleep bug you reported,
teardown is not properly implemented yet, and I did not review the
teardown code that is already in place.

But, if you look at the `register_irq()` and `unregister_irq()`
functions of `NvmeQueue` you can see that the registrations are stored
in an `Option` within the `NvmeQueue` structure. So when the `NvmeQueue`
struct is dropped, the registration will be dropped. Also, if we call
`register_irq()` twice and forget to unregister the first one, it will
be unregistered when we register the second one (because we call
Option::replace()).

So as long as the `NvmeQueue` structs are dropped, we will not leak
IRQs. In case of one of the lines you point to return an `Err`, the ref
count of the `kernel::device::Data` allocated in `probe()` would go to
zero and it would be dropped and thus the IRQs would be unregistered.

So yes, it is handled by destructors that run on drop.

Best regards,
Andreas