Hi,
I've spotted something in the way rt2x00 handles the filter setting flags that
doesn't seem to match the documentation. I suspect it makes no difference in
practice but there is a change needed in that area anyway so I'd like to get
it right in the patch.
The documentation says a driver should clear any flags for data which it can't
provide but makes no mention of doing anything about unrequested data that it
does provide. The current rt2x00 implementation will set the relevant flag if
it can't help providing something that wasn't asked for.
For example rt2x00 devices only have one promiscuous mode that covers traffic
in the same and other BSSes therefore if either of FIF_PROMISC_IN_BSS or
FIF_OTHER_BSS are set then the driver will set both of them in the returned
flags value.
It will also for some devices set the FIF_ALLMULTI flag if mc_count is non
zero.
If this behaviour is considered desirable then I'll keep it working when
making the change but if not I'll remove it.
Thanks
Adam
On Saturday 01 March 2008 00:09, Johannes Berg wrote:
> Hi,
>
> > For example rt2x00 devices only have one promiscuous mode that covers
> > traffic in the same and other BSSes therefore if either of
> > FIF_PROMISC_IN_BSS or FIF_OTHER_BSS are set then the driver will set both
> > of them in the returned flags value.
> >
> > It will also for some devices set the FIF_ALLMULTI flag if mc_count is
> > non zero.
> >
> > If this behaviour is considered desirable then I'll keep it working when
> > making the change but if not I'll remove it.
>
> Interesting. I don't think I have an opinion right now. I wanted to be
> strict about clearing the flags so that you don't end up with a flag
> that we never get traffic for, but I can't imagine any check where you'd
> want to know "do I get traffic XY".
The only way I think it might be useful is if it allows mac80211 to not bother
with checks that it would otherwise do, for example if mac80211 didn't want
to pass multicast packets that were not for us up to the higher stack layers
it would know that if FIF_ALLMULTI got set it needed to do some filtering but
if it wasn't set the hardware had a working multicast address filter.
>
> How do you keep track of that anyway? Say somebody enables
> FIF_PROMISC_IN_BSS and you also set FIF_OTHER_BSS, then when
> FIF_PROMISC_IN_BSS is disabled again FIF_OTHER_BSS should be disabled
> too but how know that it wasn't set in the meantime? I think that says
> that you shouldn't do that...
>
rt2x00 ignores the changed_flags passed from mac80211 and keeps track for
itself of what filter it is applying so it will always recalculate it's own
filter based on total_flags and reconfigure the hardware if it changes. This
does assume that mac80211 recalculates what it wants total_flags to be each
time configure_filter is called rather than just changing the value it got
back last time but that appears to be a valid assumption.
Adam
> > Interesting. I don't think I have an opinion right now. I wanted to be
> > strict about clearing the flags so that you don't end up with a flag
> > that we never get traffic for, but I can't imagine any check where you'd
> > want to know "do I get traffic XY".
>
> The only way I think it might be useful is if it allows mac80211 to not bother
> with checks that it would otherwise do, for example if mac80211 didn't want
> to pass multicast packets that were not for us up to the higher stack layers
> it would know that if FIF_ALLMULTI got set it needed to do some filtering but
> if it wasn't set the hardware had a working multicast address filter.
True, but that particular example is just more code and I don't think
it'd be worth it.
We also should use the flags for monitor flags, i.e. when the user
requests control frames but the hw can't give them, the user should be
able to query the current flags. That works the other way around too, I
guess, if the user requests other-bss frames but the hw can only do that
together with control frames, the user should know that.
> rt2x00 ignores the changed_flags passed from mac80211 and keeps track for
> itself of what filter it is applying so it will always recalculate it's own
> filter based on total_flags and reconfigure the hardware if it changes.
Ok.
> This
> does assume that mac80211 recalculates what it wants total_flags to be each
> time configure_filter is called rather than just changing the value it got
> back last time but that appears to be a valid assumption.
Right, mac80211 does that and it has to anyway because of multiple
virtual interfaces, so that assumption is indeed valid.
If the driver is capable of juggling the flags properly then I'd think
adding flags is fine.
johannes
Hi,
> For example rt2x00 devices only have one promiscuous mode that covers traffic
> in the same and other BSSes therefore if either of FIF_PROMISC_IN_BSS or
> FIF_OTHER_BSS are set then the driver will set both of them in the returned
> flags value.
>
> It will also for some devices set the FIF_ALLMULTI flag if mc_count is non
> zero.
>
> If this behaviour is considered desirable then I'll keep it working when
> making the change but if not I'll remove it.
Interesting. I don't think I have an opinion right now. I wanted to be
strict about clearing the flags so that you don't end up with a flag
that we never get traffic for, but I can't imagine any check where you'd
want to know "do I get traffic XY".
How do you keep track of that anyway? Say somebody enables
FIF_PROMISC_IN_BSS and you also set FIF_OTHER_BSS, then when
FIF_PROMISC_IN_BSS is disabled again FIF_OTHER_BSS should be disabled
too but how know that it wasn't set in the meantime? I think that says
that you shouldn't do that...
johannes