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 07:34:38AM +0000, Guo, Junfeng wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Monday, August 21, 2023 15:30
> > To: Guo, Junfeng <junfeng.guo@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Nguyen,
> > Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse
> > <jesse.brandeburg@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > ivecera <ivecera@redhat.com>; Samudrala, Sridhar
> > <sridhar.samudrala@intel.com>
> > Subject: Re: [PATCH iwl-next v5 01/15] ice: add parser create and
> > destroy skeleton
> > 
> > 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.
> 
> Yes, thanks for the carefully review!
> 
> > 
> > >
> > > >  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.
> 
> Thanks for the comments!
> Yes, the setting for rt field should be moved to patch 10/15.
> Will update in the new version patch set. Thanks!

Likewise, thanks.

If you are going to address this you may also
want to look at what seems to be similar problem with
both ICE_PARSER_FLG_NUM and ICE_ERR_NOT_IMPL appearing
in code before they are defined.

...