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

["Zhang, AlvinX" <alvinx.zhang@intel.com>]

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