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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR11MB372326A9056D496FF937B927E71FA@DM6PR11MB3723.namprd11.prod.outlook.com>
Date: Tue, 22 Aug 2023 02:47:53 +0000
From: "Guo, Junfeng" <junfeng.guo@...el.com>
To: Simon Horman <horms@...nel.org>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Brandeburg, Jesse"
	<jesse.brandeburg@...el.com>, "Zhang, Qi Z" <qi.z.zhang@...el.com>, ivecera
	<ivecera@...hat.com>, "Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [PATCH iwl-next v5 01/15] ice: add parser create and destroy
 skeleton



> -----Original Message-----
> From: Simon Horman <horms@...nel.org>
> Sent: Monday, August 21, 2023 22:48
> To: Guo, Junfeng <junfeng.guo@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Nguyen,
> Anthony L <anthony.l.nguyen@...el.com>; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>; Zhang, Qi Z <qi.z.zhang@...el.com>;
> ivecera <ivecera@...hat.com>; Samudrala, Sridhar
> <sridhar.samudrala@...el.com>
> Subject: Re: [PATCH iwl-next v5 01/15] ice: add parser create and
> destroy skeleton
> 
> On Mon, Aug 21, 2023 at 07:34:38AM +0000, Guo, Junfeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Simon Horman <horms@...nel.org>
> > > Sent: Monday, August 21, 2023 15:30
> > > To: Guo, Junfeng <junfeng.guo@...el.com>
> > > Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org;
> Nguyen,
> > > Anthony L <anthony.l.nguyen@...el.com>; Brandeburg, Jesse
> > > <jesse.brandeburg@...el.com>; Zhang, Qi Z
> <qi.z.zhang@...el.com>;
> > > ivecera <ivecera@...hat.com>; Samudrala, Sridhar
> > > <sridhar.samudrala@...el.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@...el.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.

Oh, thanks for pointing out this!

Will also check the rest code for similar problem.
Thanks for the carefully review!

> 
> ...