Paths

Table of Contentst

-
sys/net80211/
-
net80211/
-
ieee80211.c
2/5
ieee80211_crypto.h
2/5
ieee80211_crypto.c
2/5
ieee80211_var.h

Differential D44827

software cipher suites
ClosedPublic
Actions

Authored by adrian on Apr 18 2024, 2:06 AM.

Details

Reviewers

Group Reviewers

wireless

Commits

rGe9961ea16496: net80211: add driver / crypto methods to set the hardware / software cipher…

Summary

Drivers currently announce hardware crypto cipher support by
setting up ic_cryptocaps.

This adds two public function calls:

ieee80211_set_software_cipher_set() - set the software cipher set;
ieee80211_set_hardware_cipher_set() - set the hardware cipher set.

For now these just call into the newly crypto routines to set the ciphers.

This then adds the two crypto routines, similarly named, to set
the hardware/software cipher suite.

This is a no-op right now - wep/tkip/ccmp are already set by default
so drivers aren't required to call these routines for software
encryption, and drivers already set ic_cryptocaps for hardware
encryption.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57480
Build 54368: arc lint + arc unit

Event Timeline

adrian created this revision.Apr 18 2024, 2:06 AM

Herald added subscribers: glebius, melifaro, imp. · View Herald TranscriptApr 18 2024, 2:06 AM

adrian requested review of this revision.Apr 18 2024, 2:06 AM

Harbormaster completed remote builds in B57153: Diff 137274.Apr 18 2024, 2:06 AM

add review

Harbormaster completed remote builds in B57154: Diff 137275.Apr 18 2024, 2:08 AM

adrian retitled this revision from [net80211] add driver / crypto methods to set the hardware / software cipher suites to net80211: add driver / crypto methods to set the hardware / software cipher suites.Apr 18 2024, 2:09 AM

fix commit message

adrian added a parent revision: D44821: wpa: Remove the now not-needed local logic to hard-code cipher support.Apr 18 2024, 2:09 AM

Harbormaster completed remote builds in B57155: Diff 137276.Apr 18 2024, 2:10 AM

adrian added a reviewer: wireless.Apr 18 2024, 4:56 AM

rename software crypto field to better represent what its doing

Harbormaster completed remote builds in B57177: Diff 137316.Apr 18 2024, 5:20 PM

adrian added a child revision: D44864: net80211: add new ciphers and RSN flags.Apr 19 2024, 2:10 PM

Can we shorten hardware/software to hw/sw?

Why do we need wrappers from general code into crypto? Isn't one of them enough to set (and probably also clear[1]) a field in the (currently) ic?

[1] the current API won't really help you for all use cases as at least ath(4) has one "clear bit" case as well. Possibly using a KPI like ifnet does (ptr, set, clear) would help. I am also wondering if we generally want to have the KPI in a way to |= for set an &~ for clear as multiple callers are doing it bit-by-bit.

I think if we were to adjust the few callers then we could hide the field name entirely and solve that naming problem right away as well with adding the 2nd struct field.

sys/net80211/ieee80211_crypto.c
174	I would ask this to be a \|= so current code can continue better but see general comment as to whether you want to pass it differently and also have a clear bit.
184	Same.

bz added inline comments.Apr 19 2024, 7:04 PM

sys/net80211/ieee80211.c
438	s/setup/device attach/ maybe to make it more clear from where we call this normally? There's also VAP setup and all kinds of other bits. (also below)

In D44827#1023017, @bz wrote:

Can we shorten hardware/software to hw/sw?

Why do we need wrappers from general code into crypto? Isn't one of them enough to set (and probably also clear[1]) a field in the (currently) ic?

Just habit. The routines in ieee80211.c are normally the public facing ones. The crypto layer ones know about the crypto stuff in more detail.

[1] the current API won't really help you for all use cases as at least ath(4) has one "clear bit" case as well. Possibly using a KPI like ifnet does (ptr, set, clear) would help. I am also wondering if we generally want to have the KPI in a way to |= for set an &~ for clear as multiple callers are doing it bit-by-bit.

My intent here is that drivers call it to set them explicitly, not mask them. So eg, ath would simply just call it on the set of bits they care about for both hardware and software crypto.
The ath10k driver would call them based on whether software encryption was enabled or disabled at module load time.
Hardware that say, didn't support TKIP at all (hardware or software) would call the registration without those bits set.

I think if we were to adjust the few callers then we could hide the field name entirely and solve that naming problem right away as well with adding the 2nd struct field.

eg ath(4) would do something like:

        uint32_t hw_cryptocaps;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_WEP))
		hw_cryptocaps |= IEEE80211_CRYPTO_WEP;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_AES_OCB))
		hw_cryptocaps |= IEEE80211_CRYPTO_AES_OCB;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_AES_CCM))
		hw_cryptocaps |= IEEE80211_CRYPTO_AES_CCM;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_CKIP))
		hw_cryptocaps |= IEEE80211_CRYPTO_CKIP;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_TKIP)) {
		hw_cryptocaps |= IEEE80211_CRYPTO_TKIP;	
/*
		 * Check if h/w does the MIC and/or whether the
		 * separate key cache entries are required to
		 * handle both tx+rx MIC keys.
		 */
		if (ath_hal_ciphersupported(ah, HAL_CIPHER_MIC))
			hw_cryptocaps |= IEEE80211_CRYPTO_TKIPMIC;
}

