Re: [PATCH v2 05/10] rtlwifi: enable mac80211 fast-tx support

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 1/16/2018 4:45 PM, Kalle Valo wrote:
> <pkshih@realtek.com> writes:
>
>> From: Ping-Ke Shih <pkshih@realtek.com>
>>
>> To enable the mac80211 fast-tx feature, the hw/driver needs to support
>> dynamic power saving and fragmentation. Since our driver does not
>> need to fragment packet into smaller pieces, we just hook an empty
>> callback of set_frag_threshold to avoid fragmentation in mac80211.
>>
>> After this, the mac80211 will not fragment the packets and can transmit
>> them faster by cache the header information.
>>
>> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
>
> [...]
>
>> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
>> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
>> @@ -1001,6 +1001,11 @@ static void rtl_op_sta_statistics(struct ieee80211_hw *hw,
>>   	sinfo->filled = 0;
>>   }
>>
>> +static int rtl_op_set_frag_threshold(struct ieee80211_hw *hw, u32 value)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>>   /*
>>    *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3
>>    *for rtl819x  BE = 0, BK = 1, VI = 2, VO = 3
>> @@ -1906,6 +1911,7 @@ const struct ieee80211_ops rtl_ops = {
>>   	.configure_filter = rtl_op_configure_filter,
>>   	.set_key = rtl_op_set_key,
>>   	.sta_statistics = rtl_op_sta_statistics,
>> +	.set_frag_threshold = rtl_op_set_frag_threshold,
>
> This also looks fishy, I guess there's a good reason why mac80211
> requires set_frag_threshold()? To me this is just a workaround for a
> mac80211 test.

When I saw this flying by I had the same feeling. This is clearly not 
how it was intended although you could interpret the comments of the 
.set_frag_threshold() callback and IEEE80211_HW_SUPPORTS_TX_FRAG. 
However, the fragmentation threshold is a user-configurable stack 
parameter as per the standard. This patch effectively kill that option 
for the user although there may be RF conditions in which fragmentation 
can help. Having the user configure a fragmentation threshold of 2346 
also disables fragmentation and allows mac80211 to use cached fastpath.

In short this should be NACK'ed.

Regards,
Arend