[PATCH net-next 00/12][pull request] 40GbE Intel Wired LAN Driver Updates 2021-11-24

[PATCH net-next 00/12][pull request] 40GbE Intel Wired LAN Driver Updates 2021-11-24

[Tony Nguyen]

This series contains updates to iavf driver only.

Mitch adds restoration of MSI state during reset and reduces the log
level on a couple messages.

Patryk adds an info message when MTU is changed.

Grzegorz adds messaging when transitioning in and out of multicast
promiscuous mode.

Jake returns correct error codes for iavf_parse_cls_flower().

Jedrzej adds messaging for when the driver is removed and refactors
struct usage to take less memory. He also adjusts ethtool statistics to
only display information on active queues.

Tony allows for user to specify the RSS hash.

Karen resolves some static analysis warnings, corrects format specifiers,
and rewords a message to come across as informational.

The following are changes since commit d156250018ab5adbcfcc9ea90455d5fba5df6769:
  Merge branch 'hns3-next'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 40GbE

Grzegorz Szczurek (1):
  iavf: Log info when VF is entering and leaving Allmulti mode

Jacob Keller (1):
  iavf: return errno code instead of status code

Jedrzej Jagielski (3):
  iavf: Add trace while removing device
  iavf: Refactor iavf_mac_filter struct memory usage
  iavf: Fix displaying queue statistics shown by ethtool

Karen Sornek (3):
  iavf: Fix static code analysis warning
  iavf: Refactor text of informational message
  iavf: Refactor string format to avoid static analysis warnings

Mitch Williams (2):
  iavf: restore MSI state on reset
  iavf: don't be so alarming

Patryk Małek (1):
  iavf: Add change MTU message

Tony Nguyen (1):
  iavf: Enable setting RSS hash key

 drivers/net/ethernet/intel/iavf/iavf.h        | 10 ++--
 .../net/ethernet/intel/iavf/iavf_ethtool.c    | 48 +++++++++++--------
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 32 +++++++------
 drivers/net/ethernet/intel/iavf/iavf_txrx.c   |  2 +-
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   | 28 +++++++----
 5 files changed, 73 insertions(+), 47 deletions(-)

-- 
2.31.1

[PATCH net-next 01/12] iavf: restore MSI state on reset

[Tony Nguyen]

From: Mitch Williams <mitch.a.williams@intel.com>

If the PF experiences an FLR, the VF's MSI and MSI-X configuration will
be conveniently and silently removed in the process. When this happens,
reset recovery will appear to complete normally but no traffic will
pass. The netdev watchdog will helpfully notify everyone of this issue.

To prevent such public embarrassment, restore MSI configuration at every
reset. For normal resets, this will do no harm, but for VF resets
resulting from a PF FLR, this will keep the VF working.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 336e6bf95e48..4aa8d1af8a31 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -2236,6 +2236,7 @@ static void iavf_reset_task(struct work_struct *work)
 	}
 
 	pci_set_master(adapter->pdev);
