| Message ID | 20250212-arm_fix_selftest-v3-2-72596cb77e44@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: core: improvements to device lookup by hardware address. | expand |
> Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper s/_rtnl// looks like Uday's comment was missed due to the lore issue. From: Breno Leitao <leitao@debian.org> Date: Wed, 12 Feb 2025 09:47:25 -0800 > +/** > + * dev_getbyhwaddr - find a device by its hardware address While at it, could you replace '\t' after '*' to a single '\s' for all kernel-doc comment lines below ? > + * @net: the applicable net namespace > + * @type: media type of device > + * @ha: hardware address > + * > + * Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold > + * rtnl_lock. Otherwise the text here is mis-aligned. $ ./scripts/kernel-doc -man net/core/dev.c | \ scripts/split-man.pl /tmp/man && \ man /tmp/man/dev_getbyhwaddr.9 Also, the latter part should be in Context: Context: rtnl_lock() must be held. See https://docs.kernel.org/doc-guide/kernel-doc.html > + * > + * Return: pointer to the net_device, or NULL if not found > + */ > +struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type, > + const char *ha)
Hello Kuniyuki, On Thu, Feb 13, 2025 at 04:31:29PM +0900, Kuniyuki Iwashima wrote: > > Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper > > s/_rtnl// Ack! > looks like Uday's comment was missed due to the lore issue. hmm, I haven't seen Uday's comment. Didn't it reach lore? > From: Breno Leitao <leitao@debian.org> > Date: Wed, 12 Feb 2025 09:47:25 -0800 > > +/** > > + * dev_getbyhwaddr - find a device by its hardware address > > While at it, could you replace '\t' after '*' to a single '\s' > for all kernel-doc comment lines below ? > > > > + * @net: the applicable net namespace > > + * @type: media type of device > > + * @ha: hardware address > > + * > > + * Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold > > + * rtnl_lock. > > Otherwise the text here is mis-aligned. Sorry, what is misaligned specifically? I generated the documentation, and I can't see it misaligned. This is what I see when generating the document (full log at https://pastebin.mozilla.org/YkotEoHh#L250,271) dev_getbyhwaddr_rcu(9) Kernel Hacker's Manual dev_getbyhwaddr_rcu(9) NAME dev_getbyhwaddr_rcu - find a device by its hardware address SYNOPSIS struct net_device * dev_getbyhwaddr_rcu (struct net *net , unsigned short type , const char *ha ); ARGUMENTS net the applicable net namespace type media type of device ha hardware address Search for an interface by MAC address. Returns NULL if the device is not found or a pointer to the device. The caller must hold RCU. The returned device has not had its ref count increased and the caller must therefore be careful about locking RETURN pointer to the net_device, or NULL if not found dev_getbyhwaddr(9) Kernel Hacker's Manual dev_getbyhwaddr(9) NAME dev_getbyhwaddr - find a device by its hardware address SYNOPSIS struct net_device * dev_getbyhwaddr (struct net *net , unsigned short type , const char *ha ); ARGUMENTS net the applicable net namespace type media type of device ha hardware address Similar to dev_getbyhwaddr_rcu, but the owner needs to hold rtnl_lock. RETURN pointer to the net_device, or NULL if not found > $ ./scripts/kernel-doc -man net/core/dev.c | \ > scripts/split-man.pl /tmp/man && \ > man /tmp/man/dev_getbyhwaddr.9 > > Also, the latter part should be in Context: > > Context: rtnl_lock() must be held. Sure. Should I do something similar for _rcu function as well? Context: caller must hold rcu_read_lock Thanks for the review --breno
From: Breno Leitao <leitao@debian.org> Date: Thu, 13 Feb 2025 02:29:38 -0800 > Hello Kuniyuki, > > On Thu, Feb 13, 2025 at 04:31:29PM +0900, Kuniyuki Iwashima wrote: > > > Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper > > > > s/_rtnl// > > Ack! > > > looks like Uday's comment was missed due to the lore issue. > > hmm, I haven't seen Uday's comment. Didn't it reach lore? I saw the reply and my another one on lore but somehow they were removed :) > > > From: Breno Leitao <leitao@debian.org> > > Date: Wed, 12 Feb 2025 09:47:25 -0800 > > > +/** > > > + * dev_getbyhwaddr - find a device by its hardware address > > > > While at it, could you replace '\t' after '*' to a single '\s' > > for all kernel-doc comment lines below ? > > > > > > > + * @net: the applicable net namespace > > > + * @type: media type of device > > > + * @ha: hardware address > > > + * > > > + * Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold > > > + * rtnl_lock. > > > > Otherwise the text here is mis-aligned. > > Sorry, what is misaligned specifically? I generated the documentation, > and I can't see it misaligned. > > This is what I see when generating the document (full log at > https://pastebin.mozilla.org/YkotEoHh#L250,271) > > > dev_getbyhwaddr_rcu(9) Kernel Hacker's Manual dev_getbyhwaddr_rcu(9) > > NAME > dev_getbyhwaddr_rcu - find a device by its hardware address > > SYNOPSIS > struct net_device * dev_getbyhwaddr_rcu (struct net *net , unsigned short type , const char *ha ); > > ARGUMENTS > net the applicable net namespace > > type media type of device > > ha hardware address > > Search for an interface by MAC address. Returns NULL if the device is not found or a pointer to the device. The caller must hold RCU. The returned device has ^^^^^^ This scentence starts from a weird position when we use '\t' after '*'. You will see it start from the head if '\s' follows '*'. > not had its ref count increased and the caller must therefore be careful about locking > > RETURN > pointer to the net_device, or NULL if not found > > dev_getbyhwaddr(9) Kernel Hacker's Manual dev_getbyhwaddr(9) > > NAME > dev_getbyhwaddr - find a device by its hardware address > > SYNOPSIS > struct net_device * dev_getbyhwaddr (struct net *net , unsigned short type , const char *ha ); > > ARGUMENTS > net the applicable net namespace > > type media type of device > > ha hardware address > > Similar to dev_getbyhwaddr_rcu, but the owner needs to hold rtnl_lock. > > RETURN > pointer to the net_device, or NULL if not found > > > > $ ./scripts/kernel-doc -man net/core/dev.c | \ > > scripts/split-man.pl /tmp/man && \ > > man /tmp/man/dev_getbyhwaddr.9 > > > > Also, the latter part should be in Context: > > > > Context: rtnl_lock() must be held. > > Sure. Should I do something similar for _rcu function as well? > > Context: caller must hold rcu_read_lock Yes, that would be nice. https://lore.kernel.org/netdev/20250213073646.14847-1-kuniyu@amazon.com/
On Thu, Feb 13, 2025 at 07:52:17PM +0900, Kuniyuki Iwashima wrote: > From: Breno Leitao <leitao@debian.org> > Date: Thu, 13 Feb 2025 02:29:38 -0800 > > Hello Kuniyuki, > > > > On Thu, Feb 13, 2025 at 04:31:29PM +0900, Kuniyuki Iwashima wrote: > > > > Subject: [PATCH net-next v3 2/3] net: Add dev_getbyhwaddr_rtnl() helper > > > > > > s/_rtnl// > > > > Ack! > > > > > looks like Uday's comment was missed due to the lore issue. > > > > hmm, I haven't seen Uday's comment. Didn't it reach lore? > > I saw the reply and my another one on lore but somehow they > were removed :) > > > > > > > From: Breno Leitao <leitao@debian.org> > > > Date: Wed, 12 Feb 2025 09:47:25 -0800 > > > > +/** > > > > + * dev_getbyhwaddr - find a device by its hardware address > > > > > > While at it, could you replace '\t' after '*' to a single '\s' > > > for all kernel-doc comment lines below ? > > > > > > > > > > + * @net: the applicable net namespace > > > > + * @type: media type of device > > > > + * @ha: hardware address > > > > + * > > > > + * Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold > > > > + * rtnl_lock. > > > > > > Otherwise the text here is mis-aligned. > > > > Sorry, what is misaligned specifically? I generated the documentation, > > and I can't see it misaligned. > > > > This is what I see when generating the document (full log at > > https://pastebin.mozilla.org/YkotEoHh#L250,271) > > > > > > dev_getbyhwaddr_rcu(9) Kernel Hacker's Manual dev_getbyhwaddr_rcu(9) > > > > NAME > > dev_getbyhwaddr_rcu - find a device by its hardware address > > > > SYNOPSIS > > struct net_device * dev_getbyhwaddr_rcu (struct net *net , unsigned short type , const char *ha ); > > > > ARGUMENTS > > net the applicable net namespace > > > > type media type of device > > > > ha hardware address > > > > Search for an interface by MAC address. Returns NULL if the device is not found or a pointer to the device. The caller must hold RCU. The returned device has > ^^^^^^ > This scentence starts from a weird position when we use '\t' after '*'. > You will see it start from the head if '\s' follows '*'. > Makes sense. All of these functions on that file is misaligned. Having it aligned will add the section under "DESCRIPTION" manpage section.
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 5429581f22995bff639e6962a317adbd0ce30cff..641091c73710f8c4229e76c66f40ede9c235c221 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3270,6 +3270,8 @@ static inline struct net_device *first_net_device_rcu(struct net *net) } int netdev_boot_setup_check(struct net_device *dev); +struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type, + const char *hwaddr); struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type, const char *hwaddr); struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type); diff --git a/net/core/dev.c b/net/core/dev.c index 0b3480a125fcaa6f036ddf219c29fa362ea0cb29..fd15f96cbd8a99da0aa686f22e3e955d179ffc99 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1122,6 +1122,12 @@ int netdev_get_name(struct net *net, char *name, int ifindex) return ret; } +static bool dev_comp_addr(struct net_device *dev, unsigned short type, + const char *ha) +{ + return dev->type == type && !memcmp(dev->dev_addr, ha, dev->addr_len); +} + /** * dev_getbyhwaddr_rcu - find a device by its hardware address * @net: the applicable net namespace @@ -1130,7 +1136,7 @@ int netdev_get_name(struct net *net, char *name, int ifindex) * * Search for an interface by MAC address. Returns NULL if the device * is not found or a pointer to the device. - * The caller must hold RCU or RTNL. + * The caller must hold RCU. * The returned device has not had its ref count increased * and the caller must therefore be careful about locking * @@ -1142,14 +1148,38 @@ struct net_device *dev_getbyhwaddr_rcu(struct net *net, unsigned short type, struct net_device *dev; for_each_netdev_rcu(net, dev) - if (dev->type == type && - !memcmp(dev->dev_addr, ha, dev->addr_len)) + if (dev_comp_addr(dev, type, ha)) return dev; return NULL; } EXPORT_SYMBOL(dev_getbyhwaddr_rcu); +/** + * dev_getbyhwaddr - find a device by its hardware address + * @net: the applicable net namespace + * @type: media type of device + * @ha: hardware address + * + * Similar to dev_getbyhwaddr_rcu(), but the owner needs to hold + * rtnl_lock. + * + * Return: pointer to the net_device, or NULL if not found + */ +struct net_device *dev_getbyhwaddr(struct net *net, unsigned short type, + const char *ha) +{ + struct net_device *dev; + + ASSERT_RTNL(); + for_each_netdev(net, dev) + if (dev_comp_addr(dev, type, ha)) + return dev; + + return NULL; +} +EXPORT_SYMBOL(dev_getbyhwaddr); + struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type) { struct net_device *dev, *ret = NULL;
Add dedicated helper for finding devices by hardware address when holding rtnl_lock, similar to existing dev_getbyhwaddr_rcu(). This prevents PROVE_LOCKING warnings when rtnl_lock is held but RCU read lock is not. Extract common address comparison logic into dev_comp_addr(). The context about this change could be found in the following discussion: Link: https://lore.kernel.org/all/20250206-scarlet-ermine-of-improvement-1fcac5@leitao/ Cc: kuniyu@amazon.com Cc: ushankar@purestorage.com Suggested-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Breno Leitao <leitao@debian.org> --- include/linux/netdevice.h | 2 ++ net/core/dev.c | 36 +++++++++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-)