Messages in this thread | | | Subject | Re: [PATCH] net: usbnet: allow overriding of default USB interface naming | From | Jonathan Davies <> | Date | Mon, 14 Jun 2021 11:58:57 +0100 |
| |
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@nutanix.com> >>>> Suggested-by: Prashanth Sreenivasa <prashanth.sreenivasa@nutanix.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
| |