+	pci_restore_msi_state(adapter->pdev);
 
 	if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
 		dev_err(&adapter->pdev->dev, "Reset never finished (%x)\n",
-- 
2.31.1

Re: [PATCH net-next 01/12] iavf: restore MSI state on reset

[Jakub Kicinski]

On Wed, 24 Nov 2021 09:16:41 -0800 Tony Nguyen wrote:
> If the PF experiences an FLR, the VF's MSI and MSI-X configuration will
> be conveniently and silently removed in the process. When this happens,
> reset recovery will appear to complete normally but no traffic will
> pass. The netdev watchdog will helpfully notify everyone of this issue.
> 
> To prevent such public embarrassment, restore MSI configuration at every
> reset. For normal resets, this will do no harm, but for VF resets
> resulting from a PF FLR, this will keep the VF working.

Why is this not a fix?

Re: [PATCH net-next 01/12] iavf: restore MSI state on reset

[Nguyen, Anthony L]

On Wed, 2021-11-24 at 15:42 -0800, Jakub Kicinski wrote:
> On Wed, 24 Nov 2021 09:16:41 -0800 Tony Nguyen wrote:
> > If the PF experiences an FLR, the VF's MSI and MSI-X configuration
> > will
> > be conveniently and silently removed in the process. When this
> > happens,
> > reset recovery will appear to complete normally but no traffic will
> > pass. The netdev watchdog will helpfully notify everyone of this
> > issue.
> > 
> > To prevent such public embarrassment, restore MSI configuration at
> > every
> > reset. For normal resets, this will do no harm, but for VF resets
> > resulting from a PF FLR, this will keep the VF working.
> 
> Why is this not a fix?

As I'll need to do a v2 on this series, I'll go ahead and drop it here
and send it via net.

Thanks,
Tony

[PATCH net-next 02/12] iavf: Add change MTU message

[Tony Nguyen]

From: Patryk Małek <patryk.malek@intel.com>

Add a netdev_info log entry in case of a change of MTU so that user is
notified about this change in the same manner as in case of pf driver.

Signed-off-by: Patryk Małek <patryk.malek@intel.com>
Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 4aa8d1af8a31..858683b65adf 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3420,6 +3420,8 @@ static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct iavf_adapter *adapter = netdev_priv(netdev);
 
+	netdev_info(netdev, "changing MTU from %d to %d\n",
+		    netdev->mtu, new_mtu);
 	netdev->mtu = new_mtu;
 	if (CLIENT_ENABLED(adapter)) {
 		iavf_notify_client_l2_params(&adapter->vsi);
-- 
2.31.1

Re: [PATCH net-next 02/12] iavf: Add change MTU message

[Jakub Kicinski]

On Wed, 24 Nov 2021 09:16:42 -0800 Tony Nguyen wrote:
> Add a netdev_info log entry in case of a change of MTU so that user is
> notified about this change in the same manner as in case of pf driver.

Why is this an important piece of information, tho? Other major vendors
don't print this.

Re: [PATCH net-next 02/12] iavf: Add change MTU message

[Nguyen, Anthony L]

On Wed, 2021-11-24 at 15:46 -0800, Jakub Kicinski wrote:
> On Wed, 24 Nov 2021 09:16:42 -0800 Tony Nguyen wrote:
> > Add a netdev_info log entry in case of a change of MTU so that user
> > is
> > notified about this change in the same manner as in case of pf
> > driver.
> 
> Why is this an important piece of information, tho? Other major
> vendors
> don't print this.

I was going to say this was to match the behaviour of our other
drivers, however, after looking at the others, they are dev_dbgs. Would
that change work for you?

Thanks,
Tony

Re: [PATCH net-next 02/12] iavf: Add change MTU message

[Jakub Kicinski]

On Mon, 29 Nov 2021 18:43:26 +0000 Nguyen, Anthony L wrote:
> On Wed, 2021-11-24 at 15:46 -0800, Jakub Kicinski wrote:
> > On Wed, 24 Nov 2021 09:16:42 -0800 Tony Nguyen wrote:  
> > > Add a netdev_info log entry in case of a change of MTU so that user
> > > is
> > > notified about this change in the same manner as in case of pf
> > > driver.  
> > 
> > Why is this an important piece of information, tho? Other major
> > vendors
> > don't print this.  
> 
> I was going to say this was to match the behaviour of our other
> drivers, however, after looking at the others, they are dev_dbgs. Would
> that change work for you?

Yes, dbg() would be better, thanks.

[PATCH net-next 04/12] iavf: return errno code instead of status code

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The iavf_parse_cls_flower function returns an integer error code, and
not an iavf_status enumeration.

Fix the function to use the standard errno value EINVAL as its return
instead of using IAVF_ERR_CONFIG.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 858683b65adf..cc1b3caa5136 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -2902,7 +2902,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 			} else {
 				dev_err(&adapter->pdev->dev, "Bad ether dest mask %pM\n",
 					match.mask->dst);
-				return IAVF_ERR_CONFIG;
+				return -EINVAL;
 			}
 		}
 
@@ -2912,7 +2912,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 			} else {
 				dev_err(&adapter->pdev->dev, "Bad ether src mask %pM\n",
 					match.mask->src);
-				return IAVF_ERR_CONFIG;
+				return -EINVAL;
 			}
 		}
 
@@ -2947,7 +2947,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 			} else {
 				dev_err(&adapter->pdev->dev, "Bad vlan mask %u\n",
 					match.mask->vlan_id);
-				return IAVF_ERR_CONFIG;
+				return -EINVAL;
 			}
 		}
 		vf->mask.tcp_spec.vlan_id |= cpu_to_be16(0xffff);
@@ -2971,7 +2971,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 			} else {
 				dev_err(&adapter->pdev->dev, "Bad ip dst mask 0x%08x\n",
 					be32_to_cpu(match.mask->dst));
-				return IAVF_ERR_CONFIG;
+				return -EINVAL;
 			}
 		}
 
