> -----Original Message----- > From: Zhang, AlvinX <alvinx.zh...@intel.com> > Sent: Monday, April 19, 2021 15:15 > To: Wang, Haiyue <haiyue.w...@intel.com>; Guo, Jia <jia....@intel.com> > Cc: dev@dpdk.org; sta...@dpdk.org > Subject: RE: [PATCH] net/igc: fix Rx packet size error > > Hi Haiyue, > > Thanks for your review. > > > -----Original Message----- > > From: Wang, Haiyue <haiyue.w...@intel.com> > > Sent: Friday, April 16, 2021 9:57 AM > > To: Zhang, AlvinX <alvinx.zh...@intel.com>; Guo, Jia <jia....@intel.com> > > Cc: dev@dpdk.org; sta...@dpdk.org > > Subject: RE: [PATCH] net/igc: fix Rx packet size error > > > > > -----Original Message----- > > > From: Zhang, AlvinX <alvinx.zh...@intel.com> > > > Sent: Friday, April 16, 2021 09:14 > > > To: Wang, Haiyue <haiyue.w...@intel.com>; Guo, Jia <jia....@intel.com> > > > Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zh...@intel.com>; > > > sta...@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: sta...@dpdk.org > > > > > > Signed-off-by: Alvin Zhang <alvinx.zh...@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