[PATCH net-next 1/2] net: enetc: don't depend on system endianness in enetc_set_vlan_ht_filter

[PATCH net-next 1/2] net: enetc: don't depend on system endianness in enetc_set_vlan_ht_filter

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

ENETC has a 64-entry hash table for VLAN RX filtering per Station
Interface, which is accessed through two 32-bit registers: VHFR0 holding
the low portion, and VHFR1 holding the high portion.

The enetc_set_vlan_ht_filter function looks at the pf->vlan_ht_filter
bitmap, which is fundamentally an unsigned long variable, and casts it
to a u32 array of two elements. It puts the first u32 element into VHFR0
and the second u32 element into VHFR1.

It is easy to imagine that this will not work on big endian systems
(although, yes, we have bigger problems, because currently enetc assumes
that the CPU endianness is equal to the controller endianness, aka
little endian - but let's assume that we could add a cpu_to_le32 in
enetc_wd_reg and a le32_to_cpu in enetc_rd_reg).

Let's use lower_32_bits and upper_32_bits which are designed to work
regardless of endianness.

Tested that both the old and the new method produce the same results:

$ ethtool -K eth1 rx-vlan-filter on
$ ip link add link eth1 name eth1.100 type vlan id 100
enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x0 VHFR1 0x20
enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x0 VHFR1 0x20
$ ip link add link eth1 name eth1.101 type vlan id 101
enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x0 VHFR1 0x30
enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x0 VHFR1 0x30
$ ip link add link eth1 name eth1.34 type vlan id 34
enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x0 VHFR1 0x34
enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x0 VHFR1 0x34
$ ip link add link eth1 name eth1.1024 type vlan id 1024
enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x1 VHFR1 0x34
enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x1 VHFR1 0x34

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc_pf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 3a7a9102eccb..9c69ca516192 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -248,10 +248,10 @@ static void enetc_pf_set_rx_mode(struct net_device *ndev)
 }
 
 static void enetc_set_vlan_ht_filter(struct enetc_hw *hw, int si_idx,
-				     u32 *hash)
+				     unsigned long hash)
 {
-	enetc_port_wr(hw, ENETC_PSIVHFR0(si_idx), *hash);
-	enetc_port_wr(hw, ENETC_PSIVHFR1(si_idx), *(hash + 1));
+	enetc_port_wr(hw, ENETC_PSIVHFR0(si_idx), lower_32_bits(hash));
+	enetc_port_wr(hw, ENETC_PSIVHFR1(si_idx), upper_32_bits(hash));
 }
 
 static int enetc_vid_hash_idx(unsigned int vid)
@@ -279,7 +279,7 @@ static void enetc_sync_vlan_ht_filter(struct enetc_pf *pf, bool rehash)
 		}
 	}
 
-	enetc_set_vlan_ht_filter(&pf->si->hw, 0, (u32 *)pf->vlan_ht_filter);
+	enetc_set_vlan_ht_filter(&pf->si->hw, 0, *pf->vlan_ht_filter);
 }
 
 static int enetc_vlan_rx_add_vid(struct net_device *ndev, __be16 prot, u16 vid)
-- 
2.25.1

[PATCH net-next 2/2] net: enetc: don't depend on system endianness in enetc_set_mac_ht_flt

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

When enetc runs out of exact match entries for unicast address
filtering, it switches to an approach based on hash tables, where
multiple MAC addresses might end up in the same bucket.

However, the enetc_set_mac_ht_flt function currently depends on the
system endianness, because it interprets the 64-bit hash value as an
array of two u32 elements. Modify this to use lower_32_bits and
upper_32_bits.

Tested by forcing enetc to go into hash table mode by creating two
macvlan upper interfaces:

ip link add link eno0 address 00:01:02:03:00:00 eno0.0 type macvlan && ip link set eno0.0 up
ip link add link eno0 address 00:01:02:03:00:01 eno0.1 type macvlan && ip link set eno0.1 up

