Re: nvme driver: possible missing `unregister_irq`

[Dennis Dai]

Sorry I missed that point.
Now I do got it!

Thank you very much for the detailed explanation!

Best regards,
Dennis Dai

On Wed, Nov 9, 2022 at 3:58 AM Andreas Hindborg
<andreas.hindborg@xxxxxxx> wrote:
>
>
> 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