Re: [PATCH] net: usbnet: allow overriding of default USB interface naming

[Jonathan Davies]

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@xxxxxxxxxxx>
> > > > Suggested-by: Prashanth Sreenivasa <prashanth.sreenivasa@xxxxxxxxxxx>
> > > > ---
> > > >    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