diff mbox

PROBLEM: rtlwifi drops most IPv6 packets

Message ID 54E36FB2.1050000@lwfinger.net (mailing list archive)
State RFC
Delegated to: Kalle Valo
Headers show

Commit Message

Larry Finger Feb. 17, 2015, 4:43 p.m. UTC
On 02/17/2015 02:29 AM, Alan Fisher wrote:
> Larry,
>
>> I am guessing that you have an RTL8188CE, which uses rtl8192ce.
>
> Yep, my wireless card is an RTL8188CE
>
>> The purpose of rtl_is_special_data() is to ensure that management packets have
>> the highest probability of being successfully transmitted by sending them at a
>> low rate.
> ...
>> It also occurs to me that mac80211 probably handles this function, and that it
>> may be possible to remove this routine, which is essentially what your
>> workaround does.
> I couldn't find any information on mac80211 treating certain packets (ARP, DHCP,
> etc...) as special. It does seem to handle automatic rate selection, though. I
> would think that would be enough to handle packet loss reasonably well. I
> believe the protocols tested for here all have mechanisms for handling lost
> packets. I also can't find any other 802.11 drivers which try to handle DHCP
> packets in a special way. I think it would be safe to remove this routine. I
> have a patch to do that, if you're okay with that change.

The story is a bit more complicated. These drivers use firmware rate selection, 
not the ones in mac80211. At this point, I would not be comfortable with 
removing the entire routine.
>
> Regarding the patch, this change:
>
> -    } else if (0x86DD == ether_type) {
> -        return true;
>       }
>
> successfully prevents IPv6 packets from being treated as special (and thus
> dropped).
>
> However, this:
> +    if (ETH_P_IP == ether_type || ETH_P_IPV6 == ether_type) {
>           ip = (struct iphdr *)((u8 *)skb->data + offset +
>
> seems to be reading an IPv4 header (struct iphdr) from an IPv6 packet. I believe
> a struct ipv6hdr should be used here.

You are correct. My patch was prepared too hastily.

> If we are to continue handling certain types of packets differently, IPv6
> neighbor solicitation messages (like ARP in IPv4) and IPv6 router discovery
> messages (stateless IPv6 autoconfig, similar to DHCP in IPv4 networks) should
> probably be added to the list to maintain consistency with what is being handled
> for IPv4. These are both variants of ICMPv6 packets, although generally
> transmitting all ICMPv6 packets at the lowest rate is probably a bad idea, as
> ICMP echo is commonly used to measure network performance and should be treated
> the same as normal traffic.

For the moment, I think we need to return false, not true, for all IPv6 packets 
until a more complete solution is found. Does the attached patch fix the problem 
you are seeing? I do not have an IPv6 compliant ISP, thus I cannot do much testing.

Larry

Comments

Alan Fisher Feb. 22, 2015, 10:49 p.m. UTC | #1
On 02/17/2015 08:43 AM, Larry Finger wrote:
> On 02/17/2015 02:29 AM, Alan Fisher wrote:
>> Larry,
>>
>>> I am guessing that you have an RTL8188CE, which uses rtl8192ce.
>>
>> Yep, my wireless card is an RTL8188CE
>>
>>> The purpose of rtl_is_special_data() is to ensure that management 
>>> packets have
>>> the highest probability of being successfully transmitted by sending 
>>> them at a
>>> low rate.
>> ...
>>> It also occurs to me that mac80211 probably handles this function, 
>>> and that it
>>> may be possible to remove this routine, which is essentially what your
>>> workaround does.
>> I couldn't find any information on mac80211 treating certain packets 
>> (ARP, DHCP,
>> etc...) as special. It does seem to handle automatic rate selection, 
>> though. I
>> would think that would be enough to handle packet loss reasonably 
>> well. I
>> believe the protocols tested for here all have mechanisms for 
>> handling lost
>> packets. I also can't find any other 802.11 drivers which try to 
>> handle DHCP
>> packets in a special way. I think it would be safe to remove this 
>> routine. I
>> have a patch to do that, if you're okay with that change.
>
> The story is a bit more complicated. These drivers use firmware rate 
> selection, not the ones in mac80211. At this point, I would not be 
> comfortable with removing the entire routine.
Ok, makes sense.

>> Regarding the patch, this change:
>>
>> -    } else if (0x86DD == ether_type) {
>> -        return true;
>>       }
>>
>> successfully prevents IPv6 packets from being treated as special (and 
>> thus
>> dropped).
>>
>> However, this:
>> +    if (ETH_P_IP == ether_type || ETH_P_IPV6 == ether_type) {
>>           ip = (struct iphdr *)((u8 *)skb->data + offset +
>>
>> seems to be reading an IPv4 header (struct iphdr) from an IPv6 
>> packet. I believe
>> a struct ipv6hdr should be used here.
>
> You are correct. My patch was prepared too hastily.
>
>> If we are to continue handling certain types of packets differently, 
>> IPv6
>> neighbor solicitation messages (like ARP in IPv4) and IPv6 router 
>> discovery
>> messages (stateless IPv6 autoconfig, similar to DHCP in IPv4 
>> networks) should
>> probably be added to the list to maintain consistency with what is 
>> being handled
>> for IPv4. These are both variants of ICMPv6 packets, although generally
>> transmitting all ICMPv6 packets at the lowest rate is probably a bad 
>> idea, as
>> ICMP echo is commonly used to measure network performance and should 
>> be treated
>> the same as normal traffic.
>
> For the moment, I think we need to return false, not true, for all 
> IPv6 packets until a more complete solution is found. Does the 
> attached patch fix the problem you are seeing? I do not have an IPv6 
> compliant ISP, thus I cannot do much testing.
Yes, IPv6 appears to work normally with that patch applied. I recently 
spoke with someone who uses RTL8188 under Linux 3.18, and doesn't see 
any packet loss with special packets. I would guess this is because he 
has a slightly different hardware configuration (different processor, 
etc..).

>
> Larry
>

Thanks,
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: wireless-drivers-next/drivers/net/wireless/rtlwifi/base.c
===================================================================
--- wireless-drivers-next.orig/drivers/net/wireless/rtlwifi/base.c
+++ wireless-drivers-next/drivers/net/wireless/rtlwifi/base.c
@@ -1386,8 +1386,11 @@  u8 rtl_is_special_data(struct ieee80211_
 		}
 
 		return true;
-	} else if (0x86DD == ether_type) {
-		return true;
+	} else if (ETH_P_IPV6 == ether_type) {
+		/* TODO: Handle any IPv6 cases that need special handling.
+		 * For now, always return false
+		 */
+		goto end;
 	}
 
 end: