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: <PH0PR12MB54817F492455B2E32E00A932DC609@PH0PR12MB5481.namprd12.prod.outlook.com>
Date:   Tue, 23 Nov 2021 16:55:44 +0000
From:   Parav Pandit <parav@...dia.com>
To:     "Saleem, Shiraz" <shiraz.saleem@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "Ismail, Mustafa" <mustafa.ismail@...el.com>,
        "Keller, Jacob E" <jacob.e.keller@...el.com>,
        Jiri Pirko <jiri@...dia.com>,
        "Kaliszczuk, Leszek" <leszek.kaliszczuk@...el.com>
Subject: RE: [PATCH net-next 2/3] net/ice: Add support for enable_iwarp and
 enable_roce devlink param



> From: Saleem, Shiraz <shiraz.saleem@...el.com>
> Sent: Tuesday, November 23, 2021 8:18 PM
> 
> > Subject: RE: [PATCH net-next 2/3] net/ice: Add support for
> > enable_iwarp and enable_roce devlink param
> >
> > Hi Tony,
> >
> > > From: Tony Nguyen <anthony.l.nguyen@...el.com>
> > > Sent: Tuesday, November 23, 2021 2:41 AM
> > >
> > > From: Shiraz Saleem <shiraz.saleem@...el.com>
> > >
> > > Allow support for 'enable_iwarp' and 'enable_roce' devlink params to
> > > turn on/off iWARP or RoCE protocol support for E800 devices.
> > >
> > > For example, a user can turn on iWARP functionality with,
> > >
> > > devlink dev param set pci/0000:07:00.0 name enable_iwarp value true
> > > cmode runtime
> > >
> > > This add an iWARP auxiliary rdma device, ice.iwarp.<>, under this PF.
> > >
> > > A user request to enable both iWARP and RoCE under the same PF is
> > > rejected since this device does not support both protocols
> > > simultaneously on the same port.
> > >
> > > Signed-off-by: Shiraz Saleem <shiraz.saleem@...el.com>
> > > Tested-by: Leszek Kaliszczuk <leszek.kaliszczuk@...el.com>
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> > > ---
> > >  drivers/net/ethernet/intel/ice/ice.h         |   1 +
> > >  drivers/net/ethernet/intel/ice/ice_devlink.c | 144 +++++++++++++++++++
> > >  drivers/net/ethernet/intel/ice/ice_devlink.h |   6 +
> > >  drivers/net/ethernet/intel/ice/ice_idc.c     |   4 +-
> > >  drivers/net/ethernet/intel/ice/ice_main.c    |   9 +-
> > >  include/linux/net/intel/iidc.h               |   7 +-
> > >  6 files changed, 166 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice.h
> > > b/drivers/net/ethernet/intel/ice/ice.h
> > > index b2db39ee5f85..b67ad51cbcc9 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > > @@ -576,6 +576,7 @@ struct ice_pf {
> > >  	struct ice_hw_port_stats stats_prev;
> > >  	struct ice_hw hw;
> > >  	u8 stat_prev_loaded:1; /* has previous stats been loaded */
> > > +	u8 rdma_mode;
> > This can be u8 rdma_mode: 1;
> > See below.
> >
> > >  	u16 dcbx_cap;
> > >  	u32 tx_timeout_count;
> > >  	unsigned long tx_timeout_last_recovery; diff --git
> > > a/drivers/net/ethernet/intel/ice/ice_devlink.c
> > > b/drivers/net/ethernet/intel/ice/ice_devlink.c
> > > index b9bd9f9472f6..478412b28a76 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> > > @@ -430,6 +430,120 @@ static const struct devlink_ops ice_devlink_ops =
> {
> > >  	.flash_update = ice_devlink_flash_update,  };
> > >
> > > +static int
> > > +ice_devlink_enable_roce_get(struct devlink *devlink, u32 id,
> > > +			    struct devlink_param_gset_ctx *ctx) {
> > > +	struct ice_pf *pf = devlink_priv(devlink);
> > > +
> > > +	ctx->val.vbool = pf->rdma_mode & IIDC_RDMA_PROTOCOL_ROCEV2;
> > > +
> > This is logical operation, and vbool will be still zero when rdma mode
> > is rocev2, because it is not bit 0.
> > Please see below. This error can be avoided by having rdma mode as
> Boolean.
> 
> Hi Parav -
> 
> rdma_mode is used as a bit-mask.
> 0 = disabled, i.e. enable_iwarp and enable_roce set to false by user.
> 1 = IIDC_RDMA_PROTOCOL_IWARP
> 2 = IIDC_RDMA_PROTOCOL_ROCEV2
>
Yes, I got it. bit mask is ok.
But this line,
ctx->val.vbool = pf->rdma_mode & IIDC_RDMA_PROTOCOL_ROCEV2;
should be
ctx->val.vbool = !!(pf->rdma_mode & IIDC_RDMA_PROTOCOL_ROCEV2);
 or
ctx->val.vbool = pf->rdma_mode & IIDC_RDMA_PROTOCOL_ROCEV2 ? true : false;

because & IIDC_RDMA_PROTOCOL_ROCEV2 is BIT(1) = 0x2.

> Setting rocev2 involves,
> pf->rdma_mode |= IIDC_RDMA_PROTOCOL_ROCEV2;
> 
> So this operation here should reflect correct value in vbool. I don't think this is
> a bug.
>
vbool assignment is incorrect, rest is fine.
 
> > > +static int
> > > +ice_devlink_enable_iw_get(struct devlink *devlink, u32 id,
> > > +			  struct devlink_param_gset_ctx *ctx) {
> > > +	struct ice_pf *pf = devlink_priv(devlink);
> > > +
> > > +	ctx->val.vbool = pf->rdma_mode & IIDC_RDMA_PROTOCOL_IWARP;
> > > +
> > This works fine as this is bit 0, but not for roce. So lets just do
> > boolean rdma_mode.
> 
> Boolean doesn't cut it as it doesn't reflect the disabled state mentioned above.
> 
Yes, you need bit fields with above fix.

> > > --- a/drivers/net/ethernet/intel/ice/ice_devlink.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice_devlink.h
> > > @@ -4,10 +4,16 @@
> > >  #ifndef _ICE_DEVLINK_H_
> > >  #define _ICE_DEVLINK_H_
> > >
> > > +enum ice_devlink_param_id {
> > > +	ICE_DEVLINK_PARAM_ID_BASE =
> > DEVLINK_PARAM_GENERIC_ID_MAX,
> > > };
> > > +
> > This is unused in the patch. Please remove.
> 
> Sure.
> 
> Between, Thanks for the review!
> 
> Shiraz
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