Re: [PATCH net-next v2 3/8] net: s/dev_get_mac_address/netif_get_mac_address/

Stanislav Fomichev
2025-07-02T15:33:38Z

On 07/01, Willem de Bruijn wrote:
> Stanislav Fomichev wrote:
> > On 06/30, Willem de Bruijn wrote:
> > > Stanislav Fomichev wrote:
> > > > Commit cc34acd577f1 ("docs: net: document new locking reality")
> > > > introduced netif_ vs dev_ function semantics: the former expects locked
> > > > netdev, the latter takes care of the locking. We don't strictly
> > > > follow this semantics on either side, but there are more dev_xxx handlers
> > > > now that don't fit. Rename them to netif_xxx where appropriate.
> > > >
> > > > netif_get_mac_address is used only by tun/tap, so move it into
> > > > NETDEV_INTERNAL namespace.
> > > >
> > > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > > ---
> > > > drivers/net/tap.c | 6 ++++--
> > > > drivers/net/tun.c | 4 +++-
> > > > include/linux/netdevice.h | 2 +-
> > > > net/core/dev.c | 4 ++--
> > > > net/core/dev_ioctl.c | 3 ++-
> > > > net/core/net-sysfs.c | 2 +-
> > > > 6 files changed, 13 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> > > > index bdf0788d8e66..4c85770c809b 100644
> > > > --- a/drivers/net/tap.c
> > > > +++ b/drivers/net/tap.c
> > > > @@ -28,6 +28,8 @@
> > > >
> > > > #include "tun_vnet.h"
> > > >
> > > > +MODULE_IMPORT_NS("NETDEV_INTERNAL");
> > > > +
> > > > #define TAP_IFFEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE)
> > > >
> > > > static struct proto tap_proto = {
> > > > @@ -1000,8 +1002,8 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
> > > > return -ENOLINK;
> > > > }
> > > > ret = 0;
> > > > - dev_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> > > > - tap->dev->name);
> > > > + netif_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> > > > + tap->dev->name);
> > > > if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
> > > > copy_to_user(&ifr->ifr_hwaddr, &ss, sizeof(ifr->ifr_hwaddr)))
> > > > ret = -EFAULT;
> > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > > index f8c5e2fd04df..4509ae68decf 100644
> > > > --- a/drivers/net/tun.c
> > > > +++ b/drivers/net/tun.c
> > > > @@ -85,6 +85,8 @@
> > > >
> > > > #include "tun_vnet.h"
> > > >
> > > > +MODULE_IMPORT_NS("NETDEV_INTERNAL");
> > > > +
> > >
> > > Thanks for giving this a go. Now that you've implemented it, does the
> > > risk (of overlooking callers, mainly) indeed seem acceptable?
> > >
> > > Documentation/core-api/symbol-namespaces.rst says
> > >
> > > It is advisable to add the MODULE_IMPORT_NS() statement close to other module
> > > metadata definitions like MODULE_AUTHOR() or MODULE_LICENSE().
> > >
> > > No need to respin just for this from me. Something to consider,
> > > especially if anything else comes up.
> >
> > I put it at the top because it was at the top in bnxt. But it is
> > at the top in bnxt is because the MODULE_LICENSE is there :-(
> > Thanks for pointing it out, I'll definitely address that to be
> > consistent.
> >
> > > Just curious, did you use the modpost and make nsdeps, or was it
> > > sufficient to find the callers with tools like cscope and grep?
> >
> > Only grep. I'm hoping the build bots will tell me if missed something.
>
> SG.
>
> One tradeoff with this series is that renaming and refactoring always
> adds code churn that makes backports (e.g., to stable) more complex.
> I trust that you weighted the pros and cons. We just need to be
> careful to not encourage renaming series in general. Hence calling
> that out right here :)

Yeah, agreed, that why I'm only targeting the core<>driver api boundary.
Which still might be, arguably, too much :-)

> And, it's not trivial to review that the now netif_.. callees indeed
> are holding the netdev locked (or RTNL). Does it make sense to add
> lockdep_rtnl_is_held (or equivalent netdev lock) checks as part of
> this series or follow-up? And the inverse for the dev_.. variants.

Ack, let me double check whether adding more lockdep calls makes sense.
We already have a bunch of them deep down in the call stack,
that might be enough, not sure.

> Aside from this high level points, overall series LGTM, thanks.

Thank you for the review and feedback!