diff mbox series

net/igc: fix Rx packet size error

Message ID 20210416011407.29236-1-alvinx.zhang@intel.com (mailing list archive)
State Superseded
Delegated to: Qi Zhang
Headers show
Series net/igc: fix Rx packet size error | expand

Checks

Context Check Description
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot success github build: passed
ci/travis-robot success travis build: passed
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Alvin Zhang April 16, 2021, 1:14 a.m. UTC
When DEV_RX_OFFLOAD_KEEP_CRC is enabled, the PMD will minus 4 bytes
of CRC from the size of a packet, but the NIC will strip the CRC
because the CRC strip bit in DVMOLR register is not cleared. This
will cause the size of a packet to be 4 bytes less.

This patch updates the CRC strip bit according to whether
DEV_RX_OFFLOAD_KEEP_CRC is enabled.

Fixes: a5aeb2b9e225 ("net/igc: support Rx and Tx")
Cc: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---
 drivers/net/igc/igc_txrx.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Wang, Haiyue April 16, 2021, 1:57 a.m. UTC | #1
> -----Original Message-----
> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> Sent: Friday, April 16, 2021 09:14
> To: Wang, Haiyue <haiyue.wang@intel.com>; Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/igc: fix Rx packet size error
> 
> When DEV_RX_OFFLOAD_KEEP_CRC is enabled, the PMD will minus 4 bytes
> of CRC from the size of a packet, but the NIC will strip the CRC
> because the CRC strip bit in DVMOLR register is not cleared. This
> will cause the size of a packet to be 4 bytes less.
> 
> This patch updates the CRC strip bit according to whether
> DEV_RX_OFFLOAD_KEEP_CRC is enabled.
> 
> Fixes: a5aeb2b9e225 ("net/igc: support Rx and Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
>  drivers/net/igc/igc_txrx.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
> index c0a5d5e..68b102d 100644
> --- a/drivers/net/igc/igc_txrx.c
> +++ b/drivers/net/igc/igc_txrx.c
> @@ -1290,20 +1290,24 @@ int eth_igc_rx_descriptor_status(void *rx_queue, uint16_t offset)
>  	 * This needs to be done after enable.
>  	 */
>  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		uint32_t dvmolr;
> +
>  		rxq = dev->data->rx_queues[i];
>  		IGC_WRITE_REG(hw, IGC_RDH(rxq->reg_idx), 0);
> -		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx),
> -				rxq->nb_rx_desc - 1);
> +		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx), rxq->nb_rx_desc - 1);
> +
> +		dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
> 
>  		/* strip queue vlan offload */
> -		if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
> -			uint32_t dvmolr;
> -			dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
> +		dvmolr = (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) ?
> +			 (dvmolr | IGC_DVMOLR_STRVLAN) :
> +			 (dvmolr & ~IGC_DVMOLR_STRVLAN);

Just use "if ... else .."to make code readable:

		if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
			dvmolr |= IGC_DVMOLR_STRVLAN;
		else
			dvmolr &= ~IGC_DVMOLR_STRVLAN;


> 
> -			/* If vlan been stripped off, the CRC is meaningless. */

Looks like we need to handle CRC & VLAN_STRIP co-exist issue:
	If user enables VLAN strip, then keep CRC should be rejected,
	and vice versa.

> -			dvmolr |= IGC_DVMOLR_STRVLAN | IGC_DVMOLR_STRCRC;
> -			IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
> -		}
> +		dvmolr = (offloads & DEV_RX_OFFLOAD_KEEP_CRC) ?
> +			 (dvmolr & ~IGC_DVMOLR_STRCRC) :
> +			 (dvmolr | IGC_DVMOLR_STRCRC);

Just use "if ... else .."to make code readable:

		if (offloads & DEV_RX_OFFLOAD_KEEP_CRC)
			dvmolr &= ~IGC_DVMOLR_STRCRC;
		else
			dvmolr |= IGC_DVMOLR_STRCRC;

> +
> +		IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
>  	}
> 
>  	return 0;
> --
> 1.8.3.1
Alvin Zhang April 19, 2021, 7:14 a.m. UTC | #2
Hi Haiyue,

Thanks for your review.

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Friday, April 16, 2021 9:57 AM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH] net/igc: fix Rx packet size error
> 
> > -----Original Message-----
> > From: Zhang, AlvinX <alvinx.zhang@intel.com>
> > Sent: Friday, April 16, 2021 09:14
> > To: Wang, Haiyue <haiyue.wang@intel.com>; Guo, Jia <jia.guo@intel.com>
> > Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>;
> > stable@dpdk.org
> > Subject: [PATCH] net/igc: fix Rx packet size error
> >
> > When DEV_RX_OFFLOAD_KEEP_CRC is enabled, the PMD will minus 4 bytes of
> > CRC from the size of a packet, but the NIC will strip the CRC because
> > the CRC strip bit in DVMOLR register is not cleared. This will cause
> > the size of a packet to be 4 bytes less.
> >
> > This patch updates the CRC strip bit according to whether
> > DEV_RX_OFFLOAD_KEEP_CRC is enabled.
> >
> > Fixes: a5aeb2b9e225 ("net/igc: support Rx and Tx")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> >  drivers/net/igc/igc_txrx.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
> > index c0a5d5e..68b102d 100644
> > --- a/drivers/net/igc/igc_txrx.c
> > +++ b/drivers/net/igc/igc_txrx.c
> > @@ -1290,20 +1290,24 @@ int eth_igc_rx_descriptor_status(void *rx_queue,
> uint16_t offset)
> >  	 * This needs to be done after enable.
> >  	 */
> >  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +		uint32_t dvmolr;
> > +
> >  		rxq = dev->data->rx_queues[i];
> >  		IGC_WRITE_REG(hw, IGC_RDH(rxq->reg_idx), 0);
> > -		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx),
> > -				rxq->nb_rx_desc - 1);
> > +		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx), rxq->nb_rx_desc - 1);
> > +
> > +		dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
> >
> >  		/* strip queue vlan offload */
> > -		if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
> > -			uint32_t dvmolr;
> > -			dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
> > +		dvmolr = (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) ?
> > +			 (dvmolr | IGC_DVMOLR_STRVLAN) :
> > +			 (dvmolr & ~IGC_DVMOLR_STRVLAN);
> 
> Just use "if ... else .."to make code readable:
> 
> 		if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
> 			dvmolr |= IGC_DVMOLR_STRVLAN;
> 		else
> 			dvmolr &= ~IGC_DVMOLR_STRVLAN;
> 
> 
> >
> > -			/* If vlan been stripped off, the CRC is meaningless. */
> 
> Looks like we need to handle CRC & VLAN_STRIP co-exist issue:
> 	If user enables VLAN strip, then keep CRC should be rejected,
> 	and vice versa.

By default, the DEV_RX_OFFLOAD_KEEP_CRC does not been set,
so if the user set the DEV_RX_OFFLOAD_KEEP_CRC and DEV_RX_OFFLOAD_VLAN_STRIP bits,
this means the user really need CRC and VLAN stripped off, although the CRC is meaningless at this situation.
So can we issue some warnings instead of changing the user's settings?

> 
> > -			dvmolr |= IGC_DVMOLR_STRVLAN | IGC_DVMOLR_STRCRC;
> > -			IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
> > -		}
> > +		dvmolr = (offloads & DEV_RX_OFFLOAD_KEEP_CRC) ?
> > +			 (dvmolr & ~IGC_DVMOLR_STRCRC) :
> > +			 (dvmolr | IGC_DVMOLR_STRCRC);
> 
> Just use "if ... else .."to make code readable:
> 
> 		if (offloads & DEV_RX_OFFLOAD_KEEP_CRC)
> 			dvmolr &= ~IGC_DVMOLR_STRCRC;
> 		else
> 			dvmolr |= IGC_DVMOLR_STRCRC;
> 
> > +
> > +		IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
> >  	}
> >
> >  	return 0;
> > --
> > 1.8.3.1
Wang, Haiyue April 19, 2021, 7:43 a.m. UTC | #3
> -----Original Message-----
> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> Sent: Monday, April 19, 2021 15:15
> To: Wang, Haiyue <haiyue.wang@intel.com>; Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH] net/igc: fix Rx packet size error
> 
> Hi Haiyue,
> 
> Thanks for your review.
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Friday, April 16, 2021 9:57 AM
> > To: Zhang, AlvinX <alvinx.zhang@intel.com>; Guo, Jia <jia.guo@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: RE: [PATCH] net/igc: fix Rx packet size error
> >
> > > -----Original Message-----
> > > From: Zhang, AlvinX <alvinx.zhang@intel.com>
> > > Sent: Friday, April 16, 2021 09:14
> > > To: Wang, Haiyue <haiyue.wang@intel.com>; Guo, Jia <jia.guo@intel.com>
> > > Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>;
> > > stable@dpdk.org
> > > Subject: [PATCH] net/igc: fix Rx packet size error
> > >
> > > When DEV_RX_OFFLOAD_KEEP_CRC is enabled, the PMD will minus 4 bytes of
> > > CRC from the size of a packet, but the NIC will strip the CRC because
> > > the CRC strip bit in DVMOLR register is not cleared. This will cause
> > > the size of a packet to be 4 bytes less.
> > >
> > > This patch updates the CRC strip bit according to whether
> > > DEV_RX_OFFLOAD_KEEP_CRC is enabled.
> > >
> > > Fixes: a5aeb2b9e225 ("net/igc: support Rx and Tx")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > ---
> > >  drivers/net/igc/igc_txrx.c | 22 +++++++++++++---------
> > >  1 file changed, 13 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
> > > index c0a5d5e..68b102d 100644
> > > --- a/drivers/net/igc/igc_txrx.c
> > > +++ b/drivers/net/igc/igc_txrx.c
> > > @@ -1290,20 +1290,24 @@ int eth_igc_rx_descriptor_status(void *rx_queue,
> > uint16_t offset)
> > >  	 * This needs to be done after enable.
> > >  	 */
> > >  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > +		uint32_t dvmolr;
> > > +
> > >  		rxq = dev->data->rx_queues[i];
> > >  		IGC_WRITE_REG(hw, IGC_RDH(rxq->reg_idx), 0);
> > > -		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx),
> > > -				rxq->nb_rx_desc - 1);
> > > +		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx), rxq->nb_rx_desc - 1);
> > > +
> > > +		dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
> > >
> > >  		/* strip queue vlan offload */
> > > -		if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
> > > -			uint32_t dvmolr;
> > > -			dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
> > > +		dvmolr = (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) ?
> > > +			 (dvmolr | IGC_DVMOLR_STRVLAN) :
> > > +			 (dvmolr & ~IGC_DVMOLR_STRVLAN);
> >
> > Just use "if ... else .."to make code readable:
> >
> > 		if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
> > 			dvmolr |= IGC_DVMOLR_STRVLAN;
> > 		else
> > 			dvmolr &= ~IGC_DVMOLR_STRVLAN;
> >
> >
> > >
> > > -			/* If vlan been stripped off, the CRC is meaningless. */
> >
> > Looks like we need to handle CRC & VLAN_STRIP co-exist issue:
> > 	If user enables VLAN strip, then keep CRC should be rejected,
> > 	and vice versa.
> 
> By default, the DEV_RX_OFFLOAD_KEEP_CRC does not been set,
> so if the user set the DEV_RX_OFFLOAD_KEEP_CRC and DEV_RX_OFFLOAD_VLAN_STRIP bits,
> this means the user really need CRC and VLAN stripped off, although the CRC is meaningless at this
> situation.
> So can we issue some warnings instead of changing the user's settings?

