On Tue, 2014-06-03 at 08:15 +0200, Michal Kazior wrote:

Hmm... (my brain..)

Consider the following: 3 ctx, 6 vifs. cannot create more ctx.
ctx1=vif1+vif2, ctx2=vif3+vif4, ctx3=vif5+vif6. ctx1..3 have
chandef1..3.

vif5 wants chandef4 so it allocates ctx4 and sets `replaces` to ctx3.
vif6 wants chandef5 so it allocates ctx5 and sets `replaces` to ctx1.
[ note ctx1 with its assigned vif1 and vif2 ]
vif3 wants chandef5 so it reserves ctx5.
vif4 wants chandef5 so it reserves ctx5.
[ ctx2 could be considered now as "free" and could be set as
`replaces` for ctx5 instead of ctx1 ]

It's pretty far fetched for now I'd say - are there any devices that
support more than 2 channels now?

Not right now, afaik, but in theory I think our device could have such
cases. But it wouldn't have enough vifs ;-)

Somebody is working on ath9k multi-channel though I believe - will be
interesting to see what happens there.

quoted

quoted

+     struct ieee80211_sub_if_data *sdata, *sdata_tmp;
+     struct ieee80211_chanctx *new_ctx = NULL, *ctx, *ctx_tmp;

I prefer if declarations with assignments are in lines of their own, but
maybe it's just me.

Actually fair point. I'm declaring multiple vars here..

I think the above is fine, but if you want to declare on multiple lines
that's OK too. Maybe just separate the one with from the ones without
initialization?

quoted

quoted

+     list_for_each_entry(ctx, &local->chanctx_list, list) {
+             if (!(ctx->replaces && !ctx->replaced_by))
+                     continue;

if (!ctx->replaces || ctx->replaced_by) looks more natural to me. (Same
thing for some other cases in this patch).

I find it easier to reason with my condition. But yeah. Popular vote? :-)

Uh, I find them almost equally unreadable, but then again I have no idea
what "replaces" and "replaced_by" means :-)

*looking*...

Oh, those are pointers. I thought they were bool, reviewing without
paying attention is the one case where using bools as pointers is no
good I guess ;-)

I'm assuming that "ctx->replaces && ctx->replaced_by" is always false,
right? Or are some sort of concurrent replacement chains possible? In
this case though I don't understand the logic at all - shouldn't it be
equivalent to just checking ctx->replaces then?

Maybe for such logic it would be easier to understand to have

enum {
	NOT_AFFECTED,
	WILL_BE_REPLACED,
	IS_REPLACEMENT,
} replacement_state;
struct chanctx *replace_ctx;

then you can replace (pun intended) this check with

	if (ctx->replacement_state != IS_REPLACEMENT)
		continue;

But maybe I'm misunderstanding something entirely.

quoted

Hadn't we decided to disconnect the vifs that didn't follow? Can't
remember anymore. :) But now you're just canceling the whole switch?

I'm not sure myself anymore either. This keeps the current behaviour
(i.e. disconnect interfaces that are switching on failure). Johannes?

I think I'd prefer to disconnect the failing switch ones, rather than
the other ones - seems simpler (since we know which ones are affected)
and easier to understand as it's a more localized operation. Any
arguments for the other way?

quoted

quoted

+     if (!(old_ctx->replaced_by && new_ctx->replaces)) {

Isn't !old->ctx->replaced_by enough here? Do you really care if the
new_ctx will replace something else at this point?

Hmm..

Now that I think this should actually be just:

 if (!new_ctx->replaces) { ... }

Oh ... I think that's what I asked above. Nah. This check actually means
that it *is* possible that replaced_by and replaces are true at the same
time? *confused*

johannes