[PATCH net-next 0/4] net: xilinx: axienet: Multicast fixes and improvements

[PATCH net-next 0/4] net: xilinx: axienet: Multicast fixes and improvements

[Sean Anderson]

This series has a few small patches improving the handling of multicast
addresses. In particular, it makes the driver a whole lot less spammy,
and adjusts things so we aren't in promiscuous mode when we have more
than four multicast addresses (a common occurance on modern systems).

As the hardware has a 4-entry CAM, the ideal method would be to "pack"
multiple addresses into one CAM entry. Something like:

entry.address = address[0] | address[1];
entry.mask = ~(address[0] ^ address[1]);

Which would make the entry match both addresses (along with some others
that would need to be filtered in software).

Mapping addresses to entries in an efficient way is a bit tricky. If
anyone knows of an in-tree example of something like this, I'd be glad
to hear about it.


Sean Anderson (4):
  net: xilinx: axienet: Always disable promiscuous mode
  net: xilinx: axienet: Fix dangling multicast addresses
  net: xilinx: axienet: Don't print if we go into promiscuous mode
  net: xilinx: axienet: Support IFF_ALLMULTI

 drivers/net/ethernet/xilinx/xilinx_axienet.h  |  3 ++
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 52 +++++++++----------
 2 files changed, 29 insertions(+), 26 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next 1/4] net: xilinx: axienet: Always disable promiscuous mode

[Sean Anderson]

If prmiscuous mode is disabled when there are fewer than four multicast
addresses, then it will to be reflected in the hardware. Fix this by
always clearing the promiscuous mode flag even when we program multicast
addresses.

Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index ca04c298daa2..e664611c29cf 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -451,6 +451,10 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 	} else if (!netdev_mc_empty(ndev)) {
 		struct netdev_hw_addr *ha;
 
+		reg = axienet_ior(lp, XAE_FMI_OFFSET);
+		reg &= ~XAE_FMI_PM_MASK;
+		axienet_iow(lp, XAE_FMI_OFFSET, reg);
+
 		i = 0;
 		netdev_for_each_mc_addr(ha, ndev) {
 			if (i >= XAE_MULTICAST_CAM_TABLE_NUM)
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next 2/4] net: xilinx: axienet: Fix dangling multicast addresses

[Sean Anderson]

If a multicast address is removed but there are still some multicast
addresses, that address would remain programmed into the frame filter.
Fix this by explicitly setting the enable bit for each filter.

Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/xilinx_axienet.h  |  1 +
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 21 ++++++++-----------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index 0d5b300107e0..03fef656478e 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -170,6 +170,7 @@
 #define XAE_UAW0_OFFSET		0x00000700 /* Unicast address word 0 */
 #define XAE_UAW1_OFFSET		0x00000704 /* Unicast address word 1 */
 #define XAE_FMI_OFFSET		0x00000708 /* Filter Mask Index */
+#define XAE_FFE_OFFSET		0x0000070C /* Frame Filter Enable */
 #define XAE_AF0_OFFSET		0x00000710 /* Address Filter 0 */
 #define XAE_AF1_OFFSET		0x00000714 /* Address Filter 1 */
 
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index e664611c29cf..1bcabb016ca9 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -433,7 +433,7 @@ static int netdev_set_mac_address(struct net_device *ndev, void *p)
  */
 static void axienet_set_multicast_list(struct net_device *ndev)
 {
-	int i;
+	int i = 0;
 	u32 reg, af0reg, af1reg;
 	struct axienet_local *lp = netdev_priv(ndev);
 
@@ -455,7 +455,6 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 		reg &= ~XAE_FMI_PM_MASK;
 		axienet_iow(lp, XAE_FMI_OFFSET, reg);
 
-		i = 0;
 		netdev_for_each_mc_addr(ha, ndev) {
 			if (i >= XAE_MULTICAST_CAM_TABLE_NUM)
 				break;
@@ -474,6 +473,7 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 			axienet_iow(lp, XAE_FMI_OFFSET, reg);
 			axienet_iow(lp, XAE_AF0_OFFSET, af0reg);
 			axienet_iow(lp, XAE_AF1_OFFSET, af1reg);
+			axienet_iow(lp, XAE_FFE_OFFSET, 1);
 			i++;
 		}
 	} else {
@@ -481,18 +481,15 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 		reg &= ~XAE_FMI_PM_MASK;
 
 		axienet_iow(lp, XAE_FMI_OFFSET, reg);
-
-		for (i = 0; i < XAE_MULTICAST_CAM_TABLE_NUM; i++) {
-			reg = axienet_ior(lp, XAE_FMI_OFFSET) & 0xFFFFFF00;
-			reg |= i;
-
-			axienet_iow(lp, XAE_FMI_OFFSET, reg);
-			axienet_iow(lp, XAE_AF0_OFFSET, 0);
-			axienet_iow(lp, XAE_AF1_OFFSET, 0);
-		}
-
 		dev_info(&ndev->dev, "Promiscuous mode disabled.\n");
 	}
+
+	for (; i < XAE_MULTICAST_CAM_TABLE_NUM; i++) {
+		reg = axienet_ior(lp, XAE_FMI_OFFSET) & 0xFFFFFF00;
+		reg |= i;
+		axienet_iow(lp, XAE_FMI_OFFSET, reg);
+		axienet_iow(lp, XAE_FFE_OFFSET, 0);
+	}
 }
 
 /**
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next 3/4] net: xilinx: axienet: Don't print if we go into promiscuous mode

[Sean Anderson]

A message about being in promiscuous mode is printed every time each
additional multicast address beyond four is added. Suppress this message
like is done in other drivers. And don't set IFF_PROMISC in ndev->flags;
contrary to the comment we don't have to inform the net subsystem.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 1bcabb016ca9..9bcad515f156 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -439,15 +439,9 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 
 	if (ndev->flags & (IFF_ALLMULTI | IFF_PROMISC) ||
 	    netdev_mc_count(ndev) > XAE_MULTICAST_CAM_TABLE_NUM) {
-		/* We must make the kernel realize we had to move into
-		 * promiscuous mode. If it was a promiscuous mode request
-		 * the flag is already set. If not we set it.
-		 */
-		ndev->flags |= IFF_PROMISC;
 		reg = axienet_ior(lp, XAE_FMI_OFFSET);
 		reg |= XAE_FMI_PM_MASK;
 		axienet_iow(lp, XAE_FMI_OFFSET, reg);
-		dev_info(&ndev->dev, "Promiscuous mode enabled.\n");
 	} else if (!netdev_mc_empty(ndev)) {
 		struct netdev_hw_addr *ha;
 
@@ -481,7 +475,6 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 		reg &= ~XAE_FMI_PM_MASK;
 
 		axienet_iow(lp, XAE_FMI_OFFSET, reg);
-		dev_info(&ndev->dev, "Promiscuous mode disabled.\n");
 	}
 
 	for (; i < XAE_MULTICAST_CAM_TABLE_NUM; i++) {
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next 4/4] net: xilinx: axienet: Support IFF_ALLMULTI

[Sean Anderson]

Add support for IFF_ALLMULTI by configuring a single filter to match the
multicast address bit. This allows us to keep promiscuous mode disabled,
even when we have more than four multicast addresses. An even better
solution would be to "pack" addresses into the available CAM registers,
but that can wait for a future series.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/xilinx_axienet.h  |  2 ++
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 34 +++++++++++--------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index 03fef656478e..d1b68a040f5a 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -173,6 +173,8 @@
 #define XAE_FFE_OFFSET		0x0000070C /* Frame Filter Enable */
 #define XAE_AF0_OFFSET		0x00000710 /* Address Filter 0 */
 #define XAE_AF1_OFFSET		0x00000714 /* Address Filter 1 */
+#define XAE_AM0_OFFSET		0x00000750 /* Frame Filter Mask Value Bytes 3-0 */
+#define XAE_AM1_OFFSET		0x00000754 /* Frame Filter Mask Value Bytes 7-4 */
 
 #define XAE_TX_VLAN_DATA_OFFSET 0x00004000 /* TX VLAN data table address */
 #define XAE_RX_VLAN_DATA_OFFSET 0x00008000 /* RX VLAN data table address */
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 9bcad515f156..c420bc753750 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -437,18 +437,27 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 	u32 reg, af0reg, af1reg;
 	struct axienet_local *lp = netdev_priv(ndev);
 
