[PATCH 0/2 net-next v3] igb/igc: fix XDP registration

[PATCH 0/2 net-next v3] igb/igc: fix XDP registration

[Corinna Vinschen]

Fix the kernel warning "Missing unregister, handled but fix driver"
when running, e.g.,

  $ ethtool -G eth0 rx 1024

on igc.  Remove memset hack from igb and align igb code to igc. 

v3: use dev_err rather than netdev_err, just as in error case.

Corinna Vinschen (2):
  igc: avoid kernel warning when changing RX ring parameters
  igb: refactor XDP registration

 drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ----
 drivers/net/ethernet/intel/igb/igb_main.c    | 14 ++++++++++----
 drivers/net/ethernet/intel/igc/igc_main.c    | 20 +++++++++++---------
 3 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.27.0

[Intel-wired-lan] [PATCH 0/2 net-next v3] igb/igc: fix XDP registration

[Corinna Vinschen]

Fix the kernel warning "Missing unregister, handled but fix driver"
when running, e.g.,

  $ ethtool -G eth0 rx 1024

on igc.  Remove memset hack from igb and align igb code to igc. 

v3: use dev_err rather than netdev_err, just as in error case.

Corinna Vinschen (2):
  igc: avoid kernel warning when changing RX ring parameters
  igb: refactor XDP registration

 drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ----
 drivers/net/ethernet/intel/igb/igb_main.c    | 14 ++++++++++----
 drivers/net/ethernet/intel/igc/igc_main.c    | 20 +++++++++++---------
 3 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.27.0

[PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Corinna Vinschen]

Calling ethtool changing the RX ring parameters like this:

  $ ethtool -G eth0 rx 1024

on igc triggers the "Missing unregister, handled but fix driver" warning in
xdp_rxq_info_reg().

igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
reset the xdp_rxq_info member before calling igc_setup_rx_resources().
This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.

Make sure to unregister the xdp_rxq_info structure first in
igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.

Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 2f17f36e94fd..97144f6db36e 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -505,14 +505,6 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
 	u8 index = rx_ring->queue_index;
 	int size, desc_len, res;
 
-	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
-			       rx_ring->q_vector->napi.napi_id);
-	if (res < 0) {
-		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
-			   index);
-		return res;
-	}
-
 	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
 	rx_ring->rx_buffer_info = vzalloc(size);
 	if (!rx_ring->rx_buffer_info)
@@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
 	rx_ring->next_to_clean = 0;
 	rx_ring->next_to_use = 0;
 
+	/* XDP RX-queue info */
+	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
+		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
+	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
+			       rx_ring->q_vector->napi.napi_id);
+	if (res < 0) {
+		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
+			   index);
+		return res;
+	}
+
 	return 0;
 
 err:
-	xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
 	vfree(rx_ring->rx_buffer_info);
 	rx_ring->rx_buffer_info = NULL;
 	netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n");
-- 
2.27.0

[Intel-wired-lan] [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Corinna Vinschen]

Calling ethtool changing the RX ring parameters like this:

  $ ethtool -G eth0 rx 1024

on igc triggers the "Missing unregister, handled but fix driver" warning in
xdp_rxq_info_reg().

igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
reset the xdp_rxq_info member before calling igc_setup_rx_resources().
This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.

Make sure to unregister the xdp_rxq_info structure first in
igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.

Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 2f17f36e94fd..97144f6db36e 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -505,14 +505,6 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
 	u8 index = rx_ring->queue_index;
 	int size, desc_len, res;
 
-	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
-			       rx_ring->q_vector->napi.napi_id);
-	if (res < 0) {
-		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
-			   index);
-		return res;
-	}
-
 	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
 	rx_ring->rx_buffer_info = vzalloc(size);
 	if (!rx_ring->rx_buffer_info)
@@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
 	rx_ring->next_to_clean = 0;
 	rx_ring->next_to_use = 0;
 
+	/* XDP RX-queue info */
+	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
+		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
+	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
+			       rx_ring->q_vector->napi.napi_id);
+	if (res < 0) {
+		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
+			   index);
+		return res;
+	}
+
 	return 0;
 
 err:
-	xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
 	vfree(rx_ring->rx_buffer_info);
 	rx_ring->rx_buffer_info = NULL;
 	netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n");
-- 
2.27.0

[PATCH 2/2 net-next v3] igb: refactor XDP registration

[Corinna Vinschen]

On changing the RX ring parameters igb uses a hack to avoid a warning
when calling xdp_rxq_info_reg via igb_setup_rx_resources.  It just
clears the struct xdp_rxq_info content.

Change this to unregister if we're already registered instead.  ALign
code to the igc code.

Fixes: 9cbc948b5a20c ("igb: add XDP support")
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ----
 drivers/net/ethernet/intel/igb/igb_main.c    | 14 ++++++++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 51a2dcaf553d..2a5782063f4c 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -965,10 +965,6 @@ static int igb_set_ringparam(struct net_device *netdev,
 			memcpy(&temp_ring[i], adapter->rx_ring[i],
 			       sizeof(struct igb_ring));
 
-			/* Clear copied XDP RX-queue info */
-			memset(&temp_ring[i].xdp_rxq, 0,
-			       sizeof(temp_ring[i].xdp_rxq));
-
 			temp_ring[i].count = new_rx_count;
 			err = igb_setup_rx_resources(&temp_ring[i]);
 			if (err) {
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 38ba92022cd4..bfcf27177f33 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4352,7 +4352,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
 {
 	struct igb_adapter *adapter = netdev_priv(rx_ring->netdev);
 	struct device *dev = rx_ring->dev;
-	int size;
+	int size, res;
 
 	size = sizeof(struct igb_rx_buffer) * rx_ring->count;
 
@@ -4376,9 +4376,15 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
 	rx_ring->xdp_prog = adapter->xdp_prog;
 
 	/* XDP RX-queue info */
-	if (xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev,
-			     rx_ring->queue_index, 0) < 0)
-		goto err;
+	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
+		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
+	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev,
+			       rx_ring->queue_index, 0);
+	if (res < 0) {
+		dev_err(dev, "Failed to register xdp_rxq index %u\n",
+			rx_ring->queue_index);
+		return res;
+	}
 
 	return 0;
 
-- 
2.27.0

[Intel-wired-lan] [PATCH 2/2 net-next v3] igb: refactor XDP registration

[Corinna Vinschen]

On changing the RX ring parameters igb uses a hack to avoid a warning
when calling xdp_rxq_info_reg via igb_setup_rx_resources.  It just
clears the struct xdp_rxq_info content.

Change this to unregister if we're already registered instead.  ALign
code to the igc code.

Fixes: 9cbc948b5a20c ("igb: add XDP support")
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ----
 drivers/net/ethernet/intel/igb/igb_main.c    | 14 ++++++++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 51a2dcaf553d..2a5782063f4c 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -965,10 +965,6 @@ static int igb_set_ringparam(struct net_device *netdev,
 			memcpy(&temp_ring[i], adapter->rx_ring[i],
 			       sizeof(struct igb_ring));
 
-			/* Clear copied XDP RX-queue info */
-			memset(&temp_ring[i].xdp_rxq, 0,
-			       sizeof(temp_ring[i].xdp_rxq));
-
 			temp_ring[i].count = new_rx_count;
 			err = igb_setup_rx_resources(&temp_ring[i]);
 			if (err) {
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 38ba92022cd4..bfcf27177f33 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4352,7 +4352,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
 {
 	struct igb_adapter *adapter = netdev_priv(rx_ring->netdev);
 	struct device *dev = rx_ring->dev;
-	int size;
+	int size, res;
 
 	size = sizeof(struct igb_rx_buffer) * rx_ring->count;
 
@@ -4376,9 +4376,15 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
 	rx_ring->xdp_prog = adapter->xdp_prog;
 
 	/* XDP RX-queue info */
-	if (xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev,
-			     rx_ring->queue_index, 0) < 0)
-		goto err;
+	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
+		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
+	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev,
+			       rx_ring->queue_index, 0);
+	if (res < 0) {
+		dev_err(dev, "Failed to register xdp_rxq index %u\n",
+			rx_ring->queue_index);
+		return res;
+	}
 
 	return 0;
 
-- 
2.27.0

Re: [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Vinicius Costa Gomes]

Corinna Vinschen <vinschen@redhat.com> writes:

> Calling ethtool changing the RX ring parameters like this:
>
>   $ ethtool -G eth0 rx 1024
>
> on igc triggers the "Missing unregister, handled but fix driver" warning in
> xdp_rxq_info_reg().
>
> igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
> reset the xdp_rxq_info member before calling igc_setup_rx_resources().
> This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.
>
> Make sure to unregister the xdp_rxq_info structure first in
> igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
> action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.
>
> Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 2f17f36e94fd..97144f6db36e 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -505,14 +505,6 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
>  	u8 index = rx_ring->queue_index;
>  	int size, desc_len, res;
>  
> -	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> -			       rx_ring->q_vector->napi.napi_id);
> -	if (res < 0) {
> -		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> -			   index);
> -		return res;
> -	}
> -
>  	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
>  	rx_ring->rx_buffer_info = vzalloc(size);
>  	if (!rx_ring->rx_buffer_info)
> @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
>  	rx_ring->next_to_clean = 0;
>  	rx_ring->next_to_use = 0;
>  
> +	/* XDP RX-queue info */
> +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> +			       rx_ring->q_vector->napi.napi_id);
> +	if (res < 0) {
> +		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> +			   index);
> +		return res;