and verified that the same bit values are written to the registers
before and after:

enetc_sync_mac_filters: addr 00:00:80:00:40:10 exact match 0
enetc_sync_mac_filters: addr 00:00:00:00:80:00 exact match 0
enetc_set_mac_ht_flt: hash 0x80008000000000 UMHFR0 0x0 UMHFR1 0x800080

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc_pf.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 9c69ca516192..5e95afd61c87 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -129,16 +129,20 @@ static void enetc_clear_mac_ht_flt(struct enetc_si *si, int si_idx, int type)
 }
 
 static void enetc_set_mac_ht_flt(struct enetc_si *si, int si_idx, int type,
-				 u32 *hash)
+				 unsigned long hash)
 {
 	bool err = si->errata & ENETC_ERR_UCMCSWP;
 
 	if (type == UC) {
-		enetc_port_wr(&si->hw, ENETC_PSIUMHFR0(si_idx, err), *hash);
-		enetc_port_wr(&si->hw, ENETC_PSIUMHFR1(si_idx), *(hash + 1));
+		enetc_port_wr(&si->hw, ENETC_PSIUMHFR0(si_idx, err),
+			      lower_32_bits(hash));
+		enetc_port_wr(&si->hw, ENETC_PSIUMHFR1(si_idx),
+			      upper_32_bits(hash));
 	} else { /* MC */
-		enetc_port_wr(&si->hw, ENETC_PSIMMHFR0(si_idx, err), *hash);
-		enetc_port_wr(&si->hw, ENETC_PSIMMHFR1(si_idx), *(hash + 1));
+		enetc_port_wr(&si->hw, ENETC_PSIMMHFR0(si_idx, err),
+			      lower_32_bits(hash));
+		enetc_port_wr(&si->hw, ENETC_PSIMMHFR1(si_idx),
+			      upper_32_bits(hash));
 	}
 }
 
@@ -182,7 +186,7 @@ static void enetc_sync_mac_filters(struct enetc_pf *pf)
 		if (i == UC)
 			enetc_clear_mac_flt_entry(si, pos);
 
-		enetc_set_mac_ht_flt(si, 0, i, (u32 *)f->mac_hash_table);
+		enetc_set_mac_ht_flt(si, 0, i, *f->mac_hash_table);
 	}
 }
 
-- 
2.25.1

RE: [PATCH net-next 1/2] net: enetc: don't depend on system endianness in enetc_set_vlan_ht_filter

[Claudiu Manoil]

>-----Original Message-----
>From: Vladimir Oltean <olteanv@gmail.com>
>Sent: Wednesday, March 24, 2021 5:45 PM
>To: Jakub Kicinski <kuba@kernel.org>; David S. Miller
><davem@davemloft.net>
>Cc: netdev@vger.kernel.org; Claudiu Manoil <claudiu.manoil@nxp.com>;
>Vladimir Oltean <vladimir.oltean@nxp.com>
>Subject: [PATCH net-next 1/2] net: enetc: don't depend on system
>endianness in enetc_set_vlan_ht_filter
>
>From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
>ENETC has a 64-entry hash table for VLAN RX filtering per Station
>Interface, which is accessed through two 32-bit registers: VHFR0 holding
>the low portion, and VHFR1 holding the high portion.
>
>The enetc_set_vlan_ht_filter function looks at the pf->vlan_ht_filter
>bitmap, which is fundamentally an unsigned long variable, and casts it
>to a u32 array of two elements. It puts the first u32 element into VHFR0
>and the second u32 element into VHFR1.
>
>It is easy to imagine that this will not work on big endian systems
>(although, yes, we have bigger problems, because currently enetc assumes
>that the CPU endianness is equal to the controller endianness, aka
>little endian - but let's assume that we could add a cpu_to_le32 in
>enetc_wd_reg and a le32_to_cpu in enetc_rd_reg).
>
>Let's use lower_32_bits and upper_32_bits which are designed to work
>regardless of endianness.
>
>Tested that both the old and the new method produce the same results:
>
>$ ethtool -K eth1 rx-vlan-filter on
>$ ip link add link eth1 name eth1.100 type vlan id 100
>enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x0 VHFR1 0x20
>enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x0 VHFR1 0x20
>$ ip link add link eth1 name eth1.101 type vlan id 101
>enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x0 VHFR1 0x30
>enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x0 VHFR1 0x30
>$ ip link add link eth1 name eth1.34 type vlan id 34
>enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x0 VHFR1 0x34
>enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x0 VHFR1 0x34
>$ ip link add link eth1 name eth1.1024 type vlan id 1024
>enetc_set_vlan_ht_filter: method 1: si_idx 0 VHFR0 0x1 VHFR1 0x34
>enetc_set_vlan_ht_filter: method 2: si_idx 0 VHFR0 0x1 VHFR1 0x34
>
>Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>

