Page MenuHomeFreeBSD

Allow vt_timer() callout to run during mountroot input loop in vt(4)
AbandonedPublic

Authored by emaste on Aug 4 2017, 3:59 PM.

Details

Summary

Fixes bug 208239.

During mountroot in vt(4), the vt_timer() callout does not run after regular character key input (special key input causes a manual vd_flush()). Not sure if this is the right approach to enable the callout to run, because I haven't been able to figure out why the callouts are disabled in the first place.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

ray edited edge metadata.Aug 4 2017, 6:46 PM

Sorry, I don't feel myself enough knowledgeable to give sane answer here :)

emaste added a reviewer: kib.Aug 4 2017, 9:01 PM
imp added a subscriber: imp.Aug 4 2017, 9:07 PM

usually a driver implement cngrab and cnungrab to deal with locking and other issues related to multiplexing with other devices. Why can't that be used here?

cngets() already calls cngrab() in sys/kern/kern_cons.c here:

void
cngets(char *cp, size_t size, int visible)
{
	char *lp, *end;
	int c;

	cngrab();
	...
}

Should each cnputc() be wrapped with cngrab() and cnungrab() instead of the entire body of cngets()?

imp added a comment.Aug 4 2017, 9:57 PM

cngets() already calls cngrab() in sys/kern/kern_cons.c here:

void
cngets(char *cp, size_t size, int visible)
{
	char *lp, *end;
	int c;
	cngrab();
	...
}

Should each cnputc() be wrapped with cngrab() and cnungrab() instead of the entire body of cngets()?

I was referring to the cn_grab() and cn_ungrab() function pointers for the driver.

However, we can't call DROP/PICKUP because they have to be in the same block.

emaste edited edge metadata.Aug 15 2017, 8:55 PM

However, we can't call DROP/PICKUP because they have to be in the same block.

I don't follow what you mean here.

kib edited edge metadata.EditedAug 15 2017, 9:02 PM

However, we can't call DROP/PICKUP because they have to be in the same block.

I don't follow what you mean here.

You cannot directly use DROP_GIANT() in one function, and pair it with PICKUP_GIANT() in another. You would need to inline them and save state (the counter of giant mtx recursion) in between. Look at the definitions of these macros.

You cannot directly use DROP_GIANT() in one function, and PICKUP_GIANT() in another.

I mean, I don't see what @imp is pointing out with that statement; unless I'm missing something I don't see anywhere in the patch or discussion that proposes doing so.

imp added a comment.Aug 16 2017, 2:51 AM

You cannot directly use DROP_GIANT() in one function, and PICKUP_GIANT() in another.

I mean, I don't see what @imp is pointing out with that statement; unless I'm missing something I don't see anywhere in the patch or discussion that proposes doing so.

I'm saying that if the driver needs a lock dropped, shouldn't the driver do that?

But the more important, why do we hold GIANT here at all?

Likely superceded by D14712

imp added a comment.Mar 16 2018, 10:23 PM

Likely superceded by D14712

This inspired it. And down the rabbit hole to D14712 I went.

emaste commandeered this revision.Mar 21 2018, 3:16 PM
emaste edited reviewers, added: smahadevan_freebsdfoundation.org; removed: emaste.
imp added a comment.Mar 21 2018, 3:17 PM

I believe this can be closed as OBE

emaste abandoned this revision.Mar 21 2018, 3:17 PM

D14712 committed

imp added a comment.Mar 21 2018, 3:18 PM

Or at least just directly committed to stable/{10,11}... It's still relevant there, and likely the right fix. After arguing against it, I've changed my mind and think it's the right code here in older branches.