On Wed, May 13, 2020 at 10:12 PM Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>
> Register ndo callback via flow_indr_dev_register() and
> flow_indr_dev_unregister().
>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 -
> drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 51 +++++---------------
> 2 files changed, 12 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index f6a3250ef1c5..e7d1c12673bd 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -1857,7 +1857,6 @@ struct bnxt {
> u8 dsn[8];
> struct bnxt_tc_info *tc_info;
> struct list_head tc_indr_block_list;
> - struct notifier_block tc_netdev_nb;
> struct dentry *debugfs_pdev;
> struct device *hwmon_dev;
> };
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> index 782ea0771221..0eef4f5e4a46 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> @@ -1939,53 +1939,25 @@ static int bnxt_tc_setup_indr_block(struct net_device *netdev, struct bnxt *bp,
> return 0;
> }
>
> -static int bnxt_tc_setup_indr_cb(struct net_device *netdev, void *cb_priv,
> - enum tc_setup_type type, void *type_data)
> -{
> - switch (type) {
> - case TC_SETUP_BLOCK:
> - return bnxt_tc_setup_indr_block(netdev, cb_priv, type_data);
> - default:
> - return -EOPNOTSUPP;
> - }
> -}
> -
> static bool bnxt_is_netdev_indr_offload(struct net_device *netdev)
> {
> return netif_is_vxlan(netdev);
> }
>
> -static int bnxt_tc_indr_block_event(struct notifier_block *nb,
> - unsigned long event, void *ptr)
> +static int bnxt_tc_setup_indr_cb(struct net_device *netdev, void *cb_priv,
> + enum tc_setup_type type, void *type_data)
> {
> - struct net_device *netdev;
> - struct bnxt *bp;
> - int rc;
> -
> - netdev = netdev_notifier_info_to_dev(ptr);
> if (!bnxt_is_netdev_indr_offload(netdev))
> - return NOTIFY_OK;
> -
> - bp = container_of(nb, struct bnxt, tc_netdev_nb);
> + return -EOPNOTSUPP;
>
> - switch (event) {
> - case NETDEV_REGISTER:
> - rc = __flow_indr_block_cb_register(netdev, bp,
> - bnxt_tc_setup_indr_cb,
> - bp);
> - if (rc)
> - netdev_info(bp->dev,
> - "Failed to register indirect blk: dev: %s\n",
> - netdev->name);
> - break;
> - case NETDEV_UNREGISTER:
> - __flow_indr_block_cb_unregister(netdev,
> - bnxt_tc_setup_indr_cb,
> - bp);
> + switch (type) {
> + case TC_SETUP_BLOCK:
> + return bnxt_tc_setup_indr_block(netdev, cb_priv, type_data);
> + default:
> break;
> }
>
> - return NOTIFY_DONE;
> + return -EOPNOTSUPP;
> }
>
> static const struct rhashtable_params bnxt_tc_flow_ht_params = {
> @@ -2074,8 +2046,8 @@ int bnxt_init_tc(struct bnxt *bp)
>
> /* init indirect block notifications */
> INIT_LIST_HEAD(&bp->tc_indr_block_list);
> - bp->tc_netdev_nb.notifier_call = bnxt_tc_indr_block_event;
> - rc = register_netdevice_notifier(&bp->tc_netdev_nb);
> +
> + rc = flow_indr_dev_register(bnxt_tc_setup_indr_cb, bp);
> if (!rc)
> return 0;
>
> @@ -2101,7 +2073,8 @@ void bnxt_shutdown_tc(struct bnxt *bp)
> if (!bnxt_tc_flower_enabled(bp))
> return;
>
> - unregister_netdevice_notifier(&bp->tc_netdev_nb);
> + flow_indr_dev_unregister(bnxt_tc_setup_indr_cb, bp,
> + bnxt_tc_setup_indr_block_cb);
Why does the driver need to provide the "cb" again during unregister,
since both "cb" and "cb_priv" are already provided during register() ?
This interface could be simplified/improved if
flow_indr_dev_register() returns an opaque handle to the object it
creates (struct flow_indr_dev *) ? The driver should just pass this
handle during unregistration. Also, why do we need the extra (3rd)
argument (flow_setup_cb_t / bnxt_tc_setup_indr_block_cb) during unreg
? It is handled internally by the driver as a part of FLOW_BLOCK_BIND
/ UNBIND ?
Thanks,
-Harsha
> rhashtable_destroy(&tc_info->flow_table);
> rhashtable_destroy(&tc_info->l2_table);
> rhashtable_destroy(&tc_info->decap_l2_table);
> --
> 2.20.1
>
