[dpdk-dev] [PATCH] net/igc: fix Rx packet size error

Zhang, AlvinX alvinx.zhang at intel.com
Mon Apr 19 09:14:45 CEST 2021


Hi Haiyue,

Thanks for your review.

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang at intel.com>
> Sent: Friday, April 16, 2021 9:57 AM
> To: Zhang, AlvinX <alvinx.zhang at intel.com>; Guo, Jia <jia.guo at intel.com>
> Cc: dev at dpdk.org; stable at dpdk.org
> Subject: RE: [PATCH] net/igc: fix Rx packet size error
> 
> > -----Original Message-----
> > From: Zhang, AlvinX <alvinx.zhang at intel.com>
> > Sent: Friday, April 16, 2021 09:14
> > To: Wang, Haiyue <haiyue.wang at intel.com>; Guo, Jia <jia.guo at intel.com>
> > Cc: dev at dpdk.org; Zhang, AlvinX <alvinx.zhang at intel.com>;
> > stable at 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 at dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang at 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



More information about the dev mailing list