Paths

Table of Contentst

-
sys/
-
amd64/conf/
-
conf/
-
GENERIC
-
arm/conf/
-
conf/
-
GENERIC
2/5
RPI-B
2/5
TEGRA124
-
arm64/conf/
-
conf/
-
std.dev
-
i386/conf/
-
conf/
-
GENERIC
-
powerpc/conf/
-
conf/
-
GENERIC
-
GENERIC64
-
GENERIC64LE
-
MPC85XX
-
MPC85XXSPE
-
QORIQ64
-
riscv/conf/
-
conf/
-
GENERIC

conf: Add hkbd and hms to GENERIC* kernel configs
ClosedPublic
Actions

Authored by manu on Jun 20 2024, 8:11 AM.

Details

Reviewers

andrew
wulf
imp
emaste

Commits

rG8e908fab6c5b: conf: Add usbhid and hidbus to GENERIC* kernel configs
rG13d00a43cba4: conf: Add usbhid and hidbus to GENERIC* kernel configs

Summary

Include the new unified HID stack by default in generic.
This will allow us to migrate to the multi-stack hkbd and hms instead of
relying on the older ukbd and ums which only work with USB.
To test those drivers just add hw.usb.usbhid.enable=1 in loader.conf

Sponsored by: Beckhoff Automation GmbH & Co. KG

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

manu created this revision.Jun 20 2024, 8:11 AM

Herald added a reviewer: andrew. · View Herald TranscriptJun 20 2024, 8:11 AM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added subscribers: riscv, jhibbits, emaste, imp. · View Herald Transcript

manu requested review of this revision.Jun 20 2024, 8:11 AM

Harbormaster completed remote builds in B58262: Diff 140023.Jun 20 2024, 8:11 AM

manu added a child revision: D45659: Enable usbhid by default.Jun 20 2024, 8:11 AM

manu added a reviewer: wulf.Jun 20 2024, 8:11 AM

Apart from the one mistakenly added hid kernel, I like it. But I hit approve because that mistake predates this change.

sys/powerpc/conf/QORIQ64
130	I don't think this kernel should even have hid at all

This revision is now accepted and ready to land.Jun 20 2024, 2:32 PM

Seems reasonable to me, if we have ukbd compiled in as well. Can you also indicate in the commit message what we get out of using hkbd instead of ukbd?

(Mostly as a thought experiment) would it be possible for these to all be loaded as modules?

jhibbits added inline comments.Jun 20 2024, 6:57 PM

sys/powerpc/conf/QORIQ64
130	I added it because QORIQ64 is most likely to be seen with the AmigaOne X5000 desktop (I did all the book-E work over the past decade or so with that in mind).

In D45658#1041688, @emaste wrote:

Seems reasonable to me, if we have ukbd compiled in as well. Can you also indicate in the commit message what we get out of using hkbd instead of ukbd?

I thought my message was clear on this, can you suggest better wording then ?

(Mostly as a thought experiment) would it be possible for these to all be loaded as modules?

They can but I'm unsure if everything works (same with old uhid stack). If I boot MINIMAL and load usb, uhid and ukbd from loader I still don't have a keyboard, same for loading usb, usbhid and hkbd. So I'm unsure if we have everything correct for runtime loadable usb stack.

gehaowu_bitmoe.com added a subscriber: gehaowu_bitmoe.com.Jun 21 2024, 7:34 AM

gehaowu_bitmoe.com removed a subscriber: gehaowu_bitmoe.com.

wulf accepted this revision.Jun 21 2024, 3:52 PM

In D45658#1041770, @manu wrote:

In D45658#1041688, @emaste wrote:

Seems reasonable to me, if we have ukbd compiled in as well. Can you also indicate in the commit message what we get out of using hkbd instead of ukbd?

I thought my message was clear on this, can you suggest better wording then ?

Include the new unified HID stack by default in generic. This will allow us to migrate to the
multi-stack hkbd and hms instead of relying on the older ukbd and ums which only work with
USB.

manu updated this revision to Diff 140158.Jun 24 2024, 4:59 AM

manu retitled this revision from conf: Add usbhid and hidbus to GENERIC* kernel configs to conf: Add hkbd and hms to GENERIC* kernel configs.

manu edited the summary of this revision. (Show Details)

This revision now requires review to proceed.Jun 24 2024, 4:59 AM

This revision was not accepted when it landed; it landed in state Needs Review.Jul 10 2024, 6:06 AM

Closed by commit rG13d00a43cba4: conf: Add usbhid and hidbus to GENERIC* kernel configs (authored by manu). · Explain Why

This revision was automatically updated to reflect the committed changes.

manu added a commit: rG13d00a43cba4: conf: Add usbhid and hidbus to GENERIC* kernel configs.

manu added a commit: rG8e908fab6c5b: conf: Add usbhid and hidbus to GENERIC* kernel configs.Jul 29 2024, 4:37 PM

Revision Contents
Changeset List

Path

Size

sys/

amd64/

conf/

GENERIC

5 lines

arm/

conf/

GENERIC

4 lines

RPI-B

1 line

TEGRA124

4 lines

arm64/

conf/

std.dev

5 lines

i386/

conf/

GENERIC

5 lines

powerpc/

conf/

8 lines

8 lines

8 lines

4 lines

4 lines

4 lines

riscv/

conf/

GENERIC

3 lines

Diff 140749

View Options

sys/amd64/conf/GENERIC

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines

View Options

sys/arm/conf/GENERIC

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines

View Options

sys/arm/conf/RPI-B

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines

View Options

sys/arm/conf/TEGRA124

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines