atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d

Hia atl1c maintainers!

This cosmetics bothered me for some time: atl1c driver shows unexpanded % in kernel thread names. Looks like a minor bug:

    $ ping -f 172.16.0.1  # host1
    $ top  # host2
    ...
    621 root 20 0 0 0 0 S 11.0 0.0 0:05.01 napi/eth%d-385
    622 root 20 0 0 0 0 S  5.6 0.0 0:02.64 napi/eth%d-386
    ...

Was happening for a few years. Likely not a recent regression.

System:
- linux-5.16.1
- x86_64
- 02:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)

Quoted 1 line

From what I understand thread name comes from somewhere around:

net/core/dev.c:

    int dev_set_threaded(struct net_device *dev, bool threaded)
    ...
        err = napi_kthread_create(napi);
    ...
    static int napi_kthread_create(struct napi_struct *n)
    ...
        n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",

  drivers/net/ethernet/atheros/atl1c/atl1c_main.c:
    static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
    ...
        dev_set_threaded(netdev, true);

  ${somewhere} (not sure where):
    ...
      strcpy(netdev->name, "eth%d");

I was not able to pinpoint where expansion should ideally happen. Looks like many driver do strcpy(netdev->name, "eth%d"); style initialization and almost none call dev_set_threaded(netdev, true);.

Can you help me find it out how it should be fixed?

Quoted 40 lines · Sergei Trofimovich

Hia atl1c maintainers!

This cosmetics bothered me for some time: atl1c driver shows unexpanded % in kernel thread names. Looks like a minor bug:

    $ ping -f 172.16.0.1  # host1
    $ top  # host2
    ...
    621 root 20 0 0 0 0 S 11.0 0.0 0:05.01 napi/eth%d-385
    622 root 20 0 0 0 0 S  5.6 0.0 0:02.64 napi/eth%d-386
    ...

Was happening for a few years. Likely not a recent regression.

System:
- linux-5.16.1
- x86_64
- 02:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)

Quoted 1 line

From what I understand thread name comes from somewhere around:

net/core/dev.c:

    int dev_set_threaded(struct net_device *dev, bool threaded)
    ...
        err = napi_kthread_create(napi);
    ...
    static int napi_kthread_create(struct napi_struct *n)
    ...
        n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",

  drivers/net/ethernet/atheros/atl1c/atl1c_main.c:
    static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
    ...
        dev_set_threaded(netdev, true);

  ${somewhere} (not sure where):
    ...
      strcpy(netdev->name, "eth%d");

Quoted 5 lines

I was not able to pinpoint where expansion should ideally happen. Looks like many driver do strcpy(netdev->name, "eth%d"); style initialization and almost none call dev_set_threaded(netdev, true);.

Can you help me find it out how it should be fixed?

Hi Sergei

This is a fun one.

So, the driver does the usual alloc_etherdev_mq()

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/atheros/atl1c/atl1c_main.c#L2703

which ends up here:

https://elixir.bootlin.com/linux/latest/source/net/ethernet/eth.c#L391

struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
unsigned int rxqs)
{
return alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
ether_setup, txqs, rxqs);
}

So at this point in time, the device has the name "eth%d".

The normal flow is that sometime later in probe, it calls register_netdev().

https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L10454

if you follow that down, you get to: __dev_alloc_name(), which does the expansion of the %d to an actual number:

https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L1087

So between alloc_etherdev_mq() and register_netdev(), the device name is not valid. And as you pointed out, dev_set_threaded() tries to use the name, and is called between these two.

The atl1c driver appears to be the only driver actually doing this. There is a sysfs interface which can call dev_set_threaded(), but the sysfs interface is probably not available until after register_netdev() has given the interface its name.

There is a fix for atl1c. Any time after alloc_etherdev_mq(), the driver can call dev_alloc_name().

So please give this a try. I've not even compile tested it...

iff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c index da595242bc13..983a52f77bda 100644

drivers/net/ethernet/atheros/atl1c/atl1c_main.c +4 -0

@@ -2706,6 +2706,10 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
                goto err_alloc_etherdev;
        }
 
+       err = dev_alloc_name(netdev, netdev->name);
+       if (err < 0)
+               goto err_init_netdev;
+
        err = atl1c_init_netdev(netdev, pdev);
        if (err) {
                dev_err(&pdev->dev, "init netdevice failed\n");

If this works, i can turn it into a real patch submission.

Andrew

Quoted 18 lines

So please give this a try. I've not even compile tested it...

iff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c index da595242bc13..983a52f77bda 100644

drivers/net/ethernet/atheros/atl1c/atl1c_main.c +4 -0

@@ -2706,6 +2706,10 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
                goto err_alloc_etherdev;
        }
 
+       err = dev_alloc_name(netdev, netdev->name);
+       if (err < 0)
+               goto err_init_netdev;
+
        err = atl1c_init_netdev(netdev, pdev);
        if (err) {
                dev_err(&pdev->dev, "init netdevice failed\n");

Quoted 3 lines

If this works, i can turn it into a real patch submission.

Andrew

This may not work right because probe is not called with RTNL. And the alloc_name is using RTNL to prevent two devices from getting the same name.

Quoted 18 lines

Quoted 18 lines

So please give this a try. I've not even compile tested it...

iff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c index da595242bc13..983a52f77bda 100644

drivers/net/ethernet/atheros/atl1c/atl1c_main.c +4 -0

@@ -2706,6 +2706,10 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
                goto err_alloc_etherdev;
        }
 
+       err = dev_alloc_name(netdev, netdev->name);
+       if (err < 0)
+               goto err_init_netdev;
+
        err = atl1c_init_netdev(netdev, pdev);
        if (err) {
                dev_err(&pdev->dev, "init netdevice failed\n");

Quoted 8 lines

Quoted 3 lines

If this works, i can turn it into a real patch submission.

Andrew

This may not work right because probe is not called with RTNL. And the alloc_name is using RTNL to prevent two devices from getting the same name.

Oh, yes. I looked at some of the users. And some do take rtnl before calling it. And some don't!

Looking at register_netdev(), it seems we need something like:

	if (rtnl_lock_killable()) {
	       err = -EINTR;
	       goto err_init_netdev;
	}
	err = dev_alloc_name(netdev, netdev->name);
	rtnl_unlock();
	if (err < 0)
		goto err_init_netdev;

It might also be a good idea to put a ASSERT_RTNL() in __dev_alloc_name() to catch any driver doing this wrong.

Thank you Andrew! I used this second version of your patch against 5.16.1 and it seems to work:

    $ sudo ping -f 172.16.0.1

    613 root 20 0 0 0 0 S 11.0 0.0 0:07.46 napi/eth0-385
    614 root 20 0 0 0 0 R  5.3 0.0 0:03.96 napi/eth0-386

Posting used diff as is just in case:

Tested-by Sergei Trofimovich <[email protected]>

drivers/net/ethernet/atheros/atl1c/atl1c_main.c +0 -0

@@ -2706,6 +2706,15 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto err_alloc_etherdev;

Quoted 15 lines

Oh, yes. I looked at some of the users. And some do take rtnl before calling it. And some don't!

Looking at register_netdev(), it seems we need something like:

	if (rtnl_lock_killable()) {
	       err = -EINTR;
	       goto err_init_netdev;
	}
	err = dev_alloc_name(netdev, netdev->name);
	rtnl_unlock();
	if (err < 0)
		goto err_init_netdev;

Quoted 2 lines

It might also be a good idea to put a ASSERT_RTNL() in __dev_alloc_name() to catch any driver doing this wrong.

Quoted 15 lines

Quoted 15 lines

Oh, yes. I looked at some of the users. And some do take rtnl before calling it. And some don't!

Looking at register_netdev(), it seems we need something like:

	if (rtnl_lock_killable()) {
	       err = -EINTR;
	       goto err_init_netdev;
	}
	err = dev_alloc_name(netdev, netdev->name);
	rtnl_unlock();
	if (err < 0)
		goto err_init_netdev;

Quoted 2 lines

Quoted 2 lines

It might also be a good idea to put a ASSERT_RTNL() in __dev_alloc_name() to catch any driver doing this wrong.

I looked at it some more, and some of the current users. And this does not really work. There is a race condition.

Taking rtnl means you at least get a valid name, while you hold rtnl. But it does not keep track of the name it just gave out. As a result, you can release rtnl, and another device can jump in and be given the same name in register_netdev(). When this driver then calls register_netdev() the core will notice the clash and return -EEXISTS, causing the probe to fail.

There are some drivers which take rtnl and keep it until after calling register_netdevice(), rather than register_netdev(), but this is rather ugly. And there are some drivers which don't take the lock, and just hope they don't hit the race.

Maybe a better fix for this driver is:

From a5fc0e127bdc4b6ba4fb923012729cbf3d529996 Mon Sep 17 00:00:00 2001
From: Andrew Lunn <[email protected]>
Date: Sat, 22 Jan 2022 13:33:58 -0600
Subject: [PATCH] net: ethernet: atl1c: Move dev_set_threaded() after
register_netdev()

dev_set_threaded() creates new kernel threads to perform napi. The threads are given a name based on the interface name. However, the interface is not allocated a name until register_netdev() is called. By moving the call to dev_set_threaded() to later in the probe function, odd thread names like napi/eth%d-385 are avoided.

Signed-off-by Andrew Lunn <[email protected]>

---
 drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/ethernet/atheros/atl1c/atl1c_main.c +3 -1

index da595242bc13..9b8088905946 100644
@@ -2728,7 +2728,7 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
        adapter->mii.mdio_write = atl1c_mdio_write;
        adapter->mii.phy_id_mask = 0x1f;
        adapter->mii.reg_num_mask = MDIO_CTRL_REG_MASK;
-       dev_set_threaded(netdev, true);
+
        for (i = 0; i < adapter->rx_queue_count; ++i)
                netif_napi_add(netdev, &adapter->rrd_ring[i].napi,
                               atl1c_clean_rx, 64);

@@ -2781,6 +2781,8 @@ static int atl1c_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
                goto err_register;
        }
 
+       dev_set_threaded(netdev, true);
+
        cards_found++;
        return 0;
 

Signature · 1 line

2.34.1