Re: [ath9k-devel] [PATCH 1/2] ath9k_htc: add STBC TX support

[Oleksij Rempel <linux@rempel-privat.de>]

Am 04.05.2013 13:16, schrieb Felix Fietkau:
> On 2013-05-04 1:08 PM, Oleksij Rempel wrote:
>> Am 04.05.2013 12:02, schrieb Felix Fietkau:
>>> On 2013-05-04 8:50 AM, Oleksij Rempel wrote:
>>>> Am 02.05.2013 22:15, schrieb Adrian Chadd:
>>>>> Well, let's dig into the firmware a bit more and tidy up how STBC is handled.
>>>>
>>>> Does it mean, i should change this patch and provide a patch for
>>>> firmware too?
>>>> I still do not think, changing peer caps i a good idea in any case.
>>>> I mena this part of patch:
>>>> +               if (sta->ht_cap.cap & IEEE80211_HT_CAP_TX_STBC)
>>>> +                       caps |= WLAN_RC_TX_STBC_FLAG;
>>>>
>>>>
>>>> Behaviour with this patch will be fallowing:
>>>> - peer provide caps, even if it is RX-STBC12
>>>> - we pass this information to firmware and ratecontroller of FW, do
>>>> right decision based on hardware it has.
>>>>
>>>> You suggestion, if i understand it correctly, is to filter caps:
>>>> - if peer provide more than we can, we should tell firmware the value
>>>> what we can. So, if peer say it can RX-STBC12, we should tell firmware
>>>> that peer is RX-STBC1.
>>>> In my opinion, this kind of filter is a source for hidden errors.
>>> I think the discussion regarding RX-STBC12 vs RX-STBC1 is purely
>>> hypothetical. The hardware that this firmware was designed for only
>>> supports sending STBC for MCS0-7. This will not change.
>>>
>>> Also, there's no reason to tell the firmware about both rx and tx STBC
>>> capabilities, the only thing it needs to know is what tells the rate
>>> control part to enable/disable STBC.
>>
>> As you can see, in version 2 of this path there was no more discussion
>> about it. I just did it.
>>
>>> In addition to that, using the WLAN_RC_* flags is wrong, you need to use
>>> the ATH_RC_* flags, as this is what ath_rate_newassoc_11n checks for in
>>> the firmware.
>>
>> Renamed.
>>
>>> The only STBC related flag that actually gets used (when
>>> passed from the driver) is ATH_RC_RX_STBC_FLAG.
>>
>> Well, may be it is not used at the end. But it is present on some places
>> in the firmware.
>> For example here:
>> void
>> rcSibUpdate_11n(struct ath_softc_tgt *sc, struct ath_node_target *pSib,
>>                   A_UINT32 capflag, A_BOOL keepState, struct
>> ieee80211_rate  *pRateSet)
>> {
>>           rcSibUpdate_ht(sc,
>>                          pSib,
>>                          ((capflag & ATH_RC_DS_FLAG)   ? WLAN_RC_DS_FLAG
>>    : 0) |
>>                          ((capflag & ATH_RC_HT40_SGI_FLAG)  ?
>> WLAN_RC_HT40_SGI_FLAG : 0) |
>>                          ((capflag & ATH_RC_HT_FLAG)   ? WLAN_RC_HT_FLAG
>>    : 0) |
>>                          ((capflag & ATH_RC_CW40_FLAG) ? WLAN_RC_40_FLAG
>>    : 0) |
>>                          ((capflag & ATH_RC_TX_STBC_FLAG)   ?
>> WLAN_RC_STBC_FLAG  : 0),
>>                          keepState,
>>                          pRateSet);
>>
>>
>>
>> So, should i remove ATH_RC_TX_STBC_FLAG from my patch?
> I extensively reviewed this part, and it's really crazy. Here's what
> happens:
>
> ath_rate_newassoc_11n takes ATH_RC_* flags, converts them to WLAN_RC_*.
> rcSibUpdate_11n interprets the WLAN_RC_* flags as ATH_RC_* and converts
> them to WLAN_RC_* again. For most flags this is pretty much a no-op
> because the definitions are identical.
> For STBC the result 'accidentally' still contains WLAN_RC_STBC_FLAG, but
> only because ath_rate_newassoc_11n converts ATH_RC_RX_STBC_FLAG to
> WLAN_RC_STBC_FLAG and WLAN_RC_STBC_FLAG overlaps with ATH_RC_TX_STBC_FLAG.
> In the end it doesn't matter anymore, because nothing in the code takes
> the STBC info from the capflags.
>
> STBC is used if ATH_NODE_ATHEROS(an)->stbc is non-zero, and this gets
> set by ath_rate_newassoc_11n before all of those incredibly moronic
> conversions happen.

Ok, thx.

I'll remove it from my patch. And will remove it from firmware.
Even if you wont to remove bigger part of firmware, i thing it wont 
happen this year?


-- 
Regards,
Oleksij