RE: [PATCH net-next 2/3] net/ice: Add support for enable_iwarp and enable_roce devlink param

From: Saleem, Shiraz <redacted>
Sent: Tuesday, November 23, 2021 8:18 PM

quoted

Subject: RE: [PATCH net-next 2/3] net/ice: Add support for
enable_iwarp and enable_roce devlink param

Hi Tony,

quoted

From: Tony Nguyen <anthony.l.nguyen@intel.com>
Sent: Tuesday, November 23, 2021 2:41 AM

From: Shiraz Saleem <redacted>

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 <redacted>
Tested-by: Leszek Kaliszczuk <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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.

quoted

 	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 =

{

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

};
+

This is unused in the patch. Please remove.

Sure.

Between, Thanks for the review!

Shiraz