Looks good, minor comment below.
> +void rate_control_get_rate(struct net_device *dev,
> + struct ieee80211_hw_mode *mode, struct sk_buff *skb,
> + struct rate_selection *sel)
> + fc = le16_to_cpu(hdr->frame_control);
> + if ((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
> + (hdr->addr1[0] & 0x01))
Please use is_multicast_ether_addr()
> + /* If a forced rate is in effect, select it. */
> + sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> + if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> + sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> +
> + /* If we haven't found the rate yet, ask the rate control algo. */
> + if (!sel->rate)
> + ref->ops->get_rate(ref->priv, dev, mode, skb, sel);
Maybe after this we should insert
if (unlikely(!sel->rate)) {
WARN_ON(1);
sel->rate = rate_lowest(...);
}
Not sure though.
johannes
> > > + /* If a forced rate is in effect, select it. */
> > > + sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > > + if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> > > + sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> > > +
> > > + /* If we haven't found the rate yet, ask the rate control algo. */
> > > + if (!sel->rate)
> > > + ref->ops->get_rate(ref->priv, dev, mode, skb, sel);
> >
> > Maybe after this we should insert
> >
> > if (unlikely(!sel->rate)) {
> > WARN_ON(1);
> > sel->rate = rate_lowest(...);
> > }
> >
> > Not sure though.
>
> I don't think we need this. Rate control is supposed to select a rate,
> if it doesn't know it can assign a fallback rate itself.
Obviously. I just thought we could protect against buggy rate control
algorithm code that way. Not really necessary though.
johannes
On Mon, 2007-12-17 at 12:46 +0100, Johannes Berg wrote:
> Looks good, minor comment below.
>
> > +void rate_control_get_rate(struct net_device *dev,
> > + struct ieee80211_hw_mode *mode, struct sk_buff *skb,
> > + struct rate_selection *sel)
>
> > + fc = le16_to_cpu(hdr->frame_control);
> > + if ((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
> > + (hdr->addr1[0] & 0x01))
>
> Please use is_multicast_ether_addr()
Sure, we had that before. Seems Stefano gave me an old patch. Sorry
about this.
>
> > + /* If a forced rate is in effect, select it. */
> > + sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > + if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> > + sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> > +
> > + /* If we haven't found the rate yet, ask the rate control algo. */
> > + if (!sel->rate)
> > + ref->ops->get_rate(ref->priv, dev, mode, skb, sel);
>
> Maybe after this we should insert
>
> if (unlikely(!sel->rate)) {
> WARN_ON(1);
> sel->rate = rate_lowest(...);
> }
>
> Not sure though.
I don't think we need this. Rate control is supposed to select a rate,
if it doesn't know it can assign a fallback rate itself.
Mattas
On Tue, 2007-12-18 at 14:17 +0100, Johannes Berg wrote:
> > > > + /* If a forced rate is in effect, select it. */
> > > > + sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > > > + if (sdata->bss && sdata->bss->force_unicast_rateidx > -1)
> > > > + sel->rate = &mode->rates[sdata->bss->force_unicast_rateidx];
> > > > +
> > > > + /* If we haven't found the rate yet, ask the rate control algo. */
> > > > + if (!sel->rate)
> > > > + ref->ops->get_rate(ref->priv, dev, mode, skb, sel);
> > >
> > > Maybe after this we should insert
> > >
> > > if (unlikely(!sel->rate)) {
> > > WARN_ON(1);
> > > sel->rate = rate_lowest(...);
> > > }
> > >
> > > Not sure though.
> >
> > I don't think we need this. Rate control is supposed to select a rate,
> > if it doesn't know it can assign a fallback rate itself.
>
> Obviously. I just thought we could protect against buggy rate control
> algorithm code that way. Not really necessary though.
If it's buggy, it should be fixed. So it's actually better to fail
dramatically ;-)
Mattias