Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] In the commit "net: change netdev_hw_addr_list count through
helpers", a new comment on struct netdev_hw_addr_list::_count in…
--
net: change netdev_hw_addr_list count through helpers
This patch adds __hw_addr_count_add(), __hw_addr_count_inc(),
__hw_addr_count_dec() and __hw_addr_count_reset() to
net/core/dev_addr_lists.c and uses them for changes to the list count. It
also renames netdev_hw_addr_list.count to _count so that direct writes
stand out. Drivers, mac80211 and the kunit tests that read the field
directly now use the count helpers. This prepares for a later patch that
accounts every change to dev->mc.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d037faff7c44b..97dc053f234cc 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -252,13 +252,14 @@ struct netdev_hw_addr {
>
> struct netdev_hw_addr_list {
> struct list_head list;
> - int count;
> + /* Only changed through the __hw_addr_count_* helpers */
> + int _count;
[ ... ]
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 08528ca0a8b31..23f5db99a702d 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
[ ... ]
> @@ -492,14 +512,14 @@ void __hw_addr_flush(struct netdev_hw_addr_list *list)
[ ... ]
> void __hw_addr_init(struct netdev_hw_addr_list *list)
> {
> INIT_LIST_HEAD(&list->list);
> - list->count = 0;
> + list->_count = 0;
[Severity: Low]
This isn't a bug, but the new comment in struct netdev_hw_addr_list says
_count is "Only changed through the __hw_addr_count_* helpers". However,
__hw_addr_init() still writes it directly, and the write is still there at
the end of the series.
Could the comment name initialization as an exception? For example:
/* Only changed through the __hw_addr_count_* helpers after
* __hw_addr_init()
*/
This has no functional effect on the later dev->mc generation counter
patch. dev_mc_init() calls __hw_addr_init() only from alloc_netdev_mqs(),
on a freshly zeroed device, and dev->mc.owner is set only after that call.
Also, by the end of the series __hw_addr_init() sets list->owner = NULL.
So __hw_addr_changed() would do nothing here even if a helper were used.
A related point: the commit message says readers keep using
netdev_hw_addr_list_count(), but two places in this file still read the
raw field:
__hw_addr_splice():
__hw_addr_count_add(dst, src->_count);
__hw_addr_list_snapshot():
if (cache->_count) {
Should these use netdev_hw_addr_list_count() too, so they follow the
reader convention in the commit message?
> list->tree = RB_ROOT;
> }
> EXPORT_SYMBOL(__hw_addr_init);
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930112842.21323-1-sigefriedhyy%40gmail.com