Here and in the igb patch, it should be 'goto err', no?

Another suggestion is to add the warning that Lennert reported in the
commit message (the comment from Maciej in that other thread).

Apart from that, I think this is cleaner than what I had proposed.


Cheers,
-- 
Vinicius

[Intel-wired-lan] [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Vinicius Costa Gomes]

Corinna Vinschen <vinschen@redhat.com> writes:

> Calling ethtool changing the RX ring parameters like this:
>
>   $ ethtool -G eth0 rx 1024
>
> on igc triggers the "Missing unregister, handled but fix driver" warning in
> xdp_rxq_info_reg().
>
> igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
> reset the xdp_rxq_info member before calling igc_setup_rx_resources().
> This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.
>
> Make sure to unregister the xdp_rxq_info structure first in
> igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
> action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.
>
> Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 2f17f36e94fd..97144f6db36e 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -505,14 +505,6 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
>  	u8 index = rx_ring->queue_index;
>  	int size, desc_len, res;
>  
> -	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> -			       rx_ring->q_vector->napi.napi_id);
> -	if (res < 0) {
> -		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> -			   index);
> -		return res;
> -	}
> -
>  	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
>  	rx_ring->rx_buffer_info = vzalloc(size);
>  	if (!rx_ring->rx_buffer_info)
> @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
>  	rx_ring->next_to_clean = 0;
>  	rx_ring->next_to_use = 0;
>  
> +	/* XDP RX-queue info */
> +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> +			       rx_ring->q_vector->napi.napi_id);
> +	if (res < 0) {
> +		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> +			   index);
> +		return res;

Here and in the igb patch, it should be 'goto err', no?

Another suggestion is to add the warning that Lennert reported in the
commit message (the comment from Maciej in that other thread).

Apart from that, I think this is cleaner than what I had proposed.


Cheers,
-- 
Vinicius

Re: [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Corinna Vinschen]

On Jan 14 11:10, Vinicius Costa Gomes wrote:
> Corinna Vinschen <vinschen@redhat.com> writes:
> 
> > Calling ethtool changing the RX ring parameters like this:
> >
> >   $ ethtool -G eth0 rx 1024
> >
> > on igc triggers the "Missing unregister, handled but fix driver" warning in
> > xdp_rxq_info_reg().
> >
> > igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
> > reset the xdp_rxq_info member before calling igc_setup_rx_resources().
> > This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.
> >
> > Make sure to unregister the xdp_rxq_info structure first in
> > igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
> > action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.
> >
> > Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
> > Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > [...]
> > @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> >  	rx_ring->next_to_clean = 0;
> >  	rx_ring->next_to_use = 0;
> >  
> > +	/* XDP RX-queue info */
> > +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> > +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> > +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> > +			       rx_ring->q_vector->napi.napi_id);
> > +	if (res < 0) {
> > +		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> > +			   index);
> > +		return res;
> 
> Here and in the igb patch, it should be 'goto err', no?

D'oh, of course.  Soory and thanks for catching.  I'll prepare a v4.

> Another suggestion is to add the warning that Lennert reported in the
> commit message (the comment from Maciej in that other thread).

The current commit message already mentiones the "Missing unregister,
handled but fix driver" warning.  Do you mean the entire warning
snippet including call stack?  If so, no problem.  I'll add it to v4,
too.

Shall I also add "Reported-by: Lennert ..."?  Funny enough we
encountered the problem independently at almost the same time, so when I
sent my v1 of the patch I wasn't even aware of the thread started by
Lennert and only saw it afterwards :}

> Apart from that, I think this is cleaner than what I had proposed.

Thanks,
Corinna

[Intel-wired-lan] [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Corinna Vinschen]

On Jan 14 11:10, Vinicius Costa Gomes wrote:
> Corinna Vinschen <vinschen@redhat.com> writes:
> 
> > Calling ethtool changing the RX ring parameters like this:
> >
> >   $ ethtool -G eth0 rx 1024
> >
> > on igc triggers the "Missing unregister, handled but fix driver" warning in
> > xdp_rxq_info_reg().
> >
> > igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
> > reset the xdp_rxq_info member before calling igc_setup_rx_resources().
> > This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.
> >
> > Make sure to unregister the xdp_rxq_info structure first in
> > igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
> > action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.
> >
> > Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
> > Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > [...]
> > @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> >  	rx_ring->next_to_clean = 0;
> >  	rx_ring->next_to_use = 0;
> >  
> > +	/* XDP RX-queue info */
> > +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> > +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> > +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> > +			       rx_ring->q_vector->napi.napi_id);
> > +	if (res < 0) {
> > +		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> > +			   index);
> > +		return res;
> 
> Here and in the igb patch, it should be 'goto err', no?

D'oh, of course.  Soory and thanks for catching.  I'll prepare a v4.

