| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8856b29f14de96e1b1cce3ad8b995bc2c3d962a7.camel@gmail.com>
Date: Tue, 13 Dec 2022 09:15:38 -0800
From: Alexander H Duyck <alexander.duyck@...il.com>
To: Jiawen Wu <jiawenwu@...stnetic.com>, netdev@...r.kernel.org,
mengyuanlou@...-swift.com
Subject: Re: [PATCH net-next] net: wangxun: Adjust code structure
On Tue, 2022-12-13 at 14:35 +0800, Jiawen Wu wrote:
> From: Mengyuan Lou <mengyuanlou@...-swift.com>
>
> Remove useless structs 'txgbe_hw' and 'ngbe_hw' make the codes clear.
> And move the same codes which sets MAC address between txgbe and ngbe to
> libwx.
As a general rule you want to avoid having patches with "and" in them
describing what it does. If you need to take care of something else you
should split it into a separate patch as it makes it easier to review.
Specifically it might be easier to read these changes if this was split
over 2 to 3 patches, one for the MAC address handling changes, one for
the removal of the two structures, and maybe one more for the move of
the defines into the _type.h files.
>
> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>
> Signed-off-by: Jiawen Wu <jiawenwu@...stnetic.com>
> ---
> drivers/net/ethernet/wangxun/libwx/wx_hw.c | 123 ++++++++++++-
> drivers/net/ethernet/wangxun/libwx/wx_hw.h | 5 +-
> drivers/net/ethernet/wangxun/libwx/wx_type.h | 12 ++
> drivers/net/ethernet/wangxun/ngbe/ngbe.h | 79 --------
> drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c | 21 +--
> drivers/net/ethernet/wangxun/ngbe/ngbe_hw.h | 4 +-
> drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 127 ++++---------
> drivers/net/ethernet/wangxun/ngbe/ngbe_type.h | 59 +++++-
> drivers/net/ethernet/wangxun/txgbe/txgbe.h | 43 -----
> drivers/net/ethernet/wangxun/txgbe/txgbe_hw.c | 36 ++--
> drivers/net/ethernet/wangxun/txgbe/txgbe_hw.h | 6 +-
> .../net/ethernet/wangxun/txgbe/txgbe_main.c | 174 +++---------------
> .../net/ethernet/wangxun/txgbe/txgbe_type.h | 22 ++-
> 13 files changed, 305 insertions(+), 406 deletions(-)
> delete mode 100644 drivers/net/ethernet/wangxun/ngbe/ngbe.h
> delete mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe.h
>
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index c57dc3238b3f..205620a1c13b 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> @@ -2,6 +2,7 @@
> /* Copyright (c) 2015 - 2022 Beijing WangXun Technology Co., Ltd. */
>
> #include <linux/etherdevice.h>
> +#include <linux/netdevice.h>
> #include <linux/if_ether.h>
> #include <linux/iopoll.h>
> #include <linux/pci.h>
> @@ -536,8 +537,8 @@ EXPORT_SYMBOL(wx_get_mac_addr);
> *
> * Puts an ethernet address into a receive address register.
> **/
> -int wx_set_rar(struct wx_hw *wxhw, u32 index, u8 *addr, u64 pools,
> - u32 enable_addr)
> +static int wx_set_rar(struct wx_hw *wxhw, u32 index, u8 *addr, u64 pools,
> + u32 enable_addr)
> {
> u32 rar_entries = wxhw->mac.num_rar_entries;
> u32 rar_low, rar_high;
> @@ -581,7 +582,6 @@ int wx_set_rar(struct wx_hw *wxhw, u32 index, u8 *addr, u64 pools,
>
> return 0;
> }
> -EXPORT_SYMBOL(wx_set_rar);
>
> /**
> * wx_clear_rar - Remove Rx address register
> @@ -590,7 +590,7 @@ EXPORT_SYMBOL(wx_set_rar);
> *
> * Clears an ethernet address from a receive address register.
> **/
> -int wx_clear_rar(struct wx_hw *wxhw, u32 index)
> +static int wx_clear_rar(struct wx_hw *wxhw, u32 index)
> {
> u32 rar_entries = wxhw->mac.num_rar_entries;
>
> @@ -618,7 +618,6 @@ int wx_clear_rar(struct wx_hw *wxhw, u32 index)
>
> return 0;
> }
> -EXPORT_SYMBOL(wx_clear_rar);
>
> /**
> * wx_clear_vmdq - Disassociate a VMDq pool index from a rx address
> @@ -722,6 +721,112 @@ void wx_init_rx_addrs(struct wx_hw *wxhw)
> }
> EXPORT_SYMBOL(wx_init_rx_addrs);
>
> +static void wx_sync_mac_table(struct wx_hw *wxhw)
> +{
> + int i;
> +
> + for (i = 0; i < wxhw->mac.num_rar_entries; i++) {
> + if (wxhw->mac_table[i].state & WX_MAC_STATE_MODIFIED) {
> + if (wxhw->mac_table[i].state & WX_MAC_STATE_IN_USE) {
> + wx_set_rar(wxhw, i,
> + wxhw->mac_table[i].addr,
> + wxhw->mac_table[i].pools,
> + WX_PSR_MAC_SWC_AD_H_AV);
> + } else {
> + wx_clear_rar(wxhw, i);
> + }
> + wxhw->mac_table[i].state &= ~(WX_MAC_STATE_MODIFIED);
> + }
> + }
> +}
> +
> +/* this function destroys the first RAR entry */
> +void wx_mac_set_default_filter(struct wx_hw *wxhw, u8 *addr)
> +{
> + memcpy(&wxhw->mac_table[0].addr, addr, ETH_ALEN);
> + wxhw->mac_table[0].pools = 1ULL;
> + wxhw->mac_table[0].state = (WX_MAC_STATE_DEFAULT | WX_MAC_STATE_IN_USE);
> + wx_set_rar(wxhw, 0, wxhw->mac_table[0].addr,
> + wxhw->mac_table[0].pools,
> + WX_PSR_MAC_SWC_AD_H_AV);
> +}
> +EXPORT_SYMBOL(wx_mac_set_default_filter);
> +
> +void wx_flush_sw_mac_table(struct wx_hw *wxhw)
> +{
> + u32 i;
> +
> + for (i = 0; i < wxhw->mac.num_rar_entries; i++) {
> + wxhw->mac_table[i].state |= WX_MAC_STATE_MODIFIED;
> + wxhw->mac_table[i].state &= ~WX_MAC_STATE_IN_USE;
> + memset(wxhw->mac_table[i].addr, 0, ETH_ALEN);
> + wxhw->mac_table[i].pools = 0;
> + }
> + wx_sync_mac_table(wxhw);
> +}
> +EXPORT_SYMBOL(wx_flush_sw_mac_table);
Rather than flushing all of the entries it might make more sense to
only set the STATE_MODIFIED bit for the "IN_USE" entries.
> +
> +static int wx_del_mac_filter(struct wx_hw *wxhw, u8 *addr, u16 pool)
> +{
> + u32 i;
> +
> + if (is_zero_ether_addr(addr))
> + return -EINVAL;
> +
> + /* search table for addr, if found, set to 0 and sync */
> + for (i = 0; i < wxhw->mac.num_rar_entries; i++) {
> + if (ether_addr_equal(addr, wxhw->mac_table[i].addr)) {
> + if (wxhw->mac_table[i].pools & (1ULL << pool)) {
> + wxhw->mac_table[i].state |= WX_MAC_STATE_MODIFIED;
> + wxhw->mac_table[i].state &= ~WX_MAC_STATE_IN_USE;
> + wxhw->mac_table[i].pools &= ~(1ULL << pool);
> + wx_sync_mac_table(wxhw);
> + }
> + return 0;
> + }
> +
> + if (wxhw->mac_table[i].pools != (1 << pool))
> + continue;
> + if (!ether_addr_equal(addr, wxhw->mac_table[i].addr))
> + continue;
> +
> + wxhw->mac_table[i].state |= WX_MAC_STATE_MODIFIED;
> + wxhw->mac_table[i].state &= ~WX_MAC_STATE_IN_USE;
> + memset(wxhw->mac_table[i].addr, 0, ETH_ALEN);
> + wxhw->mac_table[i].pools = 0;
> + wx_sync_mac_table(wxhw);
> + return 0;
> + }
> + return -ENOMEM;
> +}
> +
This function doesn't look right. Aren't the block in the if statement
and the block after the two ifs the same? Seems like this would be an
unreachable code block since if the adresses were equal they would hit
the return 0 in the first if block and never be able to hit the second
one.
> +/**
> + * wx_set_mac - Change the Ethernet Address of the NIC
> + * @netdev: network interface device structure
> + * @p: pointer to an address structure
> + *
> + * Returns 0 on success, negative on failure
> + **/
> +int wx_set_mac(struct net_device *netdev, void *p)
> +{
> + struct wx_hw *wxhw = container_of(&netdev, struct wx_hw, netdev);
> + struct sockaddr *addr = p;
> + int retval;
> +
> + retval = eth_prepare_mac_addr_change(netdev, addr);
> + if (retval)
> + return retval;
> +
> + wx_del_mac_filter(wxhw, wxhw->mac.addr, 0);
> + eth_hw_addr_set(netdev, addr->sa_data);
> + memcpy(wxhw->mac.addr, addr->sa_data, netdev->addr_len);
> +
> + wx_mac_set_default_filter(wxhw, wxhw->mac.addr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(wx_set_mac);
> +
> void wx_disable_rx(struct wx_hw *wxhw)
> {
> u32 pfdtxgswc;
> @@ -929,6 +1034,14 @@ int wx_sw_init(struct wx_hw *wxhw)
> return err;
> }
>
> + wxhw->mac_table = kcalloc(wxhw->mac.num_rar_entries,
> + sizeof(struct wx_mac_addr),
> + GFP_KERNEL);
> + if (!wxhw->mac_table) {
> + wx_err(wxhw, "mac_table allocation failed\n");
> + return -ENOMEM;
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(wx_sw_init);
>
Powered by blists - more mailing lists