nmdm: Allow module to be force-unloaded. #1367
Conversation
Also fix the order (which is not important). Also fix a typo. Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
If an nmdm device was created by accident such as by running `ls /dev/nmdmAA`, it would restrict the module from being unloaded, despite no data passing through it. With this patch, `kldunload nmdm` will still fail if any nmdm device still exists. `kldunload -f nmdm` will try to destroy all nmdm devices before unloading. If an nmdm modem is either opened in blocking mode or data is being passed through it, after five seconds of trying, it will not be destroyed and the unload will also fail. Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
kevans91
left a comment
kevans91
left a comment
There was a problem hiding this comment.
Some problems with the locking bits, and IMO the other half is working around a bug. I don't see where the current implementation would be intentionally stopping destruction of a nmdm pair that was only ever half open.
Ah, right- we clone it in response to stat(), nothing actually opened it or ever planned on opening it. Hmm. |
kevans91
left a comment
kevans91
left a comment
There was a problem hiding this comment.
Actionable feedback on a path forward for the other half...
| { | ||
| struct nmdmsoftc *ns; | ||
| sbintime_t start_time = sbinuptime(); | ||
| sbintime_t timeout = SBT_1S * 5; // Try to force unload for 5 seconds |
There was a problem hiding this comment.
I think this is entirely valid, but I'd really prefer to accomplish it without the timeout by adding some plumbing through the tty layer to drain the deferred destroy_dev taskqueue. kern_conf.c doesn't seem to have quite the KPI that I think I'd like to see.
Ideally, I think we'd have some kind of tty_drain_freed -> destroy_dev_drain_pending -> taskqueue_drain that we can call to force the async bits to run, then we can just EBUSY it if we still have nmdm devices when we get back.
|
|
||
| mtx_lock(&nmdmsoftc_lock); | ||
| TAILQ_FOREACH(ns, &nmdmsoftc_list, ns_list) { | ||
| nmdm_destroy(ns->ns_part1.np_tty); |
There was a problem hiding this comment.
... and nmdm_destroy should ideally return EBUSY if one of them has both ttys and at least one of the ttys is opened, so that we don't bother waiting at all if we already know it's not going to magically go away.
On entry to nmdm_close(), the tty is already locked. nmdm_destroy() does not return before the tty is unlocked by tty_rel_gone(). This means that we need to lock the tty when the nmdm_close() returns. nmdm_destroy() assumes that the tty is already locked. Therefore, nmdm_unload() must lock the tty before calling nmdm_destroy(). Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
|
@kevans91 Looks like this is ready for you to re-review since it looks like your feedback has been applied. |
|
The remaining recommendation "Ideally, I think we'd have some kind of tty_drain_freed -> destroy_dev_drain_pending -> taskqueue_drain" is not done yet: I have not had time yet to try this. |
|
any update? |
bsdimp
added
changes-required
|
looks like changes are still needed... any upate? |
|
last pinged over 6 months ago, with no update. Closing. |
|
Unfortunately, I never got time to make the changes requested ("without the timeout by adding some plumbing through the tty layer to drain the deferred destroy_dev taskqueue"). Perhaps a freebsd bug report should be made for this, though. |
3 participants
If an nmdm device was created by accident such as by running
ls /dev/nmdmAA, it would restrict the module from being unloaded,despite no data passing through it.
With this patch,
kldunload nmdmwill still fail if any nmdm device stillexists.
kldunload -f nmdmwill try to destroy all nmdm devices before unloading.If an nmdm modem is either opened in blocking mode or data is being
passed through it, after five seconds of trying, it will not be destroyed
and the unload will also fail.
Likewise, fix an incorrect tty_lock, a typo, and the order of locking.