Re: [PATCH iwl-next v5 01/15] ice: add parser create and destroy skeleton

[Simon Horman <horms@kernel.org>]

On Mon, Aug 21, 2023 at 10:38:19AM +0800, Junfeng Guo wrote:
> Add new parser module which can parse a packet in binary
> and generate information like ptype, protocol/offset pairs
> and flags which can be used to feed the FXP profile creation
> directly.
> 
> The patch added skeleton of the create and destroy APIs:
> ice_parser_create
> ice_parser_destroy
> 
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>

Hi Junfeng Guo,

some minor feedback from my side.

> ---
>  drivers/net/ethernet/intel/ice/ice_common.h |  4 +++
>  drivers/net/ethernet/intel/ice/ice_ddp.c    | 10 +++---
>  drivers/net/ethernet/intel/ice/ice_ddp.h    | 13 ++++++++
>  drivers/net/ethernet/intel/ice/ice_parser.c | 34 +++++++++++++++++++++

Perhaps I am missing something, but it seems that although
ice_parser.c is added by this patch-set, it is not added to
the build by this patch-set. This seems a little odd to me.

>  drivers/net/ethernet/intel/ice/ice_parser.h | 13 ++++++++
>  5 files changed, 69 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_parser.c
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_parser.h

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_parser.c b/drivers/net/ethernet/intel/ice/ice_parser.c
> new file mode 100644
> index 000000000000..42602cac7e45
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_parser.c
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2023 Intel Corporation */
> +
> +#include "ice_common.h"
> +
> +/**
> + * ice_parser_create - create a parser instance
> + * @hw: pointer to the hardware structure
> + * @psr: output parameter for a new parser instance be created
> + */
> +int ice_parser_create(struct ice_hw *hw, struct ice_parser **psr)
> +{
> +	struct ice_parser *p;
> +
> +	p = devm_kzalloc(ice_hw_to_dev(hw), sizeof(struct ice_parser),
> +			 GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	p->hw = hw;
> +	p->rt.psr = p;

It is, perhaps academic if this file isn't compiled, but the rt field of
struct ice_parser doesn't exist at this point of the patch-set: it is added
by the last patch of the patch-set.

> +
> +	*psr = p;
> +	return 0;
> +}
> +
> +/**
> + * ice_parser_destroy - destroy a parser instance
> + * @psr: pointer to a parser instance
> + */
> +void ice_parser_destroy(struct ice_parser *psr)
> +{
> +	devm_kfree(ice_hw_to_dev(psr->hw), psr);
> +}