I re-think it again, no need to handle this now, until user complains about something.

Let's just fix the code style issue. ;-)

> 
> >
> > > -			dvmolr |= IGC_DVMOLR_STRVLAN | IGC_DVMOLR_STRCRC;
> > > -			IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
> > > -		}
> > > +		dvmolr = (offloads & DEV_RX_OFFLOAD_KEEP_CRC) ?
> > > +			 (dvmolr & ~IGC_DVMOLR_STRCRC) :
> > > +			 (dvmolr | IGC_DVMOLR_STRCRC);
> >
> > Just use "if ... else .."to make code readable:
> >
> > 		if (offloads & DEV_RX_OFFLOAD_KEEP_CRC)
> > 			dvmolr &= ~IGC_DVMOLR_STRCRC;
> > 		else
> > 			dvmolr |= IGC_DVMOLR_STRCRC;
> >
> > > +
> > > +		IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
> > >  	}
> > >
> > >  	return 0;
> > > --
> > > 1.8.3.1
diff mbox series

Patch

diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
index c0a5d5e..68b102d 100644
--- a/drivers/net/igc/igc_txrx.c
+++ b/drivers/net/igc/igc_txrx.c
@@ -1290,20 +1290,24 @@  int eth_igc_rx_descriptor_status(void *rx_queue, uint16_t offset)
 	 * This needs to be done after enable.
 	 */
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		uint32_t dvmolr;
+
 		rxq = dev->data->rx_queues[i];
 		IGC_WRITE_REG(hw, IGC_RDH(rxq->reg_idx), 0);
-		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx),
-				rxq->nb_rx_desc - 1);
+		IGC_WRITE_REG(hw, IGC_RDT(rxq->reg_idx), rxq->nb_rx_desc - 1);
+
+		dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
 
 		/* strip queue vlan offload */
-		if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
-			uint32_t dvmolr;
-			dvmolr = IGC_READ_REG(hw, IGC_DVMOLR(rxq->queue_id));
+		dvmolr = (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) ?
+			 (dvmolr | IGC_DVMOLR_STRVLAN) :
+			 (dvmolr & ~IGC_DVMOLR_STRVLAN);
 
-			/* If vlan been stripped off, the CRC is meaningless. */
-			dvmolr |= IGC_DVMOLR_STRVLAN | IGC_DVMOLR_STRCRC;
-			IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
-		}
+		dvmolr = (offloads & DEV_RX_OFFLOAD_KEEP_CRC) ?
+			 (dvmolr & ~IGC_DVMOLR_STRCRC) :
+			 (dvmolr | IGC_DVMOLR_STRCRC);
+
+		IGC_WRITE_REG(hw, IGC_DVMOLR(rxq->reg_idx), dvmolr);
 	}
 
 	return 0;