Hi,
(quoting repaired)
On Tue, Nov 16, 2010 at 06:36:27PM +0100, Ivo Van Doorn wrote:
> On Nov 16, 2010 6:01 PM, "Helmut Schaa" <[email protected]> wrote:
> > Am Dienstag 16 November 2010 schrieb Johannes Stezenbach:
> >> On Tue, Nov 16, 2010 at 05:42:08PM +0100, Helmut Schaa wrote:
> >> >
> >> > Right. We could of course add a mac80211-queue <-> rt2x00-queue mapping in
> >> > the appropriate places or rename the QID_* identifiers accordingly. Not sure
> >> > if it's worth it ...
> >>
> >> Hm, reducing confusion enhances maintainability.
> >
> > I fully agree. I just meant that there are more severe issues in rt2x00 I plan
> > to work on. But feel free to send a patch :)
>
> But is this a mapping problem, or naming problem? Queue 0 is supposed to be
> the queue with highest priority right?
> So that would inficate a naming problem only
The main source of confusion for me is the implicit conversion
between the queue number 0..3 used by the mac80211 layer
and the enum data_queue_qid used by rt2x00, which assignes a
different _meaning_ to the numbers. IMHO the enum values
should be rearranged. Maybe a comment would be good, too.
And I'm still confused. There is code like e.g.
rt2400pci_kick_tx_queue() which tests against QID_AC_BE etc.,
but does it really the kick the right queue?
IOW, where does the qid passed to it ultimately come from?
rt2x00mac_tx(): qid = skb_get_queue_mapping(skb); i.e. 2 for AC_BE
-> rt2x00queue_get_queue() returns queue #2 which has queue->qid == QID_AC_VI
-> rt2x00queue_write_tx_frame()
-> rt2400pci_kick_tx_queue() is called with qid == QID_AC_VI
Bug?
Thanks
Johannes
On Tue, Nov 16, 2010 at 8:26 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2010-11-16 at 20:23 +0100, Ivo Van Doorn wrote:
>
>> However what I meant, is when skb->priority is 0, must the highest or the lowest
>> priority be assumed? If it is the highest priority, then rt2x00 uses
>> the incorrect
>> naming, and all what is needed is to rename the fields everywhere in rt2x00.
>> However is it is the lowest priority, then the naming is correct, and we must
>> change the meaning, in which case we must rename and meaning.
>
> I'm working on this:
> http://johannes.sipsolutions.net/patches/kernel/all/LATEST/012-mac80211-ac-defines.patch
>
> 0 is highest prio (AC_VO)
Excellent. :)
So that makes the bug in rt2x00 fortunately a naming-only thing.
We don't need to map from mac80211 to rt2x00 values, but we simply need to
search & replace the enumeration names to match the meaning.
Ivo
Hi,
> On Tue, Nov 16, 2010 at 06:36:27PM +0100, Ivo Van Doorn wrote:
>> On Nov 16, 2010 6:01 PM, "Helmut Schaa" <[email protected]> wrote:
>> > Am Dienstag 16 November 2010 schrieb Johannes Stezenbach:
>> >> On Tue, Nov 16, 2010 at 05:42:08PM +0100, Helmut Schaa wrote:
>> >> >
>> >> > Right. We could of course add a mac80211-queue <-> rt2x00-queue mapping in
>> >> > the appropriate places or rename the QID_* identifiers accordingly. Not sure
>> >> > if it's worth it ...
>> >>
>> >> Hm, reducing confusion enhances maintainability.
>> >
>> > I fully agree. I just meant that there are more severe issues in rt2x00 I plan
>> > to work on. But feel free to send a patch :)
>>
>> But is this a mapping problem, or naming problem? Queue 0 is supposed to be
>> the queue with highest priority right?
>> So that would inficate a naming problem only
>
> The main source of confusion for me is the implicit conversion
> between the queue number 0..3 used by the mac80211 layer
> and the enum data_queue_qid used by rt2x00, which assignes a
> different _meaning_ to the numbers. ?IMHO the enum values
> should be rearranged. ?Maybe a comment would be good, too.
>
> And I'm still confused. ?There is code like e.g.
> rt2400pci_kick_tx_queue() which tests against QID_AC_BE etc.,
> but does it really the kick the right queue?
> IOW, where does the qid passed to it ultimately come from?
>
> ?rt2x00mac_tx(): ? qid = skb_get_queue_mapping(skb); ?i.e. 2 for AC_BE
> ?-> rt2x00queue_get_queue() ?returns queue #2 which has queue->qid == QID_AC_VI
> ? ?-> rt2x00queue_write_tx_frame()
> ? ? ?-> rt2400pci_kick_tx_queue() is called with qid == QID_AC_VI
Well it kicks the correct queue (as in the queue which on which entries
has been enqueued).
However what I meant, is when skb->priority is 0, must the highest or the lowest
priority be assumed? If it is the highest priority, then rt2x00 uses
the incorrect
naming, and all what is needed is to rename the fields everywhere in rt2x00.
However is it is the lowest priority, then the naming is correct, and we must
change the meaning, in which case we must rename and meaning.
Ivo
On Tue, 2010-11-16 at 20:23 +0100, Ivo Van Doorn wrote:
> However what I meant, is when skb->priority is 0, must the highest or the lowest
> priority be assumed? If it is the highest priority, then rt2x00 uses
> the incorrect
> naming, and all what is needed is to rename the fields everywhere in rt2x00.
> However is it is the lowest priority, then the naming is correct, and we must
> change the meaning, in which case we must rename and meaning.
I'm working on this:
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/012-mac80211-ac-defines.patch
0 is highest prio (AC_VO)
johannes