ieee80211_set_hardware_cipher_set(hw_cryptocaps);
/* All if_ath(4) HAL providers allow for pass-through unencrypted frames, so
  * software encryption is possible.
 */
ieee80211_set_software_cipher_set(
IEEE80211_CRYPTO_WEP |
 IEEE80211_CRYPTO_AES_OCB |
 IEEE80211_CRYPTO_AES_CCM |
 IEEE80211_CRYPTO_CKIP |
 IEEE80211_CRYPTO_TKIP);

Yes it means that if we add new cipher suites then we will have to add them to each of the drivers, but then by doing this we actually ensure we don't break things by enabling support for drivers when new ciphers (and later new key exchange algorithms) are added without verifying and manually enabling it on each driver.

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

In D44827#1023209, @bz wrote:

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

oops my bad. it'll be a flag for hardware encryption support! I fixed the code snippet.

comments from bz

Harbormaster completed remote builds in B57250: Diff 137446.Apr 20 2024, 7:56 PM

adrian marked an inline comment as done.Apr 20 2024, 7:57 PM

adrian marked an inline comment as done.Apr 20 2024, 8:00 PM

adrian added a project: wireless.Apr 20 2024, 8:44 PM

I find it a little confusing with the verb & noun use of set in set_software_cipher_set - do you think set_software_ciphers (or set_sw_ciphers) is reasonable?

In D44827#1023673, @emaste wrote:

I find it a little confusing with the verb & noun use of set in set_software_cipher_set - do you think set_software_ciphers (or set_sw_ciphers) is reasonable?

I think renaming it to set_software_ciphers() / set_hardware_ciphers() is fine!

comment from emaste

Harbormaster completed remote builds in B57277: Diff 137499.Apr 22 2024, 4:31 PM

In D44827#1023217, @adrian wrote:

In D44827#1023209, @bz wrote:

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

oops my bad. it'll be a flag for hardware encryption support! I fixed the code snippet.

I don't get it. Without exposing the field ic_cryptocaps? And where is the "fixed the code snippet"?

Are you going to change the few drivers which need change to get rid of the public exposure as well? Otherwise this is a dead code before added.

In D44827#1024561, @bz wrote:

Are you going to change the few drivers which need change to get rid of the public exposure as well? Otherwise this is a dead code before added.

Yup a diff later in the stack (D44936) uses it.

In D44827#1024357, @cc wrote:

In D44827#1023217, @adrian wrote:

In D44827#1023209, @bz wrote:

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

oops my bad. it'll be a flag for hardware encryption support! I fixed the code snippet.

I don't get it. Without exposing the field ic_cryptocaps? And where is the "fixed the code snippet"?

oh THAT. yeah, that's dirty and I hate it.

I'll think about it a bit. I may end up just removing doing HW TKIP support for NICs that can't do it with WME because honestly it's 2024 and those NICs, if still being used, would likely be used on something with enough CPU for it not to matter. (And I'd honestly be surprised if someone really complains about the performance change.)

adrian added inline comments.Apr 27 2024, 1:53 AM

sys/net80211/ieee80211_crypto.c
174	It needs to be a set, rather than an or, for platforms that do NOT support ciphers for software encryption.

Just some minor issues.

sys/net80211/ieee80211.c
441–447	The following alignment looks better and it does not break the 80 characters limit: void ieee80211_set_software_ciphers(struct ieee80211com *ic, uint32_t cipher_suite) { ieee80211_crypto_set_supported_software_ciphers(ic, cipher_suite); }
454–459	Same alignment issue as above.
sys/net80211/ieee80211_crypto.c
178	typo: `supporter` => `supported`

This revision now requires changes to proceed.Apr 30 2024, 7:41 PM

address comments by cc@

Harbormaster completed remote builds in B57480: Diff 137920.May 1 2024, 12:37 AM

This still has changes not marked "done".
Is there a change in the stack which does change the drivers to use this?

In D44827#1027553, @bz wrote:

This still has changes not marked "done".
Is there a change in the stack which does change the drivers to use this?

There's no native drivers in our tree right now that require the software cipher call, but in the linux side ath10k will likely require it.
D44936 replaces most of the ic_cryptocaps references to use the hardware cipher call.

adrian marked an inline comment as done.May 3 2024, 4:42 PM

This revision was not accepted when it landed; it landed in state Needs Review.May 9 2024, 12:50 AM

Closed by commit rGe9961ea16496: net80211: add driver / crypto methods to set the hardware / software cipher… (authored by adrian). · Explain Why

This revision was automatically updated to reflect the committed changes.

adrian added a commit: rGe9961ea16496: net80211: add driver / crypto methods to set the hardware / software cipher….

Revision Contents
Changeset List

Path

Size

sys/

net80211/

22 lines

4 lines

21 lines

4 lines

Diff 137920

View Options

sys/net80211/ieee80211.c

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/net80211/ieee80211_crypto.h

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/net80211/ieee80211_crypto.c

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/net80211/ieee80211_var.h

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

net80211: add driver / crypto methods to set the hardware / software cipher suitesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 137920

sys/net80211/ieee80211.c

sys/net80211/ieee80211_crypto.h

sys/net80211/ieee80211_crypto.c

sys/net80211/ieee80211_var.h

net80211: add driver / crypto methods to set the hardware / software cipher suites
ClosedPublic
Actions

Revision Contents
Changeset List