RE: [PATCH net-next 2/2] net: enetc: don't depend on system endianness in enetc_set_mac_ht_flt

[Claudiu Manoil]

>-----Original Message-----
>From: Vladimir Oltean <olteanv@gmail.com>
>Sent: Wednesday, March 24, 2021 5:45 PM
>To: Jakub Kicinski <kuba@kernel.org>; David S. Miller
><davem@davemloft.net>
>Cc: netdev@vger.kernel.org; Claudiu Manoil <claudiu.manoil@nxp.com>;
>Vladimir Oltean <vladimir.oltean@nxp.com>
>Subject: [PATCH net-next 2/2] net: enetc: don't depend on system
>endianness in enetc_set_mac_ht_flt
>
>From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
>When enetc runs out of exact match entries for unicast address
>filtering, it switches to an approach based on hash tables, where
>multiple MAC addresses might end up in the same bucket.
>
>However, the enetc_set_mac_ht_flt function currently depends on the
>system endianness, because it interprets the 64-bit hash value as an
>array of two u32 elements. Modify this to use lower_32_bits and
>upper_32_bits.
>
>Tested by forcing enetc to go into hash table mode by creating two
>macvlan upper interfaces:
>
>ip link add link eno0 address 00:01:02:03:00:00 eno0.0 type macvlan && ip link
>set eno0.0 up
>ip link add link eno0 address 00:01:02:03:00:01 eno0.1 type macvlan && ip link
>set eno0.1 up
>
>and verified that the same bit values are written to the registers
>before and after:
>
>enetc_sync_mac_filters: addr 00:00:80:00:40:10 exact match 0
>enetc_sync_mac_filters: addr 00:00:00:00:80:00 exact match 0
>enetc_set_mac_ht_flt: hash 0x80008000000000 UMHFR0 0x0 UMHFR1
>0x800080
>
>Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>

Re: [PATCH net-next 1/2] net: enetc: don't depend on system endianness in enetc_set_vlan_ht_filter

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Wed, 24 Mar 2021 17:44:54 +0200 you wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> ENETC has a 64-entry hash table for VLAN RX filtering per Station
> Interface, which is accessed through two 32-bit registers: VHFR0 holding
> the low portion, and VHFR1 holding the high portion.
> 
> The enetc_set_vlan_ht_filter function looks at the pf->vlan_ht_filter
> bitmap, which is fundamentally an unsigned long variable, and casts it
> to a u32 array of two elements. It puts the first u32 element into VHFR0
> and the second u32 element into VHFR1.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: enetc: don't depend on system endianness in enetc_set_vlan_ht_filter
    https://git.kernel.org/netdev/net-next/c/110eccdb2469
  - [net-next,2/2] net: enetc: don't depend on system endianness in enetc_set_mac_ht_flt
    https://git.kernel.org/netdev/net-next/c/e366a39208e5

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html