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

[Simon Horman <horms@kernel.org>]

On Mon, Aug 21, 2023 at 09:20:37AM +0200, Simon Horman wrote:
> 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.

Sorry, somehow I wasn't looking at the entire series.
I now see that ice_parser.c is compiled as of patch 12/15 of this series.

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

And I see this field is added in patch 10/15, rather than the last patch
(15/15) as I previously stated.

> 
> > +
> > +	*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);
> > +}
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan