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: <DM6PR11MB3723437003B055BB7C74AF63E71EA@DM6PR11MB3723.namprd11.prod.outlook.com>
Date: Mon, 21 Aug 2023 07:34:38 +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 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!

> 
> >
> > > +
> > > +	*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);
> > > +}
> >