@@ -2981,13 +2981,13 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 			} else {
 				dev_err(&adapter->pdev->dev, "Bad ip src mask 0x%08x\n",
 					be32_to_cpu(match.mask->dst));
-				return IAVF_ERR_CONFIG;
+				return -EINVAL;
 			}
 		}
 
 		if (field_flags & IAVF_CLOUD_FIELD_TEN_ID) {
 			dev_info(&adapter->pdev->dev, "Tenant id not allowed for ip filter\n");
-			return IAVF_ERR_CONFIG;
+			return -EINVAL;
 		}
 		if (match.key->dst) {
 			vf->mask.tcp_spec.dst_ip[0] |= cpu_to_be32(0xffffffff);
@@ -3008,7 +3008,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 		if (ipv6_addr_any(&match.mask->dst)) {
 			dev_err(&adapter->pdev->dev, "Bad ipv6 dst mask 0x%02x\n",
 				IPV6_ADDR_ANY);
-			return IAVF_ERR_CONFIG;
+			return -EINVAL;
 		}
 
 		/* src and dest IPv6 address should not be LOOPBACK
@@ -3018,7 +3018,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 		    ipv6_addr_loopback(&match.key->src)) {
 			dev_err(&adapter->pdev->dev,
 				"ipv6 addr should not be loopback\n");
-			return IAVF_ERR_CONFIG;
+			return -EINVAL;
 		}
 		if (!ipv6_addr_any(&match.mask->dst) ||
 		    !ipv6_addr_any(&match.mask->src))
@@ -3043,7 +3043,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 			} else {
 				dev_err(&adapter->pdev->dev, "Bad src port mask %u\n",
 					be16_to_cpu(match.mask->src));
-				return IAVF_ERR_CONFIG;
+				return -EINVAL;
 			}
 		}
 
@@ -3053,7 +3053,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
 			} else {
 				dev_err(&adapter->pdev->dev, "Bad dst port mask %u\n",
 					be16_to_cpu(match.mask->dst));
-				return IAVF_ERR_CONFIG;
+				return -EINVAL;
 			}
 		}
 		if (match.key->dst) {
-- 
2.31.1

[PATCH net-next 05/12] iavf: don't be so alarming

[Tony Nguyen]

From: Mitch Williams <mitch.a.williams@intel.com>

Reduce the log level of a couple of messages. These can appear during normal
reset and rmmod processing, and the driver recovers just fine. Debug
level is fine for these.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c     | 2 +-
 drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index cc1b3caa5136..bb2e91cb9cd4 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3405,7 +3405,7 @@ static int iavf_close(struct net_device *netdev)
 				    adapter->state == __IAVF_DOWN,
 				    msecs_to_jiffies(500));
 	if (!status)
-		netdev_warn(netdev, "Device resources not yet released\n");
+		netdev_dbg(netdev, "Device resources not yet released\n");
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 250dc8bcaedd..9c0c9a7ee06d 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1899,8 +1899,8 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 		break;
 	default:
 		if (adapter->current_op && (v_opcode != adapter->current_op))
-			dev_warn(&adapter->pdev->dev, "Expected response %d from PF, received %d\n",
-				 adapter->current_op, v_opcode);
+			dev_dbg(&adapter->pdev->dev, "Expected response %d from PF, received %d\n",
+				adapter->current_op, v_opcode);
 		break;
 	} /* switch v_opcode */
 	adapter->current_op = VIRTCHNL_OP_UNKNOWN;
-- 
2.31.1

Re: [PATCH net-next 05/12] iavf: don't be so alarming

[Stefan Assmann]

On 2021-11-24 09:16, Tony Nguyen wrote:
> From: Mitch Williams <mitch.a.williams@intel.com>
> 
> Reduce the log level of a couple of messages. These can appear during normal
> reset and rmmod processing, and the driver recovers just fine. Debug
> level is fine for these.
> 
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c     | 2 +-
>  drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index cc1b3caa5136..bb2e91cb9cd4 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3405,7 +3405,7 @@ static int iavf_close(struct net_device *netdev)
>  				    adapter->state == __IAVF_DOWN,
>  				    msecs_to_jiffies(500));
>  	if (!status)
> -		netdev_warn(netdev, "Device resources not yet released\n");
> +		netdev_dbg(netdev, "Device resources not yet released\n");
>  	return 0;

This message in particular has been a good indicator for some irregular
behaviour in VF reset. I'd rather keep it the way it is or change it
netdev_info().

  Stefan

Re: [PATCH net-next 05/12] iavf: don't be so alarming

[Nguyen, Anthony L]

