From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: Regression introduced by "r8169: simplify rtl_set_mac_address" Date: Mon, 2 Jul 2018 22:35:18 +0200 Message-ID: References: <20180702194812.GG3111@calimero.vinschen.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "netdev@vger.kernel.org" To: Corinna Vinschen Return-path: Received: from mail-wm0-f41.google.com ([74.125.82.41]:40533 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752580AbeGBUf2 (ORCPT ); Mon, 2 Jul 2018 16:35:28 -0400 Received: by mail-wm0-f41.google.com with SMTP id z13-v6so65759wma.5 for ; Mon, 02 Jul 2018 13:35:28 -0700 (PDT) In-Reply-To: <20180702194812.GG3111@calimero.vinschen.de> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 02.07.2018 21:48, Corinna Vinschen wrote: > Hi, > > the patch 1f7aa2bc268e, "r8169: simplify rtl_set_mac_address", > introduced a regression found by trying to team a r8169 NIC. > Thanks for reporting! > Try the following (assuming the r8169 NIC is eth0): > > $ nmcli con add type team con-name team0 ifname nm-team config \ > '{"runner": {"name": "lacp"}, "link_watch": {"name": "ethtool"}}' \ > ipv4.method disable ipv6.method ignore > $ nmcli con add type ethernet ifname eth0 con-name team0.0 master nm-team > $ nmcli con up team0.0 > $ teamdctl nm-team port present eth0 > command call failed (No such device) > > Bisecting turned up commit 1f7aa2bc268e, "r8169: simplify > rtl_set_mac_address" as the culprit. Reverting this patch fixes > the issue and the teamdctl call succeeds. > > The reason is apparently the usage of eth_mac_addr here. eth_mac_addr > calls eth_prepare_mac_addr_change which checks for IFF_LIVE_ADDR_CHANGE. > Debugging shows this flag not being set on r8169, thus > eth_prepare_mac_addr_change returns -EBUSY (no idea why userspace claims > "No such device", rather than "Device or resource busy", but that's not > the point here). > > Note that other devices like igb, don't call eth_mac_addr either, but > rather call memcpy by themselves to copy the new MAC, just as the > original r8169 code did, too. Consequentially this problem is not > present on igb. > Doing the memcpy directly in the driver (like it was before) would be a solution, however I'd consider this more a workaround because purpose of the core functions is to encapsulate such details. > I suggest to revert this change in the first place, but I wonder if > we're not just missing to set IFF_LIVE_ADDR_CHANGE in a lot of drivers. > I'd prefer to keep the code and set flag IFF_LIVE_ADDR_CHANGE in the driver. Setting this flag via ethtool --set-priv-flags in theory would also be an option, however the r8169 driver doesn't implement the ethtool_ops set_priv_flags callback. > > Thanks, > Corinna > Rgds, Heiner