| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 22 Mar 2023 15:04:19 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Felix Fietkau <nbd@....name>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for
offloading flows from wlan devices
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@....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)
> {
Powered by blists - more mailing lists