-	if (ndev->flags & (IFF_ALLMULTI | IFF_PROMISC) ||
-	    netdev_mc_count(ndev) > XAE_MULTICAST_CAM_TABLE_NUM) {
-		reg = axienet_ior(lp, XAE_FMI_OFFSET);
+	reg = axienet_ior(lp, XAE_FMI_OFFSET);
+	reg &= ~XAE_FMI_PM_MASK;
+	if (ndev->flags & IFF_PROMISC)
 		reg |= XAE_FMI_PM_MASK;
+	else
+		reg &= ~XAE_FMI_PM_MASK;
+	axienet_iow(lp, XAE_FMI_OFFSET, reg);
+
+	if (ndev->flags & IFF_ALLMULTI ||
+	    netdev_mc_count(ndev) > XAE_MULTICAST_CAM_TABLE_NUM) {
+		reg &= 0xFFFFFF00;
 		axienet_iow(lp, XAE_FMI_OFFSET, reg);
+		axienet_iow(lp, XAE_AF0_OFFSET, 1); /* Multicast bit */
+		axienet_iow(lp, XAE_AF1_OFFSET, 0);
+		axienet_iow(lp, XAE_AM0_OFFSET, 1); /* ditto */
+		axienet_iow(lp, XAE_AM1_OFFSET, 0);
+		axienet_iow(lp, XAE_FFE_OFFSET, 1);
+		i = 1;
 	} else if (!netdev_mc_empty(ndev)) {
 		struct netdev_hw_addr *ha;
 
-		reg = axienet_ior(lp, XAE_FMI_OFFSET);
-		reg &= ~XAE_FMI_PM_MASK;
-		axienet_iow(lp, XAE_FMI_OFFSET, reg);
-
 		netdev_for_each_mc_addr(ha, ndev) {
 			if (i >= XAE_MULTICAST_CAM_TABLE_NUM)
 				break;
@@ -461,24 +470,21 @@ static void axienet_set_multicast_list(struct net_device *ndev)
 			af1reg = (ha->addr[4]);
 			af1reg |= (ha->addr[5] << 8);
 
-			reg = axienet_ior(lp, XAE_FMI_OFFSET) & 0xFFFFFF00;
+			reg &= 0xFFFFFF00;
 			reg |= i;
 
 			axienet_iow(lp, XAE_FMI_OFFSET, reg);
 			axienet_iow(lp, XAE_AF0_OFFSET, af0reg);
 			axienet_iow(lp, XAE_AF1_OFFSET, af1reg);
+			axienet_iow(lp, XAE_AM0_OFFSET, 0xffffffff);
+			axienet_iow(lp, XAE_AM1_OFFSET, 0x0000ffff);
 			axienet_iow(lp, XAE_FFE_OFFSET, 1);
 			i++;
 		}
-	} else {
-		reg = axienet_ior(lp, XAE_FMI_OFFSET);
-		reg &= ~XAE_FMI_PM_MASK;
-
-		axienet_iow(lp, XAE_FMI_OFFSET, reg);
 	}
 
 	for (; i < XAE_MULTICAST_CAM_TABLE_NUM; i++) {
-		reg = axienet_ior(lp, XAE_FMI_OFFSET) & 0xFFFFFF00;
+		reg &= 0xFFFFFF00;
 		reg |= i;
 		axienet_iow(lp, XAE_FMI_OFFSET, reg);
 		axienet_iow(lp, XAE_FFE_OFFSET, 0);
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net-next 1/4] net: xilinx: axienet: Always disable promiscuous mode

[Simon Horman]

On Mon, Aug 12, 2024 at 04:04:34PM -0400, Sean Anderson wrote:
> If prmiscuous mode is disabled when there are fewer than four multicast
> addresses, then it will to be reflected in the hardware. Fix this by
> always clearing the promiscuous mode flag even when we program multicast
> addresses.
> 
> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index ca04c298daa2..e664611c29cf 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -451,6 +451,10 @@ static void axienet_set_multicast_list(struct net_device *ndev)
>  	} else if (!netdev_mc_empty(ndev)) {
>  		struct netdev_hw_addr *ha;
>  
> +		reg = axienet_ior(lp, XAE_FMI_OFFSET);
> +		reg &= ~XAE_FMI_PM_MASK;
> +		axienet_iow(lp, XAE_FMI_OFFSET, reg);
> +

Hi Sean,

I notice that this replicates code in another part of this function.
And that is then factored out into common code as part of the last
patch of this series.

I guess that it is in the wash, but perhaps it would
be nicer to factor out the common promisc mode setting code
as part of this patch.

Otherwise, this LGTM.

>  		i = 0;
>  		netdev_for_each_mc_addr(ha, ndev) {
>  			if (i >= XAE_MULTICAST_CAM_TABLE_NUM)
> -- 
> 2.35.1.1320.gc452695387.dirty
> 
> 

Re: [PATCH net-next 3/4] net: xilinx: axienet: Don't print if we go into promiscuous mode

[Simon Horman]

On Mon, Aug 12, 2024 at 04:04:36PM -0400, Sean Anderson wrote:
> A message about being in promiscuous mode is printed every time each
> additional multicast address beyond four is added. Suppress this message
> like is done in other drivers. And don't set IFF_PROMISC in ndev->flags;
> contrary to the comment we don't have to inform the net subsystem.

Hi Sean,

FWIIW, this feels like two things that could be two patches.

...

Re: [PATCH net-next 2/4] net: xilinx: axienet: Fix dangling multicast addresses

[Simon Horman]

On Mon, Aug 12, 2024 at 04:04:35PM -0400, Sean Anderson wrote:
> If a multicast address is removed but there are still some multicast
> addresses, that address would remain programmed into the frame filter.
> Fix this by explicitly setting the enable bit for each filter.
> 
> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 4/4] net: xilinx: axienet: Support IFF_ALLMULTI

[Simon Horman]

On Mon, Aug 12, 2024 at 04:04:37PM -0400, Sean Anderson wrote:
> Add support for IFF_ALLMULTI by configuring a single filter to match the
> multicast address bit. This allows us to keep promiscuous mode disabled,
> even when we have more than four multicast addresses. An even better
> solution would be to "pack" addresses into the available CAM registers,
> but that can wait for a future series.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

My comment on patch 1/4 notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 1/4] net: xilinx: axienet: Always disable promiscuous mode

