Received-SPF: pass (google.com: domain of linux-kernel+bounces-146633-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 16 Apr 2024 11:37:02 +0100
From: Simon Horman <horms@kernel.org>
To: Geetha sowjanya <gakula@marvell.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kuba@kernel.org,
	davem@davemloft.net, pabeni@redhat.com, edumazet@google.com,
	sgoutham@marvell.com, sbhatta@marvell.com, hkelam@marvell.com
Subject: Re: [net-next PATCH v2] octeontx2-pf: Add support for offload tc
 with skbedit mark action
Message-ID: <20240416103702.GI2320920@kernel.org>
References: <20240414062957.18840-1-gakula@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240414062957.18840-1-gakula@marvell.com>

On Sun, Apr 14, 2024 at 11:59:57AM +0530, Geetha sowjanya wrote:
> Support offloading of skbedit mark action.
> 
> For example, to mark with 0x0008, with dest ip 60.60.60.2 on eth2
> interface:
> 
>  # tc qdisc add dev eth2 ingress
>  # tc filter add dev eth2 ingress protocol ip flower \
>       dst_ip 60.60.60.2 action skbedit mark 0x0008 skip_sw
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Geetha sowjanya <gakula@marvell.com>
> ---
> v1-v2: 
>   -Changed mark_flows data type to refcount_t 

Thanks Geetha,

The nit below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

..

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 87bdb93cb066..8b8ac179f3c3 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> @@ -511,7 +511,15 @@ static int otx2_tc_parse_actions(struct otx2_nic *nic,
>  			nr_police++;
>  			break;
>  		case FLOW_ACTION_MARK:
> +			if (act->mark & ~OTX2_RX_MATCH_ID_MASK) {
> +				NL_SET_ERR_MSG_MOD(extack, "Bad flow mark, only 16 bit supported");
> +				return -EOPNOTSUPP;
> +			}
>  			mark = act->mark;
> +			req->match_id = mark & 0xFFFFULL;

nit: Is the mask necessary here?
     act->mark was already checked against OTX2_RX_MATCH_ID_MASK
     to ensure that nothing is set in the upper 16 bits.

     If it is, could OTX2_RX_MATCH_ID_MASK be used instead of 0xFFFFULL ?
     Or perhaps use lower_16_bits().

     No need for this to block this patch, AFAIK.
     But perhaps a follow-up could be considered.

> +			req->op = NIX_RX_ACTION_DEFAULT;
> +			nic->flags |= OTX2_FLAG_TC_MARK_ENABLED;
> +			refcount_inc(&nic->flow_cfg->mark_flows);
>  			break;
>  
>  		case FLOW_ACTION_RX_QUEUE_MAPPING:

..

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
> index a82ffca8ce1b..3f1d2655ff77 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
> @@ -62,6 +62,9 @@
>  #define CQ_OP_STAT_OP_ERR       63
>  #define CQ_OP_STAT_CQ_ERR       46
>  
> +/* Packet mark mask */
> +#define OTX2_RX_MATCH_ID_MASK 0x0000ffff
> +
>  struct queue_stats {
>  	u64	bytes;
>  	u64	pkts;
> -- 
> 2.25.1
> 
>