* nvme driver: possible missing `unregister_irq`
@ 2022-11-08 17:18 Dennis Dai
2022-11-08 17:46 ` Miguel Ojeda
2022-11-08 19:30 ` Andreas Hindborg
0 siblings, 2 replies; 4+ messages in thread
From: Dennis Dai @ 2022-11-08 17:18 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, rust-for-linux, linux-kernel
Cc: baijiaju1990
Hi all,
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?
Thank you very much!
[1] https://github.com/metaspace/rust-linux/commit/d88c3744d6cbdf11767e08bad56cbfb67c4c96d0
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: nvme driver: possible missing `unregister_irq`
2022-11-08 17:18 nvme driver: possible missing `unregister_irq` Dennis Dai
@ 2022-11-08 17:46 ` Miguel Ojeda
2022-11-08 19:30 ` Andreas Hindborg
1 sibling, 0 replies; 4+ messages in thread
From: Miguel Ojeda @ 2022-11-08 17:46 UTC (permalink / raw)
To: Dennis Dai
Cc: Andreas Hindborg, Wedson Almeida Filho, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
rust-for-linux, linux-kernel, baijiaju1990
On Tue, Nov 8, 2022 at 6:18 PM Dennis Dai <dzy.0424thu@gmail.com> wrote:
>
> I was inspecting the rust nvme driver [1] and would like know if the following
> code contains a missing unregister or I missed anything
(Cc'ing Andreas and fixing Wedson's email -- please do so in the
future, thanks!)
Cheers,
Miguel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: nvme driver: possible missing `unregister_irq`
2022-11-08 17:18 nvme driver: possible missing `unregister_irq` Dennis Dai
2022-11-08 17:46 ` Miguel Ojeda
@ 2022-11-08 19:30 ` Andreas Hindborg
2022-11-09 8:34 ` Dennis Dai
1 sibling, 1 reply; 4+ messages in thread
From: Andreas Hindborg @ 2022-11-08 19:30 UTC (permalink / raw)
To: Dennis Dai
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, rust-for-linux, linux-kernel,
baijiaju1990
Dennis Dai <dzy.0424thu@gmail.com> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: nvme driver: possible missing `unregister_irq`
2022-11-08 19:30 ` Andreas Hindborg
@ 2022-11-09 8:34 ` Dennis Dai
0 siblings, 0 replies; 4+ messages in thread
From: Dennis Dai @ 2022-11-09 8:34 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, rust-for-linux, linux-kernel,
baijiaju1990
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@wdc.com> wrote:
>
>
> Dennis Dai <dzy.0424thu@gmail.com> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-09 8:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-08 17:18 nvme driver: possible missing `unregister_irq` Dennis Dai
2022-11-08 17:46 ` Miguel Ojeda
2022-11-08 19:30 ` Andreas Hindborg
2022-11-09 8:34 ` Dennis Dai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.