From: Andrew Lunn <andrew@lunn•ch>
To: Sergei Trofimovich <slyich@gmail•com>
Cc: Stephen Hemminger <stephen@networkplumber•org>, netdev@vger•kernel.org
Subject: Re: atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d
Date: Sat, 22 Jan 2022 20:40:51 +0100 [thread overview]
Message-ID: <Yexdw8JSiTXtn2Bg@lunn.ch> (raw)
In-Reply-To: <20220122121228.3b73db2a@nz>
> > 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.
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 <andrew@lunn•ch>
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 <andrew@lunn•ch>
---
drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index da595242bc13..9b8088905946 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -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;
--
2.34.1
next prev parent reply other threads:[~2022-01-22 19:45 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-21 21:57 atl1c drivers run 'napi/eth%d-385' named threads with unsubstituted %d Sergei Trofimovich
2022-01-21 23:45 ` Andrew Lunn
2022-01-22 1:03 ` Stephen Hemminger
2022-01-22 1:53 ` Andrew Lunn
2022-01-22 12:12 ` Sergei Trofimovich
2022-01-22 15:54 ` Andrew Lunn
2022-01-22 19:40 ` Andrew Lunn [this message]
2022-01-22 22:01 ` Sergei Trofimovich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yexdw8JSiTXtn2Bg@lunn.ch \
--to=andrew@lunn$(echo .)ch \
--cc=netdev@vger$(echo .)kernel.org \
--cc=slyich@gmail$(echo .)com \
--cc=stephen@networkplumber$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox