RE: [PATCH net-next v7 3/7] r8169: add support for phylink

[Javen]

>
>> +     if (jumbo_before != jumbo_after) {
>> +             unsigned long caps =
>> + tp->phylink_config.mac_capabilities;
>> +
>> +             if (jumbo_after)
>> +                     caps &= ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
>> +             else
>> +                     caps |= (MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
>> +
>> +             phylink_update_mac_pause_capabilities(tp->phylink,
>> + caps);
>
>The name suggests it only changes pause. So i would expect the caps
>parameter passed here to only cover MAC_SYM_PAUSE & MAC_ASYM_PAUSE.
>So i don't see a need to look at tp->phylink_config.mac_capabilities.

In fact, I added a strict check inside phylink_update_mac_pause_capabilities() to prevent non-pause bits from being modified.

+	if (mac_pause & ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE)) {
+		phylink_err(pl, "Attempted to dynamically change non-pause MAC capabilities\n");
+		return;
+	}

But I think you are right. I will fix the caller in v2 to only pass the specific MAC_SYM_PAUSE and MAC_ASYM_PAUSE bits, which perfectly matches the function's name and its internal check.

+ if (jumbo_before != jumbo_after) {
+ 	unsigned long mac_pause = 0;
+
+	if (!jumbo_after)
+		mac_pause = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+
+	phylink_update_mac_pause_capabilities(tp->phylink, mac_pause);
+ }

>
>        Andrew