From: Simon Horman <simon.horman@corigine•com>
To: Felix Fietkau <nbd@nbd•name>
Cc: netdev@vger•kernel.org
Subject: Re: [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for offloading flows from wlan devices
Date: Wed, 22 Mar 2023 15:04:19 +0100 [thread overview]
Message-ID: <ZBsK46vmNtjxJZH6@corigine.com> (raw)
In-Reply-To: <20230321133609.49591-1-nbd@nbd.name>
On Tue, Mar 21, 2023 at 02:36:08PM +0100, Felix Fietkau wrote:
> WED version 2 (on MT7986 and later) can offload flows originating from wireless
> devices. In order to make that work, ndo_setup_tc needs to be implemented on
> the netdevs. This adds the required code to offload flows coming in from WED,
> while keeping track of the incoming wed index used for selecting the correct
> PPE device.
>
> Signed-off-by: Felix Fietkau <nbd@nbd•name>
Hi Felix,
A few nits from my side.
First, please reformat the patch description to have a maximum of 75 characters
per line, as suggested by checkpatch.
...
> @@ -512,25 +514,15 @@ mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f)
>
> static DEFINE_MUTEX(mtk_flow_offload_mutex);
>
> -static int
> -mtk_eth_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_priv)
> +int mtk_flow_offload_cmd(struct mtk_eth *eth, struct flow_cls_offload *cls,
> + int ppe_index)
> {
> - struct flow_cls_offload *cls = type_data;
> - struct net_device *dev = cb_priv;
> - struct mtk_mac *mac = netdev_priv(dev);
> - struct mtk_eth *eth = mac->hw;
> int err;
>
> - if (!tc_can_offload(dev))
> - return -EOPNOTSUPP;
> -
> - if (type != TC_SETUP_CLSFLOWER)
> - return -EOPNOTSUPP;
> -
> mutex_lock(&mtk_flow_offload_mutex);
> switch (cls->command) {
> case FLOW_CLS_REPLACE:
> - err = mtk_flow_offload_replace(eth, cls);
> + err = mtk_flow_offload_replace(eth, cls, ppe_index);
> break;
> case FLOW_CLS_DESTROY:
> err = mtk_flow_offload_destroy(eth, cls);
> @@ -547,6 +539,23 @@ mtk_eth_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_pri
> return err;
> }
>
> +static int
> +mtk_eth_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_priv)
> +{
> + struct flow_cls_offload *cls = type_data;
> + struct net_device *dev = cb_priv;
> + struct mtk_mac *mac = netdev_priv(dev);
> + struct mtk_eth *eth = mac->hw;
Reverse xmas tree - longest line to shortest -
for local variable declarations please.
> +
> + if (!tc_can_offload(dev))
> + return -EOPNOTSUPP;
> +
> + if (type != TC_SETUP_CLSFLOWER)
> + return -EOPNOTSUPP;
> +
> + return mtk_flow_offload_cmd(eth, cls, 0);
> +}
> +
> static int
> mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
> {
> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index 95d890870984..30fe1281d2d3 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
...
> @@ -1745,6 +1752,102 @@ void mtk_wed_flow_remove(int index)
> mutex_unlock(&hw_lock);
> }
>
> +static int
> +mtk_wed_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_priv)
> +{
> + struct mtk_wed_flow_block_priv *priv = cb_priv;
> + struct flow_cls_offload *cls = type_data;
> + struct mtk_wed_hw *hw = priv->hw;
> +
> + if (!tc_can_offload(priv->dev))
> + return -EOPNOTSUPP;
> +
> + if (type != TC_SETUP_CLSFLOWER)
> + return -EOPNOTSUPP;
> +
> + return mtk_flow_offload_cmd(hw->eth, cls, hw->index);
This seems very similar to mtk_eth_setup_tc_block_cb().
Can further consolidation be considered?
> +}
> +
> +static int
> +mtk_wed_setup_tc_block(struct mtk_wed_hw *hw, struct net_device *dev,
> + struct flow_block_offload *f)
> +{
> + struct mtk_wed_flow_block_priv *priv;
> + static LIST_HEAD(block_cb_list);
> + struct flow_block_cb *block_cb;
> + struct mtk_eth *eth = hw->eth;
> + bool register_block = false;
> + flow_setup_cb_t *cb;
> +
> + if (!eth->soc->offload_version)
> + return -EOPNOTSUPP;
> +
> + if (f->binder_type != FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
> + return -EOPNOTSUPP;
> +
> + cb = mtk_wed_setup_tc_block_cb;
> + f->driver_block_list = &block_cb_list;
> +
> + switch (f->command) {
> + case FLOW_BLOCK_BIND:
> + block_cb = flow_block_cb_lookup(f->block, cb, dev);
> + if (!block_cb) {
I wonder if this could be written more idiomatically as:
if (block_cb) {
flow_block_cb_incref(block_cb);
return 0;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
...
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->hw = hw;
> + priv->dev = dev;
> + block_cb = flow_block_cb_alloc(cb, dev, priv, NULL);
> + if (IS_ERR(block_cb)) {
> + kfree(priv);
> + return PTR_ERR(block_cb);
> + }
> +
> + register_block = true;
> + }
> +
> + flow_block_cb_incref(block_cb);
> +
> + if (register_block) {
> + flow_block_cb_add(block_cb, f);
> + list_add_tail(&block_cb->driver_list, &block_cb_list);
> + }
> + return 0;
> + case FLOW_BLOCK_UNBIND:
> + block_cb = flow_block_cb_lookup(f->block, cb, dev);
> + if (!block_cb)
> + return -ENOENT;
> +
> + if (!flow_block_cb_decref(block_cb)) {
> + flow_block_cb_remove(block_cb, f);
> + list_del(&block_cb->driver_list);
> + kfree(block_cb->cb_priv);
> + }
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
...
> diff --git a/include/linux/soc/mediatek/mtk_wed.h b/include/linux/soc/mediatek/mtk_wed.h
...
> @@ -237,6 +240,8 @@ mtk_wed_get_rx_capa(struct mtk_wed_device *dev)
> (_dev)->ops->msg_update(_dev, _id, _msg, _len)
> #define mtk_wed_device_stop(_dev) (_dev)->ops->stop(_dev)
> #define mtk_wed_device_dma_reset(_dev) (_dev)->ops->reset_dma(_dev)
> +#define mtk_wed_device_setup_tc(_dev, _netdev, _type, _type_data) \
> + (_dev)->ops->setup_tc(_dev, _netdev, _type, _type_data)
nit: checkpatch says:
include/linux/soc/mediatek/mtk_wed.h:243: ERROR: Macros with complex values should be enclosed in parentheses
+#define mtk_wed_device_setup_tc(_dev, _netdev, _type, _type_data) \
+ (_dev)->ops->setup_tc(_dev, _netdev, _type, _type_data)
include/linux/soc/mediatek/mtk_wed.h:243: CHECK: Macro argument reuse '_dev' - possible side-effects?
+#define mtk_wed_device_setup_tc(_dev, _netdev, _type, _type_data) \
+ (_dev)->ops->setup_tc(_dev, _netdev, _type, _type_data)
> #else
> static inline bool mtk_wed_device_active(struct mtk_wed_device *dev)
> {
next prev parent reply other threads:[~2023-03-22 14:04 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-21 13:36 [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for offloading flows from wlan devices Felix Fietkau
2023-03-21 13:36 ` [PATCH net-next 2/2] net: ethernet: mediatek: mtk_ppe: prefer newly added l2 flows over existing ones Felix Fietkau
2023-03-22 14:00 ` Simon Horman
2023-03-22 15:09 ` Felix Fietkau
2023-03-22 20:00 ` Simon Horman
2023-03-22 14:04 ` Simon Horman [this message]
2023-03-22 14:29 ` [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for offloading flows from wlan devices Simon Horman
2023-03-22 15:19 ` Felix Fietkau
2023-03-22 20:05 ` Simon Horman
2023-03-22 15:18 ` Felix Fietkau
2023-03-22 20:04 ` Simon Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZBsK46vmNtjxJZH6@corigine.com \
--to=simon.horman@corigine$(echo .)com \
--cc=nbd@nbd$(echo .)name \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox