Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Nov 2022 19:50:37 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Andy Ren <andy.ren@getcruise.com>
Cc:     netdev@vger.kernel.org, richardbgobert@gmail.com,
        davem@davemloft.net, wsa+renesas@sang-engineering.com,
        edumazet@google.com, petrm@nvidia.com, pabeni@redhat.com,
        corbet@lwn.net, andrew@lunn.ch, dsahern@gmail.com,
        sthemmin@microsoft.com, idosch@idosch.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        roman.gushchin@linux.dev
Subject: Re: [PATCH net-next] net/core: Allow live renaming when an
 interface is up
Message-ID: <20221103195037.13ff8caf@kernel.org>
In-Reply-To: <20221103235847.3919772-1-andy.ren@getcruise.com>
References: <20221103235847.3919772-1-andy.ren@getcruise.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu,  3 Nov 2022 16:58:47 -0700 Andy Ren wrote:
> @@ -1691,7 +1690,6 @@ enum netdev_priv_flags {
>  	IFF_FAILOVER			= 1<<27,
>  	IFF_FAILOVER_SLAVE		= 1<<28,
>  	IFF_L3MDEV_RX_HANDLER		= 1<<29,
> -	IFF_LIVE_RENAME_OK		= 1<<30,

As Stephen says the hole should be somehow noted.
Comment saying what it was, or just a comment saying there 
is a hole that can be reused.

>  	IFF_TX_SKB_NO_LINEAR		= BIT_ULL(31),
>  	IFF_CHANGE_PROTO_DOWN		= BIT_ULL(32),
>  };
> @@ -1726,7 +1724,6 @@ enum netdev_priv_flags {
>  #define IFF_FAILOVER			IFF_FAILOVER
>  #define IFF_FAILOVER_SLAVE		IFF_FAILOVER_SLAVE
>  #define IFF_L3MDEV_RX_HANDLER		IFF_L3MDEV_RX_HANDLER
> -#define IFF_LIVE_RENAME_OK		IFF_LIVE_RENAME_OK
>  #define IFF_TX_SKB_NO_LINEAR		IFF_TX_SKB_NO_LINEAR
>  
>  /* Specifies the type of the struct net_device::ml_priv pointer */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2e4f1c97b59e..a2d650ae15d7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1163,22 +1163,6 @@ int dev_change_name(struct net_device *dev, const char *newname)
>  
>  	net = dev_net(dev);
>  
> -	/* Some auto-enslaved devices e.g. failover slaves are
> -	 * special, as userspace might rename the device after
> -	 * the interface had been brought up and running since
> -	 * the point kernel initiated auto-enslavement. Allow
> -	 * live name change even when these slave devices are
> -	 * up and running.
> -	 *
> -	 * Typically, users of these auto-enslaving devices
> -	 * don't actually care about slave name change, as
> -	 * they are supposed to operate on master interface
> -	 * directly.
> -	 */
> -	if (dev->flags & IFF_UP &&
> -	    likely(!(dev->priv_flags & IFF_LIVE_RENAME_OK)))
> -		return -EBUSY;
> -

Let's leave a hint for potential triage and add something extra to the
netdev_info() print a few lines down in case the interface is renamed
while UP. Perhaps:

	netdev_info(dev, "renamed from %s%s\n", oldname,
		    dev->flags & IFF_UP ? " (while UP)" : "");

or some such.