diff mbox series

[2/3,net-next,v5] igb: refactor XDP registration

Message ID 20220117182915.1283151-3-vinschen@redhat.com
State Superseded
Headers show
Series igb/igc: fix XDP registration | expand

Commit Message

Corinna Vinschen Jan. 17, 2022, 6:29 p.m. UTC
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    | 12 +++++++++---
 2 files changed, 9 insertions(+), 7 deletions(-)

Comments

Alexander Lobakin Jan. 18, 2022, 3:05 p.m. UTC | #1
From: Corinna Vinschen <vinschen@redhat.com>
Date: Mon, 17 Jan 2022 19:29:14 +0100

> 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    | 12 +++++++++---
>  2 files changed, 9 insertions(+), 7 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..cea89d301bfd 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)
> +	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);
>  		goto err;

Error path always returns -ENOMEM, even in this case, and reports
that it failed to allocate memory for rings. Handle this correctly
and return `res` instead and without one more error message?

> +	}

As I mentioned a bit above, `res` is unused here as an error code,
only to test the value on < 0. Does it make sense to add a new
variable?

>  
>  	return 0;
>  
> -- 
> 2.27.0

Thanks,
Al
Corinna Vinschen Jan. 19, 2022, 6:08 a.m. UTC | #2
On Jan 18 16:05, Alexander Lobakin wrote:
> From: Corinna Vinschen <vinschen@redhat.com>
> Date: Mon, 17 Jan 2022 19:29:14 +0100
> 
> > 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    | 12 +++++++++---
> >  2 files changed, 9 insertions(+), 7 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..cea89d301bfd 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)
> > +	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);
> >  		goto err;
> 
> Error path always returns -ENOMEM, even in this case, and reports
> that it failed to allocate memory for rings. Handle this correctly
> and return `res` instead and without one more error message?

In that case, it makes sense to revert the code to the way igc did it,
rather then trying to do as igb did it.

I. e., for both drivers, call xdp_rxq_info_is_reg before the first
allocation took place, and just return immediately from there if it
fails.  Everything else complicates the code unnecessarily.

> As I mentioned a bit above, `res` is unused here as an error code,
> only to test the value on < 0. Does it make sense to add a new
> variable?

Following my above sugggestion, res will be used as error code, so
it should stay.

I'll provide a matching patchset later today.


Thanks,
Corinna
diff mbox series

Patch

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..cea89d301bfd 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)
+	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);
 		goto err;
+	}
 
 	return 0;