| Message ID | 20240815193614.4120810-3-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: xilinx: axienet: Multicast fixes and improvements | expand |
On Thu, 15 Aug 2024 15:36:11 -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> Again, I'd go for net. > 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 */ There is a conflict with current net / net-next here, because of 9ff2f816e2aa65ca9, you'll need to rebase / repost (which is why I'm allowing myself the nit picks ;)) > 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; Consider renaming i to addr_cnt ? or addr_num ?
On 8/19/24 21:33, Jakub Kicinski wrote: > On Thu, 15 Aug 2024 15:36:11 -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> > > Again, I'd go for net. > >> 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 */ > > There is a conflict with current net / net-next here, because of > 9ff2f816e2aa65ca9, you'll need to rebase / repost (which is why > I'm allowing myself the nit picks ;)) > >> 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; > > Consider renaming i to addr_cnt ? or addr_num ? Well, this doesn't really have anything to do with addresses. It selects the "Filter Index" (as named by the datasheet) so I'd rename it `filter` if anything. This hardware is actually really unusual since the CAM acts on the entire first 64 bytes of the packet. So you could theoretically filter for the source address too, but I don't know why you'd want to since all you can do is drop the packet. Seems like a big waste of resources to me. --Sean
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); + } } /**
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> --- (no changes since v1) drivers/net/ethernet/xilinx/xilinx_axienet.h | 1 + .../net/ethernet/xilinx/xilinx_axienet_main.c | 21 ++++++++----------- 2 files changed, 10 insertions(+), 12 deletions(-)