Message-ID: <a620bc87-5ee7-6132-6aa0-6b99e1052960@nutanix.com>
Date:   Mon, 14 Jun 2021 11:58:57 +0100
From:   Jonathan Davies <jonathan.davies@...anix.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Oliver Neukum <oneukum@...e.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        andrew@...n.ch
Subject: Re: [PATCH] net: usbnet: allow overriding of default USB interface
 naming

On 14/06/2021 10:43, Greg KH wrote:
> On Mon, Jun 14, 2021 at 10:32:05AM +0100, Jonathan Davies wrote:
>> On 12/06/2021 08:01, Greg KH wrote:
>>> On Fri, Jun 11, 2021 at 03:23:39PM +0000, Jonathan Davies wrote:
>>>> When the predictable device naming scheme for NICs is not in use, it is
>>>> common for there to be udev rules to rename interfaces to names with
>>>> prefix "eth".
>>>>
>>>> Since the timing at which USB NICs are discovered is unpredictable, it
>>>> can be interfere with udev's attempt to rename another interface to
>>>> "eth0" if a freshly discovered USB interface is initially given the name
>>>> "eth0".
>>>>
>>>> Hence it is useful to be able to override the default name. A new usbnet
>>>> module parameter allows this to be configured.
>>>>
>>>> Signed-off-by: Jonathan Davies <jonathan.davies@...anix.com>
>>>> Suggested-by: Prashanth Sreenivasa <prashanth.sreenivasa@...anix.com>
>>>> ---
>>>>    drivers/net/usb/usbnet.c | 13 ++++++++++---
>>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>> index ecf6284..55f6230 100644
>>>> --- a/drivers/net/usb/usbnet.c
>>>> +++ b/drivers/net/usb/usbnet.c
>>>> @@ -72,6 +72,13 @@ static int msg_level = -1;
>>>>    module_param (msg_level, int, 0);
>>>>    MODULE_PARM_DESC (msg_level, "Override default message level");
>>>> +#define DEFAULT_ETH_DEV_NAME "eth%d"
>>>> +
>>>> +static char *eth_device_name = DEFAULT_ETH_DEV_NAME;
>>>> +module_param(eth_device_name, charp, 0644);
>>>> +MODULE_PARM_DESC(eth_device_name, "Device name pattern for Ethernet devices"
>>>> +				  " (default: \"" DEFAULT_ETH_DEV_NAME "\")");
>>>
>>> This is not the 1990's, please do not add new module parameters as they
>>> are on a global driver level, and not on a device level.
>>
>> The initial name is set at probe-time, so the device doesn't exist yet. So I
>> felt like it was a choice between either changing the hard-coded "eth%d"
>> string or providing a driver-level module parameter. Is there a better
>> alternative?
> 
> This has always been this way, why is this suddenly an issue?  What
> changed to cause the way we can name these devices after they have been
> found like we have been for the past decade+?

The thing that changed for me was that system-udevd does *not* have the 
backoff and retry logic that traditional versions of udev had.

Compare implementations of rename_netif in 
https://git.kernel.org/pub/scm/linux/hotplug/udev.git/tree/src/udev-event.c 
(traditional udev, which handles collisions) and 
https://github.com/systemd/systemd/blob/main/src/udev/udev-event.c 
(systemd-udevd, which does not handle collisions).

I think this logic was removed under the assumption that users of 
systemd-udevd would also use the predictable device naming scheme, 
meaning renames are guaranteed to not collide with devices being probed.

>>> Also changing the way usb network devices are named is up to userspace,
>>> the kernel should not be involved in this.  What is wrong with just
>>> renaming it in userspace as you want to today?
>>
>> Yes, renaming devices is the responsibility of userspace. Normally udev will
>> rename a device shortly after it is probed. But there's a window during
>> which it has the name the kernel initially assigns. If there's other
>> renaming activity happening during that window there's a chance of
>> collisions.
>>
>> Userspace solutions include:
>>   1. udev backing off and retrying in the event of a collision; or
>>   2. avoiding ever renaming a device to a name in the "eth%d" namespace.
> 
> Picking a different namespace does not cause a lack of collisions to
> happen, you could have multiple usb network devices being found at the
> same time, right?
> 
> So no matter what, 1) has to happen.

Within a namespace, the "%d" in "eth%d" means __dev_alloc_name finds a 
name that's not taken. I didn't check the locking but assume that can 
only happen serially, in which case two devices probed in parallel would 
not mutually collide.

So I don't think it's necessarily true that 1) has to happen.

>> Solution 1 is ugly and slow. It's much neater to avoid the collisions in the
>> first place where possible.
> 
> This is not being solved by changing the name as you have to do this no
> matter what.
> 
> And the code and logic in userspace is already there to do this, right?
> This is not a new issue, what changed to cause it to show up for you?

As above, the logic's not there if userspace is using systemd-udevd.

>> Solution 2 arises naturally from use of the predictable device naming
>> scheme. But when userspace is not using that, solution 2 may not apply.
> 
> Again you always have to do 1 no matter what, so might as well just do
> it.
> 
>> Yes, the problem is a result of userspace decisions, but that doesn't mean
>> the kernel can't help make things easier.
> 
> Ideally, if you _can_ do something in userspace, you should, especially
> for policy decisions like naming.  That is why udev was created 17 years
> ago :)

I'm arguing that a bit of flexibility in the kernel can avoid an 
undesirable workaround in userspace. But I can respect the principle you 
describe.

Thanks,
Jonathan

Powered by blists - more mailing lists