| 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
| ||
|
Message-ID: <ZBtfXe5mvfIr/a8z@corigine.com>
Date: Wed, 22 Mar 2023 21:04:45 +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 Wed, Mar 22, 2023 at 04:18:29PM +0100, Felix Fietkau wrote:
> On 22.03.23 15:04, Simon Horman wrote:
> > 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.
> Will do
>
> > > @@ -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.
> Will do.
>
> > > 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?
> It's similar, but using different data structures and pointer chains.
> Consolidation does not make sense here.
Thanks, I see that now.
> > > +}
> > > +
> > > +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);
> flow_block_cb_incref needs to be called for the newly allocated flow block
> cb as well. I was following the same pattern that several
I guess that to some extent it is a question of style.
It seems to me that having separate calls to
flow_block_cb_incref() for the different cases
leads to nicer, and less indented, code.
But its just a suggestion from my side.
> > > 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)
> In my opinion that's a false positive. The newly added macros follow the
> same pattern as the existing ones.
Ack.
Powered by blists - more mailing lists