On Thu, 2021-11-25 at 07:45 +0100, Stefan Assmann wrote:
> On 2021-11-24 09:16, Tony Nguyen wrote:
> > From: Mitch Williams <mitch.a.williams@intel.com>
> > 
> > Reduce the log level of a couple of messages. These can appear
> > during normal
> > reset and rmmod processing, and the driver recovers just fine.
> > Debug
> > level is fine for these.
> > 
> > Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> > Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/iavf/iavf_main.c     | 2 +-
> >  drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index cc1b3caa5136..bb2e91cb9cd4 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -3405,7 +3405,7 @@ static int iavf_close(struct net_device
> > *netdev)
> >                                     adapter->state == __IAVF_DOWN,
> >                                     msecs_to_jiffies(500));
> >         if (!status)
> > -               netdev_warn(netdev, "Device resources not yet
> > released\n");
> > +               netdev_dbg(netdev, "Device resources not yet
> > released\n");
> >         return 0;
> 
> This message in particular has been a good indicator for some
> irregular
> behaviour in VF reset. I'd rather keep it the way it is or change it
> netdev_info().

I'll drop this from the series then to keep it as is.

Thanks,
Tony

[PATCH net-next 06/12] iavf: Add trace while removing device

[Tony Nguyen]

From: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

Add kernel trace that device was removed.
Currently there is no such information.
I.e. Host admin removes a PCI device from a VM,
than on VM shall be info about the event.

This patch adds info log to iavf_remove function.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index bb2e91cb9cd4..4019191e6b0c 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3987,6 +3987,7 @@ static void iavf_remove(struct pci_dev *pdev)
 	if (iavf_lock_timeout(&adapter->crit_lock, 5000))
 		dev_warn(&adapter->pdev->dev, "failed to acquire crit_lock in %s\n", __FUNCTION__);
 
+	dev_info(&adapter->pdev->dev, "Removing device\n");
 	/* Shut down all the garbage mashers on the detention level */
 	iavf_change_state(adapter, __IAVF_REMOVE);
 	adapter->aq_required = 0;
-- 
2.31.1

Re: [PATCH net-next 06/12] iavf: Add trace while removing device

[Jakub Kicinski]

On Wed, 24 Nov 2021 09:16:46 -0800 Tony Nguyen wrote:
> Add kernel trace that device was removed.
> Currently there is no such information.
> I.e. Host admin removes a PCI device from a VM,
> than on VM shall be info about the event.
> 
> This patch adds info log to iavf_remove function.

Why is this an important thing to print to logs about?
If it is why is PCI core not doing the printing?

Re: [PATCH net-next 06/12] iavf: Add trace while removing device

[Stefan Assmann]

On 2021-11-24 15:48, Jakub Kicinski wrote:
> On Wed, 24 Nov 2021 09:16:46 -0800 Tony Nguyen wrote:
> > Add kernel trace that device was removed.
> > Currently there is no such information.
> > I.e. Host admin removes a PCI device from a VM,
> > than on VM shall be info about the event.
> > 
> > This patch adds info log to iavf_remove function.
> 
> Why is this an important thing to print to logs about?
> If it is why is PCI core not doing the printing?
> 

From personal experience I'd say this piece of information has value,
especially when debugging it can be interesting to know exactly when the
driver was removed.

  Stefan

Re: [PATCH net-next 06/12] iavf: Add trace while removing device

[Jakub Kicinski]

On Thu, 25 Nov 2021 07:50:49 +0100 Stefan Assmann wrote:
> On 2021-11-24 15:48, Jakub Kicinski wrote:
> > On Wed, 24 Nov 2021 09:16:46 -0800 Tony Nguyen wrote:  
> > > Add kernel trace that device was removed.
> > > Currently there is no such information.
> > > I.e. Host admin removes a PCI device from a VM,
> > > than on VM shall be info about the event.
> > > 
> > > This patch adds info log to iavf_remove function.  
> > 
> > Why is this an important thing to print to logs about?
> > If it is why is PCI core not doing the printing?
> 
> From personal experience I'd say this piece of information has value,
> especially when debugging it can be interesting to know exactly when
> the driver was removed.

But there isn't anything specific to iavf here, right? If it really 
is important then core should be doing the printing for all drivers.

Actually, I can't come up with any uses for this print on the spot.
What debugging scenarios do you have in mind?

Re: [PATCH net-next 06/12] iavf: Add trace while removing device

[Stefan Assmann]

On 2021-11-25 07:13, Jakub Kicinski wrote:
> On Thu, 25 Nov 2021 07:50:49 +0100 Stefan Assmann wrote:
> > On 2021-11-24 15:48, Jakub Kicinski wrote:
> > > On Wed, 24 Nov 2021 09:16:46 -0800 Tony Nguyen wrote:  
> > > > Add kernel trace that device was removed.
> > > > Currently there is no such information.
> > > > I.e. Host admin removes a PCI device from a VM,
> > > > than on VM shall be info about the event.
> > > > 
> > > > This patch adds info log to iavf_remove function.  
> > > 
> > > Why is this an important thing to print to logs about?
> > > If it is why is PCI core not doing the printing?
> > 
> > From personal experience I'd say this piece of information has value,
> > especially when debugging it can be interesting to know exactly when
> > the driver was removed.
> 
> But there isn't anything specific to iavf here, right? If it really 
> is important then core should be doing the printing for all drivers.
> 
> Actually, I can't come up with any uses for this print on the spot.
> What debugging scenarios do you have in mind?

There was a lot of trouble with iavf in terms of device reset, device
unbinding (DPDK), stress testing of driver load/unload issues. When
looking through the crash logs it was not always easy to determine if
the driver was still loaded.
Especially on problems that weren't easy to reproduce.

So for iavf having that information would have been valuable. Not sure
if that justifies a PCI core message or if others might find that too
verbose.

  Stefan

Re: [PATCH net-next 06/12] iavf: Add trace while removing device

[Jakub Kicinski]

On Thu, 25 Nov 2021 16:43:49 +0100 Stefan Assmann wrote:
> On 2021-11-25 07:13, Jakub Kicinski wrote:
> > On Thu, 25 Nov 2021 07:50:49 +0100 Stefan Assmann wrote:  
> > > From personal experience I'd say this piece of information has value,
> > > especially when debugging it can be interesting to know exactly when
> > > the driver was removed.  
> > 
> > But there isn't anything specific to iavf here, right? If it really 
> > is important then core should be doing the printing for all drivers.
> > 
> > Actually, I can't come up with any uses for this print on the spot.
> > What debugging scenarios do you have in mind?  
> 
> There was a lot of trouble with iavf in terms of device reset, device
> unbinding (DPDK), stress testing of driver load/unload issues. When
> looking through the crash logs it was not always easy to determine if
> the driver was still loaded.
> Especially on problems that weren't easy to reproduce.

That's a slippery slope, historically we were always pushing for
avoiding informational prints. Otherwise every driver reconfig would
result in a line in the logs.

> So for iavf having that information would have been valuable. Not sure
> if that justifies a PCI core message or if others might find that too
> verbose.

So what you're saying is from your experience iavf is of significantly
poorer quality than other vendors' drivers?

Re: [PATCH net-next 06/12] iavf: Add trace while removing device

[Stefan Assmann]

On 2021-11-25 07:57, Jakub Kicinski wrote:
> On Thu, 25 Nov 2021 16:43:49 +0100 Stefan Assmann wrote:
> > On 2021-11-25 07:13, Jakub Kicinski wrote:
> > > On Thu, 25 Nov 2021 07:50:49 +0100 Stefan Assmann wrote:  
> > > > From personal experience I'd say this piece of information has value,
> > > > especially when debugging it can be interesting to know exactly when
> > > > the driver was removed.  
> > > 
> > > But there isn't anything specific to iavf here, right? If it really 
> > > is important then core should be doing the printing for all drivers.
> > > 
> > > Actually, I can't come up with any uses for this print on the spot.
> > > What debugging scenarios do you have in mind?  
> > 
> > There was a lot of trouble with iavf in terms of device reset, device
> > unbinding (DPDK), stress testing of driver load/unload issues. When
> > looking through the crash logs it was not always easy to determine if
> > the driver was still loaded.
> > Especially on problems that weren't easy to reproduce.
> 
> That's a slippery slope, historically we were always pushing for
> avoiding informational prints. Otherwise every driver reconfig would
> result in a line in the logs.
> 
> > So for iavf having that information would have been valuable. Not sure
> > if that justifies a PCI core message or if others might find that too
> > verbose.
> 
> So what you're saying is from your experience iavf is of significantly
> poorer quality than other vendors' drivers?

No, I can't really comment on how iavf compares to other vendor drivers
since I've mostly worked with Intel. There's a lot of async tasks and
communication going on between PF and VF and while that seems great it
has a lot of potential for things to go wrong as well. If you look at
the git history of iavf (previously i40evf) you'll see that the
especially the reset, shutdown, error handling code has seen a lot of
fixes and refactoring. Just recently in net-next
898ef1cb1cb2 iavf: Combine init and watchdog state machines
45eebd62999d iavf: Refactor iavf state machine tracking

Finding and fixing all those corner cases is hard, especially when we
have situation where we rely on firmware to perform a certain task in
defined time.
Example:
In iavf_reset_task() we poll a register for a certain amount of time as
the firmware is supposed to complete the reset in that amount of time.
Now that works well 99,999% of the time but sometime it doesn't so the
driver logs a message and continues operation.
That might work, it might not and things go wrong and we had to figure
out why.
Or some async message came in at a bad time, while the driver was
already being removed, and rescheduled the reset task which of course
caused a crash as the structures were being free'd underneath the reset
task.

So from the perspective of somebody working on the driver I'd like to
see it when the driver gets removed.

  Stefan

[PATCH net-next 07/12] iavf: Enable setting RSS hash key

[Tony Nguyen]

Driver support for changing the RSS hash key exists, however, checks
have caused it to be reported as unsupported. Remove the check and
allow the hash key to be specified.

Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
Tested-by: Tony Brelinski <tony.brelinski@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index d327f576136f..ddb3f9f1c58e 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -1885,7 +1885,7 @@ static int iavf_get_rxfh(struct net_device *netdev, u32 *indir, u8 *key,
  * @key: hash key
  * @hfunc: hash function to use
  *
- * Returns -EINVAL if the table specifies an inavlid queue id, otherwise
+ * Returns -EINVAL if the table specifies an invalid queue id, otherwise
  * returns 0 after programming the table.
  **/
 static int iavf_set_rxfh(struct net_device *netdev, const u32 *indir,
@@ -1894,19 +1894,21 @@ static int iavf_set_rxfh(struct net_device *netdev, const u32 *indir,
 	struct iavf_adapter *adapter = netdev_priv(netdev);
 	u16 i;
 
-	/* We do not allow change in unsupported parameters */
-	if (key ||
-	    (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP))
+	/* Only support toeplitz hash function */
+	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
 		return -EOPNOTSUPP;
-	if (!indir)
+
+	if (!key && !indir)
 		return 0;
 
 	if (key)
 		memcpy(adapter->rss_key, key, adapter->rss_key_size);
 
-	/* Each 32 bits pointed by 'indir' is stored with a lut entry */
-	for (i = 0; i < adapter->rss_lut_size; i++)
-		adapter->rss_lut[i] = (u8)(indir[i]);
+	if (indir) {
+		/* Each 32 bits pointed by 'indir' is stored with a lut entry */
+		for (i = 0; i < adapter->rss_lut_size; i++)
+			adapter->rss_lut[i] = (u8)(indir[i]);
+	}
 
 	return iavf_config_rss(adapter);
 }
-- 
2.31.1

[PATCH net-next 08/12] iavf: Refactor iavf_mac_filter struct memory usage

[Tony Nguyen]

From: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

iavf_mac_filter struct contained couple boolean
flags using up more memory than is necessary.
Change the flags to be bitfields in an anonymous struct
so all the flags now fit in one byte.

Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 75635bd57cf6..526d424a439d 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -137,9 +137,13 @@ struct iavf_q_vector {
 struct iavf_mac_filter {
 	struct list_head list;
 	u8 macaddr[ETH_ALEN];
-	bool is_new_mac;	/* filter is new, wait for PF decision */
-	bool remove;		/* filter needs to be removed */
-	bool add;		/* filter needs to be added */
+	struct {
+		u8 is_new_mac:1;    /* filter is new, wait for PF decision */
+		u8 remove:1;        /* filter needs to be removed */
+		u8 add:1;           /* filter needs to be added */
+		u8 is_primary:1;    /* filter is a default VF MAC */
+		u8 padding:4;
+	};
 };
 
 struct iavf_vlan_filter {
-- 
2.31.1

Re: [PATCH net-next 08/12] iavf: Refactor iavf_mac_filter struct memory usage

[Jakub Kicinski]

On Wed, 24 Nov 2021 09:16:48 -0800 Tony Nguyen wrote:
> -	bool is_new_mac;	/* filter is new, wait for PF decision */
> -	bool remove;		/* filter needs to be removed */
> -	bool add;		/* filter needs to be added */
> +	struct {
> +		u8 is_new_mac:1;    /* filter is new, wait for PF decision */
> +		u8 remove:1;        /* filter needs to be removed */
> +		u8 add:1;           /* filter needs to be added */
> +		u8 is_primary:1;    /* filter is a default VF MAC */
> +		u8 padding:4;
> +	};

Why did you wrap it in a struct? Just curious.

RE: [PATCH net-next 08/12] iavf: Refactor iavf_mac_filter struct memory usage

[Jagielski, Jedrzej]

Hello,

Just to group them so they all occupy one memory chunk and for it to be clear how and where to add new ones in the future.


-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org> 
Sent: czwartek, 25 listopada 2021 00:50
To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
Cc: davem@davemloft.net; Jagielski, Jedrzej <jedrzej.jagielski@intel.com>; netdev@vger.kernel.org; sassmann@redhat.com; Dziedziuch, SylwesterX <sylwesterx.dziedziuch@intel.com>; Jankowski, Konrad0 <konrad0.jankowski@intel.com>
Subject: Re: [PATCH net-next 08/12] iavf: Refactor iavf_mac_filter struct memory usage

On Wed, 24 Nov 2021 09:16:48 -0800 Tony Nguyen wrote:
> -	bool is_new_mac;	/* filter is new, wait for PF decision */
> -	bool remove;		/* filter needs to be removed */
> -	bool add;		/* filter needs to be added */
> +	struct {
> +		u8 is_new_mac:1;    /* filter is new, wait for PF decision */
> +		u8 remove:1;        /* filter needs to be removed */
> +		u8 add:1;           /* filter needs to be added */
> +		u8 is_primary:1;    /* filter is a default VF MAC */
> +		u8 padding:4;
> +	};

Why did you wrap it in a struct? Just curious.

[PATCH net-next 09/12] iavf: Fix static code analysis warning

[Tony Nguyen]

From: Karen Sornek <karen.sornek@intel.com>

Change min() to min_t() to fix static code analysis warning of possible
overflow.

Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Karen Sornek <karen.sornek@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index 3525eab8e9f9..42c9f9dc235c 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -1766,7 +1766,7 @@ int iavf_napi_poll(struct napi_struct *napi, int budget)
 	if (likely(napi_complete_done(napi, work_done)))
 		iavf_update_enable_itr(vsi, q_vector);
 
-	return min(work_done, budget - 1);
+	return min_t(int, work_done, budget - 1);
 }
 
 /**
-- 
2.31.1

[PATCH net-next 10/12] iavf: Refactor text of informational message

[Tony Nguyen]

From: Karen Sornek <karen.sornek@intel.com>

This message is intended to be informational to indicate a reset is about
to happen, but the use of "warning" in the message text can cause concern
with users.  Reword the message to make it less alarming.

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Karen Sornek <karen.sornek@intel.com>
Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 9c0c9a7ee06d..2285bd6e9186 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1518,7 +1518,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 			iavf_print_link_message(adapter);
 			break;
 		case VIRTCHNL_EVENT_RESET_IMPENDING:
-			dev_info(&adapter->pdev->dev, "Reset warning received from the PF\n");
+			dev_info(&adapter->pdev->dev, "Reset indication received from the PF\n");
 			if (!(adapter->flags & IAVF_FLAG_RESET_PENDING)) {
 				adapter->flags |= IAVF_FLAG_RESET_PENDING;
 				dev_info(&adapter->pdev->dev, "Scheduling reset task\n");
-- 
2.31.1

[PATCH net-next 11/12] iavf: Refactor string format to avoid static analysis warnings

[Tony Nguyen]

From: Karen Sornek <karen.sornek@intel.com>

Change format to match variable type that is used in string.

Use %u format for unsigned variable and %d format for signed variable
to remove static analysis warnings.

Signed-off-by: Michal Swiatkowski <michal.swiatkowski@intel.com>
Signed-off-by: Karen Sornek <karen.sornek@intel.com>
Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c     | 6 +++---
 drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 4019191e6b0c..125cef580c3b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -450,14 +450,14 @@ iavf_request_traffic_irqs(struct iavf_adapter *adapter, char *basename)
 
 		if (q_vector->tx.ring && q_vector->rx.ring) {
 			snprintf(q_vector->name, sizeof(q_vector->name),
-				 "iavf-%s-TxRx-%d", basename, rx_int_idx++);
+				 "iavf-%s-TxRx-%u", basename, rx_int_idx++);
 			tx_int_idx++;
 		} else if (q_vector->rx.ring) {
 			snprintf(q_vector->name, sizeof(q_vector->name),
-				 "iavf-%s-rx-%d", basename, rx_int_idx++);
+				 "iavf-%s-rx-%u", basename, rx_int_idx++);
 		} else if (q_vector->tx.ring) {
 			snprintf(q_vector->name, sizeof(q_vector->name),
-				 "iavf-%s-tx-%d", basename, tx_int_idx++);
+				 "iavf-%s-tx-%u", basename, tx_int_idx++);
 		} else {
 			/* skip this unused q_vector */
 			continue;
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 2285bd6e9186..bdc604036113 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1013,7 +1013,7 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
 	} else if (link_speed_mbps == SPEED_UNKNOWN) {
 		snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%s", "Unknown Mbps");
 	} else {
-		snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%u %s",
+		snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
 			 link_speed_mbps, "Mbps");
 	}
 
-- 
2.31.1

[PATCH net-next 12/12] iavf: Fix displaying queue statistics shown by ethtool

[Tony Nguyen]

From: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

Driver provided too many lines as an output to ethtool -S command.
Return actual length of string set of ethtool stats. Instead of predefined
maximal value use the actual value on netdev, iterate over active queues.
Without this patch, ethtool -S report would produce additional
erroneous lines of queues that are not configured.

Signed-off-by: Witold Fijalkowski <witoldx.fijalkowski@intel.com>
Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../net/ethernet/intel/iavf/iavf_ethtool.c    | 30 ++++++++++++-------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index ddb3f9f1c58e..8db3e8b68bf1 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -331,9 +331,16 @@ static int iavf_get_link_ksettings(struct net_device *netdev,
  **/
 static int iavf_get_sset_count(struct net_device *netdev, int sset)
 {
+	/* Report the maximum number queues, even if not every queue is
+	 * currently configured. Since allocation of queues is in pairs,
+	 * use netdev->real_num_tx_queues * 2. The real_num_tx_queues is set
+	 * at device creation and never changes.
+	 */
+
 	if (sset == ETH_SS_STATS)
 		return IAVF_STATS_LEN +
-			(IAVF_QUEUE_STATS_LEN * 2 * IAVF_MAX_REQ_QUEUES);
+			(IAVF_QUEUE_STATS_LEN * 2 *
+			 netdev->real_num_tx_queues);
 	else if (sset == ETH_SS_PRIV_FLAGS)
 		return IAVF_PRIV_FLAGS_STR_LEN;
 	else
@@ -357,17 +364,18 @@ static void iavf_get_ethtool_stats(struct net_device *netdev,
 	iavf_add_ethtool_stats(&data, adapter, iavf_gstrings_stats);
 
 	rcu_read_lock();
-	for (i = 0; i < IAVF_MAX_REQ_QUEUES; i++) {
+	/* As num_active_queues describe both tx and rx queues, we can use
+	 * it to iterate over rings' stats.
+	 */
+	for (i = 0; i < adapter->num_active_queues; i++) {
 		struct iavf_ring *ring;
 
-		/* Avoid accessing un-allocated queues */
-		ring = (i < adapter->num_active_queues ?
-			&adapter->tx_rings[i] : NULL);
+		/* Tx rings stats */
+		ring = &adapter->tx_rings[i];
 		iavf_add_queue_stats(&data, ring);
 
-		/* Avoid accessing un-allocated queues */
-		ring = (i < adapter->num_active_queues ?
-			&adapter->rx_rings[i] : NULL);
+		/* Rx rings stats */
+		ring = &adapter->rx_rings[i];
 		iavf_add_queue_stats(&data, ring);
 	}
 	rcu_read_unlock();
@@ -404,10 +412,10 @@ static void iavf_get_stat_strings(struct net_device *netdev, u8 *data)
 
 	iavf_add_stat_strings(&data, iavf_gstrings_stats);
 
-	/* Queues are always allocated in pairs, so we just use num_tx_queues
-	 * for both Tx and Rx queues.
+	/* Queues are always allocated in pairs, so we just use
+	 * real_num_tx_queues for both Tx and Rx queues.
 	 */
-	for (i = 0; i < netdev->num_tx_queues; i++) {
+	for (i = 0; i < netdev->real_num_tx_queues; i++) {
 		iavf_add_stat_strings(&data, iavf_gstrings_queue_stats,
 				      "tx", i);
 		iavf_add_stat_strings(&data, iavf_gstrings_queue_stats,
-- 
2.31.1

Re: [PATCH net-next 00/12][pull request] 40GbE Intel Wired LAN Driver Updates 2021-11-24

[Jakub Kicinski]

On Wed, 24 Nov 2021 09:16:40 -0800 Tony Nguyen wrote:
> Mitch adds restoration of MSI state during reset and reduces the log
> level on a couple messages.
> 
> Patryk adds an info message when MTU is changed.
> 
> Grzegorz adds messaging when transitioning in and out of multicast
> promiscuous mode.
> 
> Jake returns correct error codes for iavf_parse_cls_flower().
> 
> Jedrzej adds messaging for when the driver is removed and refactors
> struct usage to take less memory. He also adjusts ethtool statistics to
> only display information on active queues.
> 
> Tony allows for user to specify the RSS hash.
> 
> Karen resolves some static analysis warnings, corrects format specifiers,
> and rewords a message to come across as informational.

Looks like patch 03/12 still hasn't gotten to the ML :S

Re: [PATCH net-next 00/12][pull request] 40GbE Intel Wired LAN Driver Updates 2021-11-24

[Nguyen, Anthony L]

On Wed, 2021-11-24 at 15:42 -0800, Jakub Kicinski wrote:
> On Wed, 24 Nov 2021 09:16:40 -0800 Tony Nguyen wrote:
> > Mitch adds restoration of MSI state during reset and reduces the
> > log
> > level on a couple messages.
> > 
> > Patryk adds an info message when MTU is changed.
> > 
> > Grzegorz adds messaging when transitioning in and out of multicast
> > promiscuous mode.
> > 
> > Jake returns correct error codes for iavf_parse_cls_flower().
> > 
> > Jedrzej adds messaging for when the driver is removed and refactors
> > struct usage to take less memory. He also adjusts ethtool
> > statistics to
> > only display information on active queues.
> > 
> > Tony allows for user to specify the RSS hash.
> > 
> > Karen resolves some static analysis warnings, corrects format
> > specifiers,
> > and rewords a message to come across as informational.
> 
> Looks like patch 03/12 still hasn't gotten to the ML :S

I looked back on the email and somehow patch 3 didn't get netdev list
added. There will be a v2, so I'll make sure the netdev list in on all
of them.

Thanks,
Tony