Re: [RFC 00/12] multi-channel support

[Michal Kazior <michal.kazior@tieto.com>]

Johannes Berg wrote:
> On Fri, 2012-03-23 at 15:03 +0100, Michał Kazior wrote:
>
>> What if we create a new structure that would be a channel
>> stream/reservation? Driver could then report multiple such objects.
>> Each such would have a tx queue and it's own locking. Let's assume
>> something like:
>>
>>     struct ieee80211_channel_stream {
>>       struct list_head vif; // list of vifs we're attached to
>>       struct ieee80211_channel *ch;
>>       struct sk_buff_head pending;
>>       int stop_reasons[IEEE80211_MAX_QUEUES];
>>       // .. other stuff
>>       u8 *drv_priv[0] __attribute__((__aligned__((void *))));
>>     };
>
> I'm not sure I see the need to have the driver register these, but it
> seems useful to maintain this abstraction somehow in mac80211.
>
> Due to the interface combinations advertising, we already know (*) what
> is supported by the driver, so there's no need for the driver to
> register this again.
>
> Like I said earlier, I see basically two ways of handling this.
>
> The explicit way would essentially consist of something like this:
>
> struct ieee80211_channel_context {
> 	struct ieee80211_channel *chan;
> 	enum nl80211_channel_type type;
> };
>
> with API to add/remove such contexts from/to the device:
>
> struct ieee80211_ops {
> 	...
> 	void (*add_channel_context)(hw, ctx),
> 	void (*remove_channel_context)(hw, ctx),
> 	void (*change_channel_type)(hw, ctx),
> 	void (*assign_vif_channel_context)(hw, vif, ctx),
> };
>
> The assign function would have rather strange semantics though -- it can
> be called only when the interface is up (added to the driver), but it
> can't be called while the interface is associated, active as an AP, etc.
> EXCEPT ... maybe it can in CSA scenarios. But I said that before: CSA is
> a bit of a mess to handle.

Are we going to assume that "add_channel_context" always succeeds?
So I understand you want to use interface combinations to control when 
these functions are valid to be called.

But can we express the "off-channel only" channel context through 
interface combinations? Is it valid for a driver to say the following?

struct ieee80211_iface_limit limits[] = {
    { .max = 1, .types = BIT(NL80211_IFTYPE_AP) },
    { .max = 1, .types = BIT(NL80211_IFTYPE_STATION) },
    { .max = 1, .types = 0 }
};

struct ieee80211_iface_combination combination = {
   .limits = limits,
   .n_limits = 3,
   .max_interfaces = 2,
   .num_different_channels = 3,
};


You also assume that a driver can do a generic "add_channel_context" 
that would allow it to prepare the device. This may not be true for all 
devices. It is unknown whether this channel context is going to be used 
for off-channel, STA or AP. Especially off-channel is broken this way 
since it isn't bound to vif - in fact it can't be. The same goes for 
CSA. Or maybe I didn't get the idea behind "add_channel_context"?


How about:

struct ieee80211_ops {
   // ..
   void (*changed_channel_type)(hw, ctx);
   void (*detach_channel_context)(ctx);
   void (*attach_channel_context_to_vif)(hw, vif, ctx);
   void (*attach_channel_context_to_hw)(hw, ctx);
};

This way mac80211 immediately states it's intention - and we can use 
"attach_channel_context_to_hw" to do off-channel, and 
"attach_channel_context_to_vif" to do CSA as well as normal interface 
start up. We could maybe split detach_* into two different functions if 
necessary. The "attach_channel_context_to_vif" would be allowed even 
when associated for the purpose of dealing with CSA.

What do you think?


The off-channel flow would be then:

  * hw supports 1 interface on 1 channel
  * we're connected as STA
  1. scan request
  2. stop sta
  3. detach_channel_context(oper) without freeing it of course
  4. create channel context "tmp"
     5. set next scan channel in channel context "tmp"
     6. attach_channel_context_to_hw(tmp)
     7. work
     8. detach_channel_context(tmp)
     9. go to 5 if more work needed
10. detach_channel_context(tmp)
11. attach_channel_context(oper)

Are we okay with this?

Also if the device were to support at least 1 extra channel that can do 
(offloaded) off-channel we wouldn't have to do steps: 2, 3, 11.


> Now that I think about it, the implicit way I previously thought was
> quite a separate mechanism is really very similar: you'd still have to
> have the assign_vif_channel_context function, maybe as part of
> bss_info_changed, but not have explicit add/remove. I think the
> add/remove is useful so let's not consider implicit methods any more :)

I'm unsure about the add/remove. See above. We can't just add a channel 
context without stating the purpose of that channel context. A driver 
might need to know of the purpose beforehand.


> The internal handling in mac80211 might have a linked list of the
> channel contexts, or just a static array of a handful of them, doesn't
> really matter. It would, however, make sure that there aren't more than
> the current interface combination advertised as supported (see (*)
> again.)
>
> Of course mac80211 would also assign the interfaces to channel contexts
> as needed -- if a given interface is already on channel 1, the next one
> wouldn't need a new channel context if it also uses channel 1 and the
> channel types are compatible. But that reminds me -- when we do that we
> may have to update the channel context to change its channel type, so
> add an operation for that.
>
>
> Wrt. off-channel, I'm not sure I've understood you correctly. The
> current off-channel is a very short-lived thing that doesn't really use
> a "complete" channel context. In fact, in our implementation it is
> completely separate since it's temporary (**).

Maybe you're right, but how can we then express off-channel in a neat 
clean way? If we use channel contexts we get software AND hardware 
off-channel (as long as the device supports it) at the same time.

If it's a concern that userspace may want to start a new interface on a 
different channel while off-channel is taking up "a precious channel 
context", we may either do -EBUSY or just sleep a few seconds/defer the 
work if the off-channel is really short-lived.

Also if we consider channel contexts that can't do normal operation at 
all - there wouldn't be such problem since they would never be used for 
anything but off-channel.


>
> As far as scanning is concerned, I'm not really sure. I haven't spent
> much time thinking about it since we have hardware offload for it (and
> also off-channel) -- I'm not sure what a good solution would be.
>
> johannes
>
>
> (*) after we refactor cfg80211_can_change_interface() and export
> cfg80211_get_current_combination() that returns the pointer to the
> combination that's currently in use, or something like that. We'd have
> to figure out if another combination is available that has more channels
> though, if needed.

Is there a lot of work to be done?


> (**) However, we're thinking about implementing a P2P-device context
> that would allow offloading things like periodic listen
> (discoverability) or find (search + listen). That would probably use a
> channel context then.

But that wouldn't need a permanent channel context, would it? It'll 
probably be cheap to attach/detach channel contexts.


 >> Should we add capability
 >> flags for each ieee80211_channel_stream? Or we could add new driver
 >> callback, eg. "set_channel_try" and allow the driver to decide
 >> whether it allows a certain hardware state or not.
 >
 > This is not a fully feasible "stream" anyway. Think of it more like a
 > single scan dwell -- you wouldn't express that as a separate "stream"
 > I think?

Why not? Should we care what vif we're working with, or hw for that 
matter? Off-channel, as well as other operation means we want to do 
something on a certain channel. It shouldn't matter how long we use that 
channel (except if we're borrowing time from another operational mode).


Another question would be if we could do software multi-channel 
operation. Do you think it's even possible and worth exploring?



-- Pozdrawiam / Best regards, Michal Kazior.