[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-07-25

[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-07-25

[Tony Nguyen]

This series contains updates to iavf driver only.

Przemyslaw prevents setting of TC max rate below minimum supported values
and reports updated queue counts when setting up TCs.

Sudheer prevents configuration of TC filters when TC offloads are not
enabled.

The following are changes since commit 9af0620de1e118666881376f6497d1785758b04c:
  Merge branch 'net-sysctl-races-part-6'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

Przemyslaw Patynowski (2):
  iavf: Fix max_rate limiting
  iavf: Fix 'tc qdisc show' listing too many queues

Sudheer Mogilappagari (1):
  iavf: enable tc filter configuration only if hw-tc-offload is on

 drivers/net/ethernet/intel/iavf/iavf.h      |  6 +++
 drivers/net/ethernet/intel/iavf/iavf_main.c | 52 ++++++++++++++++++++-
 2 files changed, 56 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH net 1/3] iavf: Fix max_rate limiting

[Tony Nguyen]

From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>

Fix max_rate option in TC, check for proper quanta boundaries.
Check for minimum value provided and if it fits expected 50Mbps
quanta.

Without this patch, iavf could send settings for max_rate limiting
that would be accepted from by PF even the max_rate option is less
than expected 50Mbps quanta. It results in no rate limiting
on traffic as rate limiting will be floored to 0.

Example:
tc qdisc add dev $vf root mqprio num_tc 3 map 0 2 1 queues \
2@0 2@2 2@4 hw 1 mode channel shaper bw_rlimit \
max_rate 50Mbps 500Mbps 500Mbps

Should limit TC0 to circa 50 Mbps

tc qdisc add dev $vf root mqprio num_tc 3 map 0 2 1 queues \
2@0 2@2 2@4 hw 1 mode channel shaper bw_rlimit \
max_rate 0Mbps 100Kbit 500Mbps

Should return error

Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Signed-off-by: Jun Zhang <xuejun.zhang@intel.com>
Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h      |  1 +
 drivers/net/ethernet/intel/iavf/iavf_main.c | 25 +++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 0ea0361cd86b..c241fbc30f93 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -92,6 +92,7 @@ struct iavf_vsi {
 #define IAVF_HKEY_ARRAY_SIZE ((IAVF_VFQF_HKEY_MAX_INDEX + 1) * 4)
 #define IAVF_HLUT_ARRAY_SIZE ((IAVF_VFQF_HLUT_MAX_INDEX + 1) * 4)
 #define IAVF_MBPS_DIVISOR	125000 /* divisor to convert to Mbps */
+#define IAVF_MBPS_QUANTA	50
 
 #define IAVF_VIRTCHNL_VF_RESOURCE_SIZE (sizeof(struct virtchnl_vf_resource) + \
 					(IAVF_MAX_VF_VSI * \
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 2e2c153ce46a..51ae10eb348c 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3322,6 +3322,7 @@ static int iavf_validate_ch_config(struct iavf_adapter *adapter,
 				   struct tc_mqprio_qopt_offload *mqprio_qopt)
 {
 	u64 total_max_rate = 0;
+	u32 tx_rate_rem = 0;
 	int i, num_qps = 0;
 	u64 tx_rate = 0;
 	int ret = 0;
@@ -3336,12 +3337,32 @@ static int iavf_validate_ch_config(struct iavf_adapter *adapter,
 			return -EINVAL;
 		if (mqprio_qopt->min_rate[i]) {
 			dev_err(&adapter->pdev->dev,
-				"Invalid min tx rate (greater than 0) specified\n");
+				"Invalid min tx rate (greater than 0) specified for TC%d\n",
+				i);
 			return -EINVAL;
 		}
-		/*convert to Mbps */
+
+		/* convert to Mbps */
 		tx_rate = div_u64(mqprio_qopt->max_rate[i],
 				  IAVF_MBPS_DIVISOR);
+
+		if (mqprio_qopt->max_rate[i] &&
+		    tx_rate < IAVF_MBPS_QUANTA) {
+			dev_err(&adapter->pdev->dev,
+				"Invalid max tx rate for TC%d, minimum %dMbps\n",
+				i, IAVF_MBPS_QUANTA);
+			return -EINVAL;
+		}
+
+		(void)div_u64_rem(tx_rate, IAVF_MBPS_QUANTA, &tx_rate_rem);
+
+		if (tx_rate_rem != 0) {
+			dev_err(&adapter->pdev->dev,
+				"Invalid max tx rate for TC%d, not divisible by %d\n",
+				i, IAVF_MBPS_QUANTA);
+			return -EINVAL;
+		}
+
 		total_max_rate += tx_rate;
 		num_qps += mqprio_qopt->qopt.count[i];
 	}
-- 
2.35.1

[PATCH net 2/3] iavf: Fix 'tc qdisc show' listing too many queues

[Tony Nguyen]

From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>

Fix tc qdisc show dev <ethX> root displaying too many fq_codel qdiscs.
tc_modify_qdisc, which is caller of ndo_setup_tc, expects driver to call
netif_set_real_num_tx_queues, which prepares qdiscs.
Without this patch, fq_codel qdiscs would not be adjusted to number of
queues on VF.
e.g.:
tc qdisc show dev <ethX>
qdisc mq 0: root
qdisc fq_codel 0: parent :4 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
qdisc fq_codel 0: parent :3 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
qdisc fq_codel 0: parent :2 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
qdisc fq_codel 0: parent :1 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
tc qdisc add dev <ethX> root mqprio num_tc 2 map 1 0 0 0 0 0 0 0 queues 1@0 1@1 hw 1 mode channel shaper bw_rlimit max_rate 5000Mbit 150Mbit
tc qdisc show dev <ethX>
qdisc mqprio 8003: root tc 2 map 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
             queues:(0:0) (1:1)
             mode:channel
             shaper:bw_rlimit   max_rate:5Gbit 150Mbit
qdisc fq_codel 0: parent 8003:4 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
qdisc fq_codel 0: parent 8003:3 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
qdisc fq_codel 0: parent 8003:2 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
qdisc fq_codel 0: parent 8003:1 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64

While after fix:
tc qdisc add dev <ethX> root mqprio num_tc 2 map 1 0 0 0 0 0 0 0 queues 1@0 1@1 hw 1 mode channel shaper bw_rlimit max_rate 5000Mbit 150Mbit
tc qdisc show dev <ethX> #should show 2, shows 4
qdisc mqprio 8004: root tc 2 map 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
             queues:(0:0) (1:1)
             mode:channel
             shaper:bw_rlimit   max_rate:5Gbit 150Mbit
qdisc fq_codel 0: parent 8004:2 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
qdisc fq_codel 0: parent 8004:1 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64

Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Co-developed-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
Co-developed-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h      |  5 +++++
 drivers/net/ethernet/intel/iavf/iavf_main.c | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index c241fbc30f93..a988c08e906f 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -431,6 +431,11 @@ struct iavf_adapter {
 	/* lock to protect access to the cloud filter list */
 	spinlock_t cloud_filter_list_lock;
 	u16 num_cloud_filters;
+	/* snapshot of "num_active_queues" before setup_tc for qdisc add
+	 * is invoked. This information is useful during qdisc del flow,
+	 * to restore correct number of queues
+	 */
+	int orig_num_active_queues;
 
 #define IAVF_MAX_FDIR_FILTERS 128	/* max allowed Flow Director filters */
 	u16 fdir_active_fltr;
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 51ae10eb348c..3dbfaead2ac7 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3429,6 +3429,7 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
 			netif_tx_disable(netdev);
 			iavf_del_all_cloud_filters(adapter);
 			adapter->aq_required = IAVF_FLAG_AQ_DISABLE_CHANNELS;
+			total_qps = adapter->orig_num_active_queues;
 			goto exit;
 		} else {
 			return -EINVAL;
@@ -3472,7 +3473,21 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
 				adapter->ch_config.ch_info[i].offset = 0;
 			}
 		}
+
+		/* Take snapshot of original config such as "num_active_queues"
+		 * It is used later when delete ADQ flow is exercised, so that
+		 * once delete ADQ flow completes, VF shall go back to its
+		 * original queue configuration
+		 */
+
+		adapter->orig_num_active_queues = adapter->num_active_queues;
+
+		/* Store queue info based on TC so that VF gets configured
+		 * with correct number of queues when VF completes ADQ config
+		 * flow
+		 */
 		adapter->ch_config.total_qps = total_qps;
+
 		netif_tx_stop_all_queues(netdev);
 		netif_tx_disable(netdev);
 		adapter->aq_required |= IAVF_FLAG_AQ_ENABLE_CHANNELS;
@@ -3489,6 +3504,12 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
 		}
 	}
 exit:
+	if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
+		return 0;
+
+	netif_set_real_num_rx_queues(netdev, total_qps);
+	netif_set_real_num_tx_queues(netdev, total_qps);
+
 	return ret;
 }
 
-- 
2.35.1

[PATCH net 3/3] iavf: enable tc filter configuration only if hw-tc-offload is on

[Tony Nguyen]

From: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>

Allow configuration of tc filter only if NETIF_F_HW_TC is set for the
device.

Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
Signed-off-by: Jun Zhang <xuejun.zhang@intel.com>
Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 3dbfaead2ac7..9279bb37e4aa 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3802,6 +3802,12 @@ static int iavf_configure_clsflower(struct iavf_adapter *adapter,
 		return -EINVAL;
 	}
 
+	if (!(adapter->netdev->features & NETIF_F_HW_TC)) {
+		dev_err(&adapter->pdev->dev,
+			"Can't apply TC flower filters, turn ON hw-tc-offload and try again");
+		return -EOPNOTSUPP;
+	}
+
 	filter = kzalloc(sizeof(*filter), GFP_KERNEL);
 	if (!filter)
 		return -ENOMEM;
-- 
2.35.1

Re: [PATCH net 3/3] iavf: enable tc filter configuration only if hw-tc-offload is on

[Jakub Kicinski]

On Mon, 25 Jul 2022 10:04:52 -0700 Tony Nguyen wrote:
> From: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
> 
> Allow configuration of tc filter only if NETIF_F_HW_TC is set for the
> device.
> 
> Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
> Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
> Signed-off-by: Jun Zhang <xuejun.zhang@intel.com>
> Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 3dbfaead2ac7..9279bb37e4aa 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3802,6 +3802,12 @@ static int iavf_configure_clsflower(struct iavf_adapter *adapter,
>  		return -EINVAL;
>  	}
>  
> +	if (!(adapter->netdev->features & NETIF_F_HW_TC)) {
> +		dev_err(&adapter->pdev->dev,
> +			"Can't apply TC flower filters, turn ON hw-tc-offload and try again");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	filter = kzalloc(sizeof(*filter), GFP_KERNEL);
>  	if (!filter)
>  		return -ENOMEM;

tc_can_offload() checks this in the core already, no?

RE: [PATCH net 3/3] iavf: enable tc filter configuration only if hw-tc-offload is on

[Mogilappagari, Sudheer]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, July 25, 2022 7:46 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com>;
> netdev@vger.kernel.org; Zhang, Xuejun <xuejun.zhang@intel.com>; Sreenivas,
> Bharathi <bharathi.sreenivas@intel.com>
> Subject: Re: [PATCH net 3/3] iavf: enable tc filter configuration only if
> hw-tc-offload is on
> 
> On Mon, 25 Jul 2022 10:04:52 -0700 Tony Nguyen wrote:
> > From: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
> >
> > Allow configuration of tc filter only if NETIF_F_HW_TC is set for the
> > device.
> >
> > Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
> > Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
> > Signed-off-by: Jun Zhang <xuejun.zhang@intel.com>
> > Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index 3dbfaead2ac7..9279bb37e4aa 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -3802,6 +3802,12 @@ static int iavf_configure_clsflower(struct
> iavf_adapter *adapter,
> >  		return -EINVAL;
> >  	}
> >
> > +	if (!(adapter->netdev->features & NETIF_F_HW_TC)) {
> > +		dev_err(&adapter->pdev->dev,
> > +			"Can't apply TC flower filters, turn ON hw-tc-offload
> and try again");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> >  	filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> >  	if (!filter)
> >  		return -ENOMEM;
> 
> tc_can_offload() checks this in the core already, no?

Hi Jakub,
Seems like there is no check in core code in this path. Tested again
to confirm that no error is thrown by core code. Below is the call
trace while adding filter.
[  927.358001]  dump_stack_lvl+0x44/0x58
[  927.358009]  ice_add_cls_flower+0x73/0x90 [ice]
[  927.358066]  tc_setup_cb_add+0xc7/0x1e0
[  927.358074]  fl_hw_replace_filter+0x143/0x1e0 [cls_flower]
[  927.358081]  fl_change+0xbc3/0xed8 [cls_flower]
[  927.358086]  tc_new_tfilter+0x382/0xbc0

Thanks
Sudheer

Re: [PATCH net 3/3] iavf: enable tc filter configuration only if hw-tc-offload is on

[Jakub Kicinski]

On Wed, 27 Jul 2022 23:37:27 +0000 Mogilappagari, Sudheer wrote:
> > > +	if (!(adapter->netdev->features & NETIF_F_HW_TC)) {
> > > +		dev_err(&adapter->pdev->dev,
> > > +			"Can't apply TC flower filters, turn ON hw-tc-offload and try again");  
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > >  	filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> > >  	if (!filter)
> > >  		return -ENOMEM;  
> > 
> > tc_can_offload() checks this in the core already, no?  
> 
> Hi Jakub,
> Seems like there is no check in core code in this path. Tested again
> to confirm that no error is thrown by core code. Below is the call
> trace while adding filter.
> [  927.358001]  dump_stack_lvl+0x44/0x58
> [  927.358009]  ice_add_cls_flower+0x73/0x90 [ice]
> [  927.358066]  tc_setup_cb_add+0xc7/0x1e0
> [  927.358074]  fl_hw_replace_filter+0x143/0x1e0 [cls_flower]
> [  927.358081]  fl_change+0xbc3/0xed8 [cls_flower]
> [  927.358086]  tc_new_tfilter+0x382/0xbc0

Oh, you're right, we moved to drivers doing the check it seems.

But you already have a check in iavf_setup_tc_block_cb()
- tc_cls_can_offload_and_chain0() should validate the device has
TC offload enabled. It that not working somehow?

RE: [PATCH net 3/3] iavf: enable tc filter configuration only if hw-tc-offload is on

[Mogilappagari, Sudheer]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, July 27, 2022 6:11 PM
> To: Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org; Zhang,
> Xuejun <xuejun.zhang@intel.com>; Sreenivas, Bharathi
> <bharathi.sreenivas@intel.com>
> Subject: Re: [PATCH net 3/3] iavf: enable tc filter configuration only if
> hw-tc-offload is on
> 
> On Wed, 27 Jul 2022 23:37:27 +0000 Mogilappagari, Sudheer wrote:
> > > > +	if (!(adapter->netdev->features & NETIF_F_HW_TC)) {
> > > > +		dev_err(&adapter->pdev->dev,
> > > > +			"Can't apply TC flower filters, turn ON hw-tc-
> offload and try again");
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > > +
> > > >  	filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> > > >  	if (!filter)
> > > >  		return -ENOMEM;
> > >
> > > tc_can_offload() checks this in the core already, no?
> >
> > Hi Jakub,
> > Seems like there is no check in core code in this path. Tested again
> > to confirm that no error is thrown by core code. Below is the call
> > trace while adding filter.
> > [  927.358001]  dump_stack_lvl+0x44/0x58 [  927.358009]
> > ice_add_cls_flower+0x73/0x90 [ice] [  927.358066]
> > tc_setup_cb_add+0xc7/0x1e0 [  927.358074]
> > fl_hw_replace_filter+0x143/0x1e0 [cls_flower] [  927.358081]
> > fl_change+0xbc3/0xed8 [cls_flower] [  927.358086]
> > tc_new_tfilter+0x382/0xbc0
> 
> Oh, you're right, we moved to drivers doing the check it seems.
> 
> But you already have a check in iavf_setup_tc_block_cb()
> - tc_cls_can_offload_and_chain0() should validate the device has TC offload
> enabled. It that not working somehow?

Hi Jakub,
You are correct. tc_cls_can_offload_and_chain0() check in iavf_setup_tc_block_cb()
is taking caring of it already. So, this patch is not required.

I had tested with ice driver since it was same as iavf wrt this check.
We will modify ice driver to use tc_cls_can_offload_and_chain0().

Thanks
Sudheer