Re: [PATCH 01/15] ath9k: fix oops by downgrading assert in rc.c

["Luis R. Rodriguez" <lrodriguez@atheros.com>]

On Mon, Jun 08, 2009 at 05:26:41PM -0700, Luis R. Rodriguez wrote:
> On Fri, Jun 5, 2009 at 11:55 PM, Vasanth
> Thiagarajan<Vasanth.Thiagarajan@atheros.com> wrote:
> >
> > ________________________________________
> >
> >> > >> +       /*
> >> > >> +        * Fine tuning for when no decent rate was found, the
> >> > >> +        * lowest should *not* be used under normal circumstances.
> >> > >> +        */
> >> > >> +       if (rix == ath_rc_priv->valid_rate_index[0]) {
> >> > >> +               DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> >> > >> +                       "disabling MRR\n");
> >> > >> +               rates[0].idx = rate_lowest_index(sband, sta);
> >> > >> +               /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> >> > >> +               rates[1].idx = -1;
> >> > >> +       }
> >> > >
> >> > > I think we can still fill other rates (1..3) with the lowest rate
> >> > > index as we dont differentiate the situation where the lowest rate
> >> > > is chosen truely by the algorithm from this particular case.
> >> >
> >> > I thought about that as well, but does it really make sense for us to
> >> > use MRR with the same lowest rate? That's why I just used one segment.
> >> > Thoughts?
> >>
> >> or we can try for max_retry (4) times. In that case the rate indices of
> >> other rates (just not 1) should be made -1 or this segment should
> >> moved just below the rate find.
> >
> > and the next segment [1]
> > is set to -1. Please let me know if there is anything else you see needs
> > change.
> >
> > Setting rate index of the rate series[1] is not enough as you are still filling the others rate
> > segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
> > set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
> > just below ath_rc_ratefind_ht(), like the following diff.
> >
> >
> >        rate_table = sc->cur_rate_table;
> >        rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
> > +
> > +       if (rix == ath_rc_priv->valid_rate_index[0]) {
> > +               DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > +                               "disabling MRR\n");
> > +
> > +               ath_rc_rate_set_series(rate_table, &rates[0], txrc,
> > +                                      4, rix, 0);
> 
> The above sets the rate[0].idx to rix
> 
> > +               rates[0].idx = rate_lowest_index(sband, sta);
> 
> and then here we set it to rate_lowest_index(sband, sta) comes up
> with. They should be the same, but this just goes to show we need to
> clean this better.

I just found this is wrong too.. the rate table used to find
rix is different than the rate table for the mode. ie, in 5ghz
when associated to an 11n station this could not include any
legacy rates.

  Luis