Re: [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for offloading flows from wlan devices

[Simon Horman <simon.horman@corigine.com>]

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@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.