| Message ID | 20220103131039.3473876-2-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: lan966x: Extend switchdev with mdb support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 68 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Jan 03, 2022 at 02:10:37PM +0100, Horatiu Vultur wrote: > Extend mac functionality with the function lan966x_mac_cpu_copy. This > function adds an entry in the MAC table where a frame is copy to the CPU s/copy/copied/ > but also can be forward on the front ports. s/forward/forwarded/ > This functionality is needed for mdb support. In case the CPU and some > of the front ports subscribe to an IP multicast address. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > .../ethernet/microchip/lan966x/lan966x_mac.c | 27 ++++++++++++++++--- > .../ethernet/microchip/lan966x/lan966x_main.h | 5 ++++ > .../ethernet/microchip/lan966x/lan966x_regs.h | 6 +++++ > 3 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c > index efadb8d326cc..ae3a7a10e0d6 100644 > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c > @@ -68,16 +68,18 @@ static void lan966x_mac_select(struct lan966x *lan966x, > lan_wr(mach, lan966x, ANA_MACHDATA); > } > > -int lan966x_mac_learn(struct lan966x *lan966x, int port, > - const unsigned char mac[ETH_ALEN], > - unsigned int vid, > - enum macaccess_entry_type type) > +static int lan966x_mac_learn_impl(struct lan966x *lan966x, int port, > + bool cpu_copy, > + const unsigned char mac[ETH_ALEN], > + unsigned int vid, > + enum macaccess_entry_type type) In the kernel, the __lan966x_mac_learn naming scheme seems to be more popular than _impl. Also, may I suggest that the "int port" argument may be better named "int pgid"? > { > lan966x_mac_select(lan966x, mac, vid); > > /* Issue a write command */ > lan_wr(ANA_MACACCESS_VALID_SET(1) | > ANA_MACACCESS_CHANGE2SW_SET(0) | > + ANA_MACACCESS_MAC_CPU_COPY_SET(cpu_copy) | > ANA_MACACCESS_DEST_IDX_SET(port) | > ANA_MACACCESS_ENTRYTYPE_SET(type) | > ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_LEARN), > @@ -86,6 +88,23 @@ int lan966x_mac_learn(struct lan966x *lan966x, int port, > return lan966x_mac_wait_for_completion(lan966x); > } > > +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port, > + bool cpu_copy, > + const unsigned char mac[ETH_ALEN], > + unsigned int vid, > + enum macaccess_entry_type type) This doesn't seem to use the "port" argument from any of its call sites. It is always 0. What would it even mean? > +{ > + return lan966x_mac_learn_impl(lan966x, port, cpu_copy, mac, vid, type); > +} > + > +int lan966x_mac_learn(struct lan966x *lan966x, int port, > + const unsigned char mac[ETH_ALEN], > + unsigned int vid, > + enum macaccess_entry_type type) > +{ > + return lan966x_mac_learn_impl(lan966x, port, false, mac, vid, type); If you call lan966x_mac_cpu_copy() on an address and then lan966x_mac_learn() on the same address but on an external port, how does that work (why doesn't the "false" here overwrite the cpu_copy in the previous command, breaking the copy-to-CPU feature)? > +} > + > int lan966x_mac_forget(struct lan966x *lan966x, > const unsigned char mac[ETH_ALEN], > unsigned int vid, > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > index c399b1256edc..a7a2a3537036 100644 > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > @@ -157,6 +157,11 @@ int lan966x_port_pcs_set(struct lan966x_port *port, > struct lan966x_port_config *config); > void lan966x_port_init(struct lan966x_port *port); > > +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port, > + bool cpu_copy, > + const unsigned char mac[ETH_ALEN], > + unsigned int vid, > + enum macaccess_entry_type type); > int lan966x_mac_learn(struct lan966x *lan966x, int port, > const unsigned char mac[ETH_ALEN], > unsigned int vid, > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h > index a13c469e139a..797560172aca 100644 > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h > @@ -169,6 +169,12 @@ enum lan966x_target { > #define ANA_MACACCESS_CHANGE2SW_GET(x)\ > FIELD_GET(ANA_MACACCESS_CHANGE2SW, x) > > +#define ANA_MACACCESS_MAC_CPU_COPY BIT(16) > +#define ANA_MACACCESS_MAC_CPU_COPY_SET(x)\ > + FIELD_PREP(ANA_MACACCESS_MAC_CPU_COPY, x) > +#define ANA_MACACCESS_MAC_CPU_COPY_GET(x)\ > + FIELD_GET(ANA_MACACCESS_MAC_CPU_COPY, x) > + > #define ANA_MACACCESS_VALID BIT(12) > #define ANA_MACACCESS_VALID_SET(x)\ > FIELD_PREP(ANA_MACACCESS_VALID, x) > -- > 2.33.0 >
The 01/03/2022 14:27, Vladimir Oltean wrote: > > On Mon, Jan 03, 2022 at 02:10:37PM +0100, Horatiu Vultur wrote: > > Extend mac functionality with the function lan966x_mac_cpu_copy. This > > function adds an entry in the MAC table where a frame is copy to the CPU > > s/copy/copied/ > > > but also can be forward on the front ports. > > s/forward/forwarded/ > > > This functionality is needed for mdb support. In case the CPU and some > > of the front ports subscribe to an IP multicast address. > > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > --- > > .../ethernet/microchip/lan966x/lan966x_mac.c | 27 ++++++++++++++++--- > > .../ethernet/microchip/lan966x/lan966x_main.h | 5 ++++ > > .../ethernet/microchip/lan966x/lan966x_regs.h | 6 +++++ > > 3 files changed, 34 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c > > index efadb8d326cc..ae3a7a10e0d6 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c > > @@ -68,16 +68,18 @@ static void lan966x_mac_select(struct lan966x *lan966x, > > lan_wr(mach, lan966x, ANA_MACHDATA); > > } > > > > -int lan966x_mac_learn(struct lan966x *lan966x, int port, > > - const unsigned char mac[ETH_ALEN], > > - unsigned int vid, > > - enum macaccess_entry_type type) > > +static int lan966x_mac_learn_impl(struct lan966x *lan966x, int port, > > + bool cpu_copy, > > + const unsigned char mac[ETH_ALEN], > > + unsigned int vid, > > + enum macaccess_entry_type type) > > In the kernel, the __lan966x_mac_learn naming scheme seems to be more > popular than _impl. > > Also, may I suggest that the "int port" argument may be better named > "int pgid"? Yes, I will rename it and change the argument name. > > > { > > lan966x_mac_select(lan966x, mac, vid); > > > > /* Issue a write command */ > > lan_wr(ANA_MACACCESS_VALID_SET(1) | > > ANA_MACACCESS_CHANGE2SW_SET(0) | > > + ANA_MACACCESS_MAC_CPU_COPY_SET(cpu_copy) | > > ANA_MACACCESS_DEST_IDX_SET(port) | > > ANA_MACACCESS_ENTRYTYPE_SET(type) | > > ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_LEARN), > > @@ -86,6 +88,23 @@ int lan966x_mac_learn(struct lan966x *lan966x, int port, > > return lan966x_mac_wait_for_completion(lan966x); > > } > > > > +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port, > > + bool cpu_copy, > > + const unsigned char mac[ETH_ALEN], > > + unsigned int vid, > > + enum macaccess_entry_type type) > > This doesn't seem to use the "port" argument from any of its call sites. > It is always 0. What would it even mean? When adding MAC table entries for IPv4/IPv6 then the port which is the DEST_IDX needs to be 0. It should not point to a PGID entry because the destination ports are encoded in the MAC address. > > > +{ > > + return lan966x_mac_learn_impl(lan966x, port, cpu_copy, mac, vid, type); > > +} > > + > > +int lan966x_mac_learn(struct lan966x *lan966x, int port, > > + const unsigned char mac[ETH_ALEN], > > + unsigned int vid, > > + enum macaccess_entry_type type) > > +{ > > + return lan966x_mac_learn_impl(lan966x, port, false, mac, vid, type); > > If you call lan966x_mac_cpu_copy() on an address and then > lan966x_mac_learn() on the same address but on an external port, how > does that work (why doesn't the "false" here overwrite the cpu_copy in > the previous command, breaking the copy-to-CPU feature)? Then you will overwrite the cpu_copy so the frames will not reach the CPU anymore. But you should not do that. The function lan966x_mac_cpu_copy() should be used for IPv4/IPv6 and lan966x_mac_learn() for the other types. Maybe the function lan966x_mac_cpu_copy() is too generic. It should be something like lan966x_mac_ipv4(), lan966x_mac_ipv6() and these functions will call __lan966x_mac_learn with the correct parameters. Also I can add a WARN_ON(...) inside lan966x_mac_learn not to be used with the IPv4/IPv6 types. > > > +} > > + > > int lan966x_mac_forget(struct lan966x *lan966x, > > const unsigned char mac[ETH_ALEN], > > unsigned int vid, > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > index c399b1256edc..a7a2a3537036 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > @@ -157,6 +157,11 @@ int lan966x_port_pcs_set(struct lan966x_port *port, > > struct lan966x_port_config *config); > > void lan966x_port_init(struct lan966x_port *port); > > > > +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port, > > + bool cpu_copy, > > + const unsigned char mac[ETH_ALEN], > > + unsigned int vid, > > + enum macaccess_entry_type type); > > int lan966x_mac_learn(struct lan966x *lan966x, int port, > > const unsigned char mac[ETH_ALEN], > > unsigned int vid, > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h > > index a13c469e139a..797560172aca 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h > > @@ -169,6 +169,12 @@ enum lan966x_target { > > #define ANA_MACACCESS_CHANGE2SW_GET(x)\ > > FIELD_GET(ANA_MACACCESS_CHANGE2SW, x) > > > > +#define ANA_MACACCESS_MAC_CPU_COPY BIT(16) > > +#define ANA_MACACCESS_MAC_CPU_COPY_SET(x)\ > > + FIELD_PREP(ANA_MACACCESS_MAC_CPU_COPY, x) > > +#define ANA_MACACCESS_MAC_CPU_COPY_GET(x)\ > > + FIELD_GET(ANA_MACACCESS_MAC_CPU_COPY, x) > > + > > #define ANA_MACACCESS_VALID BIT(12) > > #define ANA_MACACCESS_VALID_SET(x)\ > > FIELD_PREP(ANA_MACACCESS_VALID, x) > > -- > > 2.33.0 > >
On Mon, Jan 03, 2022 at 04:39:10PM +0100, Horatiu Vultur wrote: > > > +int lan966x_mac_learn(struct lan966x *lan966x, int port, > > > + const unsigned char mac[ETH_ALEN], > > > + unsigned int vid, > > > + enum macaccess_entry_type type) > > > +{ > > > + return lan966x_mac_learn_impl(lan966x, port, false, mac, vid, type); > > > > If you call lan966x_mac_cpu_copy() on an address and then > > lan966x_mac_learn() on the same address but on an external port, how > > does that work (why doesn't the "false" here overwrite the cpu_copy in > > the previous command, breaking the copy-to-CPU feature)? > > Then you will overwrite the cpu_copy so the frames will not reach the > CPU anymore. > But you should not do that. The function lan966x_mac_cpu_copy() should be > used for IPv4/IPv6 and lan966x_mac_learn() for the other types. > > Maybe the function lan966x_mac_cpu_copy() is too generic. It should be > something like lan966x_mac_ipv4(), lan966x_mac_ipv6() and these functions > will call __lan966x_mac_learn with the correct parameters. Also I can > add a WARN_ON(...) inside lan966x_mac_learn not to be used with the > IPv4/IPv6 types. The intended usage pattern isn't clear at all in the current series.
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c index efadb8d326cc..ae3a7a10e0d6 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c @@ -68,16 +68,18 @@ static void lan966x_mac_select(struct lan966x *lan966x, lan_wr(mach, lan966x, ANA_MACHDATA); } -int lan966x_mac_learn(struct lan966x *lan966x, int port, - const unsigned char mac[ETH_ALEN], - unsigned int vid, - enum macaccess_entry_type type) +static int lan966x_mac_learn_impl(struct lan966x *lan966x, int port, + bool cpu_copy, + const unsigned char mac[ETH_ALEN], + unsigned int vid, + enum macaccess_entry_type type) { lan966x_mac_select(lan966x, mac, vid); /* Issue a write command */ lan_wr(ANA_MACACCESS_VALID_SET(1) | ANA_MACACCESS_CHANGE2SW_SET(0) | + ANA_MACACCESS_MAC_CPU_COPY_SET(cpu_copy) | ANA_MACACCESS_DEST_IDX_SET(port) | ANA_MACACCESS_ENTRYTYPE_SET(type) | ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_LEARN), @@ -86,6 +88,23 @@ int lan966x_mac_learn(struct lan966x *lan966x, int port, return lan966x_mac_wait_for_completion(lan966x); } +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port, + bool cpu_copy, + const unsigned char mac[ETH_ALEN], + unsigned int vid, + enum macaccess_entry_type type) +{ + return lan966x_mac_learn_impl(lan966x, port, cpu_copy, mac, vid, type); +} + +int lan966x_mac_learn(struct lan966x *lan966x, int port, + const unsigned char mac[ETH_ALEN], + unsigned int vid, + enum macaccess_entry_type type) +{ + return lan966x_mac_learn_impl(lan966x, port, false, mac, vid, type); +} + int lan966x_mac_forget(struct lan966x *lan966x, const unsigned char mac[ETH_ALEN], unsigned int vid, diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h index c399b1256edc..a7a2a3537036 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h @@ -157,6 +157,11 @@ int lan966x_port_pcs_set(struct lan966x_port *port, struct lan966x_port_config *config); void lan966x_port_init(struct lan966x_port *port); +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port, + bool cpu_copy, + const unsigned char mac[ETH_ALEN], + unsigned int vid, + enum macaccess_entry_type type); int lan966x_mac_learn(struct lan966x *lan966x, int port, const unsigned char mac[ETH_ALEN], unsigned int vid, diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h index a13c469e139a..797560172aca 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h @@ -169,6 +169,12 @@ enum lan966x_target { #define ANA_MACACCESS_CHANGE2SW_GET(x)\ FIELD_GET(ANA_MACACCESS_CHANGE2SW, x) +#define ANA_MACACCESS_MAC_CPU_COPY BIT(16) +#define ANA_MACACCESS_MAC_CPU_COPY_SET(x)\ + FIELD_PREP(ANA_MACACCESS_MAC_CPU_COPY, x) +#define ANA_MACACCESS_MAC_CPU_COPY_GET(x)\ + FIELD_GET(ANA_MACACCESS_MAC_CPU_COPY, x) + #define ANA_MACACCESS_VALID BIT(12) #define ANA_MACACCESS_VALID_SET(x)\ FIELD_PREP(ANA_MACACCESS_VALID, x)
Extend mac functionality with the function lan966x_mac_cpu_copy. This function adds an entry in the MAC table where a frame is copy to the CPU but also can be forward on the front ports. This functionality is needed for mdb support. In case the CPU and some of the front ports subscribe to an IP multicast address. Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- .../ethernet/microchip/lan966x/lan966x_mac.c | 27 ++++++++++++++++--- .../ethernet/microchip/lan966x/lan966x_main.h | 5 ++++ .../ethernet/microchip/lan966x/lan966x_regs.h | 6 +++++ 3 files changed, 34 insertions(+), 4 deletions(-)