by Ivan Khoronzhuk

[permalink] [raw]

Subject: Re: [RFC PATCH v3 net-next 03/11] net: ethernet: ti: cpsw: ale: add functions to modify VLANs/MDBs

On Thu, Apr 25, 2019 at 01:24:19AM +0300, Grygorii Strashko wrote:
>From: Ilias Apalodimas <[email protected]>
>
>A following patch introduces switchdev functionality. Add functions
>to cpsw ALE engine to modify VLANs/MDBs
>
>Signed-off-by: Ilias Apalodimas <[email protected]>
>Signed-off-by: Grygorii Strashko <[email protected]>
>---
> drivers/net/ethernet/ti/cpsw_ale.c | 179 ++++++++++++++++++++++++++++-
> drivers/net/ethernet/ti/cpsw_ale.h | 10 ++
> 2 files changed, 187 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
>index d09768368dbd..add3f6e4917b 100644
>--- a/drivers/net/ethernet/ti/cpsw_ale.c
>+++ b/drivers/net/ethernet/ti/cpsw_ale.c
>@@ -400,6 +400,45 @@ int cpsw_ale_del_mcast(struct cpsw_ale *ale, const u8 *addr, int port_mask,
> return 0;
> }
>
>+static int cpsw_ale_read_mc(struct cpsw_ale *ale, u8 *addr, int flags, u16 vid)
See below.

>+{
>+ u32 ale_entry[ALE_ENTRY_WORDS] = {0, 0, 0};
>+ int idx;
>+
>+ idx = cpsw_ale_match_addr(ale, addr, (flags & ALE_VLAN) ? vid : 0);
>+ if (idx >= 0)
>+ cpsw_ale_read(ale, idx, ale_entry);
>+
>+ return cpsw_ale_get_port_mask(ale_entry, ale->port_mask_bits);
>+}
>+
>+int cpsw_ale_mcast_add_modify(struct cpsw_ale *ale, u8 *addr, int port_mask,
>+ int flags, u16 vid, int mcast_state)
>+{
>+ int mcast_members, ret;
>+
>+ mcast_members = cpsw_ale_read_mc(ale, addr, flags, vid) | port_mask;
>+ ret = cpsw_ale_add_mcast(ale, addr, mcast_members, flags, vid,
>+ mcast_state);
By fact, cpsw_ale_add_mcast() is doing modify itself already, just name is
different.

These 3 func duplicate the existent functionality as result code is done twice.
Better remove/combine them and rename existent ones, like:
cpsw_ale_add_mcast() -> cpsw_ale_add_mcast_modify()
cpsw_ale_del_mcast() -> cpsw_ale_del_mcast_modify()

>+
>+ return ret;
>+}
>+
>+int cpsw_ale_mcast_del_modify(struct cpsw_ale *ale, u8 *addr, int port_mask,
>+ int flags, u16 vid)
See above.

>+{
>+ int mcast_members, ret;
>+ int idx;
>+
>+ mcast_members = cpsw_ale_read_mc(ale, addr, flags, vid) & ~port_mask;
>+ idx = cpsw_ale_match_addr(ale, addr, (flags & ALE_VLAN) ? vid : 0);
>+ if (idx < 0)
>+ return 0;
>+ ret = cpsw_ale_del_mcast(ale, addr, mcast_members, flags, vid);
>+
>+ return ret;
>+}
>+

[...]

--
Regards,
Ivan Khoronzhuk

2019-04-26 10:55:03

by Ilias Apalodimas

[permalink] [raw]

Subject: Re: [RFC PATCH v3 net-next 03/11] net: ethernet: ti: cpsw: ale: add functions to modify VLANs/MDBs

Hi Ivan,

> >}
> >
> >+static int cpsw_ale_read_mc(struct cpsw_ale *ale, u8 *addr, int flags, u16 vid)
> See below.
>
Yes
> >+{
> >+ u32 ale_entry[ALE_ENTRY_WORDS] = {0, 0, 0};
> >+ int idx;
> >+
> >+ idx = cpsw_ale_match_addr(ale, addr, (flags & ALE_VLAN) ? vid : 0);
> >+ if (idx >= 0)
> >+ cpsw_ale_read(ale, idx, ale_entry);
> >+
> >+ return cpsw_ale_get_port_mask(ale_entry, ale->port_mask_bits);
> >+}
> >+
> >+int cpsw_ale_mcast_add_modify(struct cpsw_ale *ale, u8 *addr, int port_mask,
> >+ int flags, u16 vid, int mcast_state)
> >+{
> >+ int mcast_members, ret;
> >+
> >+ mcast_members = cpsw_ale_read_mc(ale, addr, flags, vid) | port_mask;
> >+ ret = cpsw_ale_add_mcast(ale, addr, mcast_members, flags, vid,
> >+ mcast_state);
> By fact, cpsw_ale_add_mcast() is doing modify itself already, just name is
> different.
>
> These 3 func duplicate the existent functionality as result code is done twice.
> Better remove/combine them and rename existent ones, like:
> cpsw_ale_add_mcast() -> cpsw_ale_add_mcast_modify()
> cpsw_ale_del_mcast() -> cpsw_ale_del_mcast_modify()
>
Yes this is correct. The reason the duplication exists is that this was
originally coded on top of the current cpsw code. So i didn't want to change any
part of the working driver.
Since we are re-working a bunch of stuff now this makes sense

> >+
> >+ return ret;
> >+}
> >+
> >+int cpsw_ale_mcast_del_modify(struct cpsw_ale *ale, u8 *addr, int port_mask,
> >+ int flags, u16 vid)
> See above.
and yes :)
>
> >+{
> >+ int mcast_members, ret;
> >+ int idx;
> >+
> >+ mcast_members = cpsw_ale_read_mc(ale, addr, flags, vid) & ~port_mask;
> >+ idx = cpsw_ale_match_addr(ale, addr, (flags & ALE_VLAN) ? vid : 0);
> >+ if (idx < 0)
> >+ return 0;
> >+ ret = cpsw_ale_del_mcast(ale, addr, mcast_members, flags, vid);
> >+
> >+ return ret;
> >+}
> >+

Regards,
/Ilias