Messages in this thread | | | Date | Thu, 30 Dec 2021 16:19:54 +0200 | From | Ido Schimmel <> | Subject | Re: [PATCH net-next v2 4/6] net: marvell: prestera: add hardware router objects accounting |
| |
On Mon, Dec 27, 2021 at 11:52:29PM +0200, Yevhen Orlov wrote: > Add prestera_router_hw.c. This file contains functions, which track HW > objects relations and links. This include implicity creation of objects, > that needed by requested one and implicity removing of objects, which > reference counter is became zero. > > We need this layer, because kernel callbacks not always mapped to > creation of single HW object. So let it be two different layers - one > for subscribing and parsing kernel structures, and another > (prestera_router_hw.c) for HW objects relations tracking. > > There is two types of objects on router_hw layer: > - Explicit objects (rif_entry) : created by higher layer. > - Implicit objects (vr) : created on demand by explicit objects. > > Co-developed-by: Taras Chornyi <tchornyi@marvell.com> > Signed-off-by: Taras Chornyi <tchornyi@marvell.com> > Co-developed-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu> > Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu> > Signed-off-by: Yevhen Orlov <yevhen.orlov@plvision.eu> > --- > v1-->v2 > * No changes > --- > .../net/ethernet/marvell/prestera/Makefile | 2 +- > .../marvell/prestera/prestera_router.c | 10 + > .../marvell/prestera/prestera_router_hw.c | 209 ++++++++++++++++++ > .../marvell/prestera/prestera_router_hw.h | 36 +++ > 4 files changed, 256 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_router_hw.c > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_router_hw.h > > diff --git a/drivers/net/ethernet/marvell/prestera/Makefile b/drivers/net/ethernet/marvell/prestera/Makefile > index ec69fc564a9f..d395f4131648 100644 > --- a/drivers/net/ethernet/marvell/prestera/Makefile > +++ b/drivers/net/ethernet/marvell/prestera/Makefile > @@ -4,6 +4,6 @@ prestera-objs := prestera_main.o prestera_hw.o prestera_dsa.o \ > prestera_rxtx.o prestera_devlink.o prestera_ethtool.o \ > prestera_switchdev.o prestera_acl.o prestera_flow.o \ > prestera_flower.o prestera_span.o prestera_counter.o \ > - prestera_router.o > + prestera_router.o prestera_router_hw.o > > obj-$(CONFIG_PRESTERA_PCI) += prestera_pci.o > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router.c b/drivers/net/ethernet/marvell/prestera/prestera_router.c > index f3980d10eb29..2a32831df40f 100644 > --- a/drivers/net/ethernet/marvell/prestera/prestera_router.c > +++ b/drivers/net/ethernet/marvell/prestera/prestera_router.c > @@ -5,10 +5,12 @@ > #include <linux/types.h> > > #include "prestera.h" > +#include "prestera_router_hw.h" > > int prestera_router_init(struct prestera_switch *sw) > { > struct prestera_router *router; > + int err; > > router = kzalloc(sizeof(*sw->router), GFP_KERNEL); > if (!router) > @@ -17,7 +19,15 @@ int prestera_router_init(struct prestera_switch *sw) > sw->router = router; > router->sw = sw; > > + err = prestera_router_hw_init(sw); > + if (err) > + goto err_router_lib_init; > + > return 0; > + > +err_router_lib_init: > + kfree(sw->router); > + return err; > } > > void prestera_router_fini(struct prestera_switch *sw)
Looks suspicious that you don't call prestera_router_hw_fini() here. You can at least verify that the two lists you initialize in prestera_router_hw_init() are indeed empty.
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c > new file mode 100644 > index 000000000000..4f66fb21a299 > --- /dev/null > +++ b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c > @@ -0,0 +1,209 @@ > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 > +/* Copyright (c) 2019-2021 Marvell International Ltd. All rights reserved */ > + > +#include <linux/rhashtable.h> > + > +#include "prestera.h" > +#include "prestera_hw.h" > +#include "prestera_router_hw.h" > +#include "prestera_acl.h" > + > +/* +--+ > + * +------->|vr| > + * | +--+ > + * | > + * +-+-------+ > + * |rif_entry| > + * +---------+ > + * Rif is > + * used as > + * entry point > + * for vr in hw > + */ > + > +int prestera_router_hw_init(struct prestera_switch *sw) > +{ > + INIT_LIST_HEAD(&sw->router->vr_list); > + INIT_LIST_HEAD(&sw->router->rif_entry_list); > + > + return 0; > +} > + > +static struct prestera_vr *__prestera_vr_find(struct prestera_switch *sw, > + u32 tb_id) > +{ > + struct prestera_vr *vr; > + > + list_for_each_entry(vr, &sw->router->vr_list, router_node) {
Probably better to store VRs in something like IDR instead of a linked list
> + if (vr->tb_id == tb_id) > + return vr; > + } > + > + return NULL; > +} > + > +static struct prestera_vr *__prestera_vr_create(struct prestera_switch *sw, > + u32 tb_id, > + struct netlink_ext_ack *extack) > +{ > + struct prestera_vr *vr; > + u16 hw_vr_id; > + int err; > + > + err = prestera_hw_vr_create(sw, &hw_vr_id); > + if (err) > + return ERR_PTR(-ENOMEM); > + > + vr = kzalloc(sizeof(*vr), GFP_KERNEL); > + if (!vr) { > + err = -ENOMEM; > + goto err_alloc_vr; > + } > + > + vr->tb_id = tb_id; > + vr->hw_vr_id = hw_vr_id; > + > + list_add(&vr->router_node, &sw->router->vr_list); > + > + return vr; > + > +err_alloc_vr: > + prestera_hw_vr_delete(sw, hw_vr_id); > + kfree(vr);
You failed to allocate it, so no need to free it
> + return ERR_PTR(err); > +} > + > +static void __prestera_vr_destroy(struct prestera_switch *sw, > + struct prestera_vr *vr) > +{ > + prestera_hw_vr_delete(sw, vr->hw_vr_id); > + list_del(&vr->router_node);
Not symmetric with __prestera_vr_create()
> + kfree(vr); > +} > + > +static struct prestera_vr *prestera_vr_get(struct prestera_switch *sw, u32 tb_id, > + struct netlink_ext_ack *extack) > +{ > + struct prestera_vr *vr; > + > + vr = __prestera_vr_find(sw, tb_id); > + if (!vr) > + vr = __prestera_vr_create(sw, tb_id, extack); > + if (IS_ERR(vr)) > + return ERR_CAST(vr); > + > + return vr; > +} > + > +static void prestera_vr_put(struct prestera_switch *sw, struct prestera_vr *vr) > +{ > + if (!vr->ref_cnt) > + __prestera_vr_destroy(sw, vr); > +}
These two functions should increase/decrease the reference count of the VR
> + > +/* iface is overhead struct. vr_id also can be removed. */
Unclear
> +static int > +__prestera_rif_entry_key_copy(const struct prestera_rif_entry_key *in, > + struct prestera_rif_entry_key *out) > +{ > + memset(out, 0, sizeof(*out)); > + > + switch (in->iface.type) { > + case PRESTERA_IF_PORT_E: > + out->iface.dev_port.hw_dev_num = in->iface.dev_port.hw_dev_num; > + out->iface.dev_port.port_num = in->iface.dev_port.port_num; > + break; > + case PRESTERA_IF_LAG_E: > + out->iface.lag_id = in->iface.lag_id; > + break; > + case PRESTERA_IF_VID_E: > + out->iface.vlan_id = in->iface.vlan_id; > + break; > + default: > + pr_err("Unsupported iface type");
If this should never happen, then consider using WARN_ON(1)
> + return -EINVAL; > + } > + > + out->iface.type = in->iface.type; > + return 0; > +} > + > +struct prestera_rif_entry * > +prestera_rif_entry_find(const struct prestera_switch *sw, > + const struct prestera_rif_entry_key *k) > +{ > + struct prestera_rif_entry *rif_entry; > + struct prestera_rif_entry_key lk; /* lookup key */ > + > + if (__prestera_rif_entry_key_copy(k, &lk)) > + return NULL; > + > + list_for_each_entry(rif_entry, &sw->router->rif_entry_list, > + router_node) { > + if (!memcmp(k, &rif_entry->key, sizeof(*k))) > + return rif_entry;
Looks like rhashtable is a better option than a linked list
> + } > + > + return NULL; > +} > + > +void prestera_rif_entry_destroy(struct prestera_switch *sw, > + struct prestera_rif_entry *e)
It's easier to maintain/review code that follows a pattern of create() followed by destroy(). You can see if the error path is the same as what you have in destroy()
> +{ > + struct prestera_iface iface; > + > + list_del(&e->router_node); > + > + memcpy(&iface, &e->key.iface, sizeof(iface)); > + iface.vr_id = e->vr->hw_vr_id; > + prestera_hw_rif_delete(sw, e->hw_id, &iface); > + > + e->vr->ref_cnt--; > + prestera_vr_put(sw, e->vr); > + kfree(e); > +} > + > +struct prestera_rif_entry * > +prestera_rif_entry_create(struct prestera_switch *sw, > + struct prestera_rif_entry_key *k, > + u32 tb_id, const unsigned char *addr) > +{ > + int err; > + struct prestera_rif_entry *e; > + struct prestera_iface iface; > + > + e = kzalloc(sizeof(*e), GFP_KERNEL); > + if (!e) > + goto err_kzalloc; > + > + if (__prestera_rif_entry_key_copy(k, &e->key)) > + goto err_key_copy; > + > + e->vr = prestera_vr_get(sw, tb_id, NULL); > + if (IS_ERR(e->vr)) > + goto err_vr_get; > + > + e->vr->ref_cnt++; > + memcpy(&e->addr, addr, sizeof(e->addr)); > + > + /* HW */ > + memcpy(&iface, &e->key.iface, sizeof(iface)); > + iface.vr_id = e->vr->hw_vr_id; > + err = prestera_hw_rif_create(sw, &iface, e->addr, &e->hw_id); > + if (err) > + goto err_hw_create; > + > + list_add(&e->router_node, &sw->router->rif_entry_list); > + > + return e; > + > +err_hw_create: > + e->vr->ref_cnt--; > + prestera_vr_put(sw, e->vr); > +err_vr_get: > +err_key_copy: > + kfree(e); > +err_kzalloc: > + return NULL; > +} > + > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.h b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.h > new file mode 100644 > index 000000000000..fed53595f7bb > --- /dev/null > +++ b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */ > +/* Copyright (c) 2019-2021 Marvell International Ltd. All rights reserved. */ > + > +#ifndef _PRESTERA_ROUTER_HW_H_ > +#define _PRESTERA_ROUTER_HW_H_ > + > +struct prestera_vr { > + struct list_head router_node; > + unsigned int ref_cnt;
Use refcount_t
> + u32 tb_id; /* key (kernel fib table id) */ > + u16 hw_vr_id; /* virtual router ID */ > + u8 __pad[2]; > +}; > + > +struct prestera_rif_entry { > + struct prestera_rif_entry_key { > + struct prestera_iface iface; > + } key; > + struct prestera_vr *vr; > + unsigned char addr[ETH_ALEN]; > + u16 hw_id; /* rif_id */ > + struct list_head router_node; /* ht */ > +}; > + > +struct prestera_rif_entry * > +prestera_rif_entry_find(const struct prestera_switch *sw, > + const struct prestera_rif_entry_key *k); > +void prestera_rif_entry_destroy(struct prestera_switch *sw, > + struct prestera_rif_entry *e); > +struct prestera_rif_entry * > +prestera_rif_entry_create(struct prestera_switch *sw, > + struct prestera_rif_entry_key *k, > + u32 tb_id, const unsigned char *addr); > +int prestera_router_hw_init(struct prestera_switch *sw); > + > +#endif /* _PRESTERA_ROUTER_HW_H_ */ > -- > 2.17.1 >
| |