[Sean Anderson]

On 8/15/24 10:58, Simon Horman wrote:
> On Mon, Aug 12, 2024 at 04:04:34PM -0400, Sean Anderson wrote:
>> If prmiscuous mode is disabled when there are fewer than four multicast
>> addresses, then it will to be reflected in the hardware. Fix this by
>> always clearing the promiscuous mode flag even when we program multicast
>> addresses.
>> 
>> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index ca04c298daa2..e664611c29cf 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> @@ -451,6 +451,10 @@ static void axienet_set_multicast_list(struct net_device *ndev)
>>  	} else if (!netdev_mc_empty(ndev)) {
>>  		struct netdev_hw_addr *ha;
>>  
>> +		reg = axienet_ior(lp, XAE_FMI_OFFSET);
>> +		reg &= ~XAE_FMI_PM_MASK;
>> +		axienet_iow(lp, XAE_FMI_OFFSET, reg);
>> +
> 
> Hi Sean,
> 
> I notice that this replicates code in another part of this function.
> And that is then factored out into common code as part of the last
> patch of this series.
> 
> I guess that it is in the wash, but perhaps it would
> be nicer to factor out the common promisc mode setting code
> as part of this patch.
> 
> Otherwise, this LGTM.

I thought about doing that, but it would have required changing the
indentation of ~10 lines and I thought it would be easier to review
the patch without that noise.

--Sean

Re: [PATCH net-next 3/4] net: xilinx: axienet: Don't print if we go into promiscuous mode

[Sean Anderson]

On 8/15/24 10:59, Simon Horman wrote:
> On Mon, Aug 12, 2024 at 04:04:36PM -0400, Sean Anderson wrote:
>> A message about being in promiscuous mode is printed every time each
>> additional multicast address beyond four is added. Suppress this message
>> like is done in other drivers. And don't set IFF_PROMISC in ndev->flags;
>> contrary to the comment we don't have to inform the net subsystem.
> 
> Hi Sean,
> 
> FWIIW, this feels like two things that could be two patches.
> 
> ...

OK

Re: [PATCH net-next 1/4] net: xilinx: axienet: Always disable promiscuous mode

[Simon Horman]

On Thu, Aug 15, 2024 at 11:14:41AM -0400, Sean Anderson wrote:
> On 8/15/24 10:58, Simon Horman wrote:
> > On Mon, Aug 12, 2024 at 04:04:34PM -0400, Sean Anderson wrote:
> >> If prmiscuous mode is disabled when there are fewer than four multicast
> >> addresses, then it will to be reflected in the hardware. Fix this by
> >> always clearing the promiscuous mode flag even when we program multicast
> >> addresses.
> >> 
> >> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >> 
> >>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> >> index ca04c298daa2..e664611c29cf 100644
> >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> >> @@ -451,6 +451,10 @@ static void axienet_set_multicast_list(struct net_device *ndev)
> >>  	} else if (!netdev_mc_empty(ndev)) {
> >>  		struct netdev_hw_addr *ha;
> >>  
> >> +		reg = axienet_ior(lp, XAE_FMI_OFFSET);
> >> +		reg &= ~XAE_FMI_PM_MASK;
> >> +		axienet_iow(lp, XAE_FMI_OFFSET, reg);
> >> +
> > 
> > Hi Sean,
> > 
> > I notice that this replicates code in another part of this function.
> > And that is then factored out into common code as part of the last
> > patch of this series.
> > 
> > I guess that it is in the wash, but perhaps it would
> > be nicer to factor out the common promisc mode setting code
> > as part of this patch.
> > 
> > Otherwise, this LGTM.
> 
> I thought about doing that, but it would have required changing the
> indentation of ~10 lines and I thought it would be easier to review
> the patch without that noise.

Sure, I was also wrestling with that in my mind.
I think we can stick with what you have :)

Reviewed-by: Simon Horman <horms@kernel.org>