Re: [PATCH 1/2] mac80211: add sta_update_rates callback

Re: [PATCH 1/2] mac80211: add sta_update_rates callback

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

On Thu, Aug 09, 2012 at 08:14:57AM +0200, Johannes Berg wrote:
> On Thu, 2012-08-09 at 03:11 +0200, Antonio Quartulli wrote:
> > some drivers need to be notified in case of rates update. This callback tells
> > the driver that something has been changed in the supported rates set of the
> > station passed as argument and that it needs to update its internal tables
> > 
> > Reported-by: Guido Iribarren <guidoiribarren@buenosaireslibre.org>
> > Tested-by: Guido Iribarren <guidoiribarren@buenosaireslibre.org>
> > Signed-off-by: Antonio Quartulli <ordex@autistici.org>
> > ---
> > 
> > ** This is the first time that I play with trace.h, so it could be the case that
> > I made something completely wrong! Sorry :)
> > 
> > 
> > 
> > 
> >  include/net/mac80211.h    |    4 ++++
> >  net/mac80211/driver-ops.h |   14 ++++++++++++++
> >  net/mac80211/ibss.c       |    4 +++-
> >  net/mac80211/trace.h      |   27 +++++++++++++++++++++++++++
> >  4 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index bb86aa6..c5dc725 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -2284,6 +2284,10 @@ struct ieee80211_ops {
> >  				 struct ieee80211_vif *vif,
> >  				 struct ieee80211_bss_conf *info,
> >  				 u32 changed);
> > +	void (*sta_update_rates)(struct ieee80211_hw *hw,
> > +				 struct ieee80211_sta *sta,
> > +				 struct ieee80211_vif *vif,
> > +				 struct ieee80211_bss_conf *info);
> 
> Passing bss_conf doesn't make a lot of sense, it hasn't changed when the
> rate changes...?

Right. I approached this the other way around..starting from the ath9k_htc
driver and I wrongly exposed useless arguments. Will fix this.

> 
> Also there's already an update call sta_rc_update() so I think you
> should just define a new change flag for that?

mh, at the very beginning I thought it was not correct what you said, but indeed
we should be able to do the job in sta_rc_update().

But then why does the ath9k_htc driver implement ath9k_htc_update_rate() to
update the rate used to talk to the AP? Should it use sta_rc_update() as well?

Cheers,


> 
> johannes

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] mac80211: add sta_update_rates callback

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On Thu, Aug 09, 2012 at 11:32:46AM +0200, Antonio Quartulli wrote:
> On Thu, Aug 09, 2012 at 08:14:57AM +0200, Johannes Berg wrote:
> > Also there's already an update call sta_rc_update() so I think you
> > should just define a new change flag for that?
> 
> mh, at the very beginning I thought it was not correct what you said, but indeed
> we should be able to do the job in sta_rc_update().
> 
> But then why does the ath9k_htc driver implement ath9k_htc_update_rate() to
> update the rate used to talk to the AP? Should it use sta_rc_update() as well?

Well, I am digging into the driver a bit more and I realised that the supported
rate set does not touch the RC at all. The supported rates are only stored in
the device (this is why there is another function for doing that in case of STA
mode) and then the RC will play its game from a different point.

For the reason above sta_rc_update() can't do what we want. At this point I
think we have two options:
1) mac80211 forces this change to be done by sta_rc_update() => ath9k has to
adapt it's implementation to follow the API
2) mac80211 uses another callback (sta_update_rates()) to refresh the supported
rates set.

I think that option 2) is probably the way to go, because the RC stuff is
usually not strictly related to the real device (e.g. ath9k has its own rc
routines shared among ath9k and ath9k_htc but they have different routins to set
the supported_rates set for a station.)

Cheers,

> 
> Cheers,
> 
> 
> > 
> > johannes
> 
> -- 
> Antonio Quartulli
> 
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara



-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] mac80211: add sta_update_rates callback

[Johannes Berg]

On Thu, 2012-08-09 at 11:44 +0200, Antonio Quartulli wrote:
> On Thu, Aug 09, 2012 at 11:32:46AM +0200, Antonio Quartulli wrote:
> > On Thu, Aug 09, 2012 at 08:14:57AM +0200, Johannes Berg wrote:
> > > Also there's already an update call sta_rc_update() so I think you
> > > should just define a new change flag for that?
> > 
> > mh, at the very beginning I thought it was not correct what you said, but indeed
> > we should be able to do the job in sta_rc_update().
> > 
> > But then why does the ath9k_htc driver implement ath9k_htc_update_rate() to
> > update the rate used to talk to the AP? Should it use sta_rc_update() as well?
> 
> Well, I am digging into the driver a bit more and I realised that the supported
> rate set does not touch the RC at all. The supported rates are only stored in
> the device (this is why there is another function for doing that in case of STA
> mode) and then the RC will play its game from a different point.

That's strange, why would the device care about the supported rateset of
an IBSS peer? or does it have some rate control in the device?

> For the reason above sta_rc_update() can't do what we want. At this point I
> think we have two options:
> 1) mac80211 forces this change to be done by sta_rc_update() => ath9k has to
> adapt it's implementation to follow the API
> 2) mac80211 uses another callback (sta_update_rates()) to refresh the supported
> rates set.
> 
> I think that option 2) is probably the way to go, because the RC stuff is
> usually not strictly related to the real device (e.g. ath9k has its own rc
> routines shared among ath9k and ath9k_htc but they have different routins to set
> the supported_rates set for a station.)

But the rate control algorithm is re-inited in this case or something?

johannes

Re: [ath9k-devel] [PATCH 1/2] mac80211: add sta_update_rates callback

[Sujith Manoharan]

Antonio Quartulli wrote:
> mh, at the very beginning I thought it was not correct what you said, but indeed
> we should be able to do the job in sta_rc_update().
> 
> But then why does the ath9k_htc driver implement ath9k_htc_update_rate() to
> update the rate used to talk to the AP? Should it use sta_rc_update() as well?

We didn't have this new callback back then, and sta_rc_update() is sufficient to
fix this. Maybe with a new flag - IEEE80211_RC_BASIC_RATES_CHANGED.

Sujith

Re: [ath9k-devel] [PATCH 1/2] mac80211: add sta_update_rates callback

[Sujith Manoharan]

Sujith Manoharan wrote:
> We didn't have this new callback back then, and sta_rc_update() is sufficient to
> fix this. Maybe with a new flag - IEEE80211_RC_BASIC_RATES_CHANGED.

Um, IEEE80211_RC_SUPP_RATES_CHANGED or something.

Sujith