> Another suggestion is to add the warning that Lennert reported in the
> commit message (the comment from Maciej in that other thread).

The current commit message already mentiones the "Missing unregister,
handled but fix driver" warning.  Do you mean the entire warning
snippet including call stack?  If so, no problem.  I'll add it to v4,
too.

Shall I also add "Reported-by: Lennert ..."?  Funny enough we
encountered the problem independently at almost the same time, so when I
sent my v1 of the patch I wasn't even aware of the thread started by
Lennert and only saw it afterwards :}

> Apart from that, I think this is cleaner than what I had proposed.

Thanks,
Corinna

Re: [Intel-wired-lan] [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Corinna Vinschen]

On Jan 14 20:25, Corinna Vinschen wrote:
> On Jan 14 11:10, Vinicius Costa Gomes wrote:
> > Corinna Vinschen <vinschen@redhat.com> writes:
> > 
> > > Calling ethtool changing the RX ring parameters like this:
> > >
> > >   $ ethtool -G eth0 rx 1024
> > >
> > > on igc triggers the "Missing unregister, handled but fix driver" warning in
> > > xdp_rxq_info_reg().
> > >
> > > igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
> > > reset the xdp_rxq_info member before calling igc_setup_rx_resources().
> > > This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.
> > >
> > > Make sure to unregister the xdp_rxq_info structure first in
> > > igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
> > > action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.
> > >
> > > Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
> > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> > > ---
> > >  drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
> > >  1 file changed, 11 insertions(+), 9 deletions(-)
> > > [...]
> > > @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> > >  	rx_ring->next_to_clean = 0;
> > >  	rx_ring->next_to_use = 0;
> > >  
> > > +	/* XDP RX-queue info */
> > > +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> > > +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> > > +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> > > +			       rx_ring->q_vector->napi.napi_id);
> > > +	if (res < 0) {
> > > +		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> > > +			   index);
> > > +		return res;
> > 
> > Here and in the igb patch, it should be 'goto err', no?
> 
> D'oh, of course.  Soory and thanks for catching.  I'll prepare a v4.
> 
> > Another suggestion is to add the warning that Lennert reported in the
> > commit message (the comment from Maciej in that other thread).
> 
> The current commit message already mentiones the "Missing unregister,
> handled but fix driver" warning.  Do you mean the entire warning
> snippet including call stack?  If so, no problem.  I'll add it to v4,
> too.
> 
> Shall I also add "Reported-by: Lennert ..."?  Funny enough we
> encountered the problem independently at almost the same time, so when I
> sent my v1 of the patch I wasn't even aware of the thread started by
> Lennert and only saw it afterwards :}

Never mind, I just send a v4 with all of that.


Corinna

[Intel-wired-lan] [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

[Corinna Vinschen]

On Jan 14 20:25, Corinna Vinschen wrote:
> On Jan 14 11:10, Vinicius Costa Gomes wrote:
> > Corinna Vinschen <vinschen@redhat.com> writes:
> > 
> > > Calling ethtool changing the RX ring parameters like this:
> > >
> > >   $ ethtool -G eth0 rx 1024
> > >
> > > on igc triggers the "Missing unregister, handled but fix driver" warning in
> > > xdp_rxq_info_reg().
> > >
> > > igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
> > > reset the xdp_rxq_info member before calling igc_setup_rx_resources().
> > > This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.
> > >
> > > Make sure to unregister the xdp_rxq_info structure first in
> > > igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
> > > action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.
> > >
> > > Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
> > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> > > ---
> > >  drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
> > >  1 file changed, 11 insertions(+), 9 deletions(-)
> > > [...]
> > > @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> > >  	rx_ring->next_to_clean = 0;
> > >  	rx_ring->next_to_use = 0;
> > >  
> > > +	/* XDP RX-queue info */
> > > +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> > > +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> > > +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> > > +			       rx_ring->q_vector->napi.napi_id);
> > > +	if (res < 0) {
> > > +		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> > > +			   index);
> > > +		return res;
> > 
> > Here and in the igb patch, it should be 'goto err', no?
> 
> D'oh, of course.  Soory and thanks for catching.  I'll prepare a v4.
> 
> > Another suggestion is to add the warning that Lennert reported in the
> > commit message (the comment from Maciej in that other thread).
> 
> The current commit message already mentiones the "Missing unregister,
> handled but fix driver" warning.  Do you mean the entire warning
> snippet including call stack?  If so, no problem.  I'll add it to v4,
> too.
> 
> Shall I also add "Reported-by: Lennert ..."?  Funny enough we
> encountered the problem independently at almost the same time, so when I
> sent my v1 of the patch I wasn't even aware of the thread started by
> Lennert and only saw it afterwards :}

Never mind, I just send a v4 with all of that.


Corinna