Wednesday, June 20, 2018 7:13 PM. Ferruh Yigit:
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add new offload flag to keep CRC
>
> On 6/20/2018 2:44 PM, Shahaf Shuler wrote:
> >
> > Hi Ferruh,
> >
> > Tuesday, June 19, 2018 9:03 PM, Ferruh Yigit
> >> Subject: [dpdk-dev] [PATCH] ethdev: add new offload flag to keep CRC
> >>
> >
> > [...]
> >
> >
> >> diff --git a/drivers/net/mlx5/mlx5_rxq.c
> >> b/drivers/net/mlx5/mlx5_rxq.c index de3f869ed..28cf168aa 100644
> >> --- a/drivers/net/mlx5/mlx5_rxq.c
> >> +++ b/drivers/net/mlx5/mlx5_rxq.c
> >> @@ -388,6 +388,9 @@ mlx5_get_rx_queue_offloads(struct rte_eth_dev
> >> *dev)
> >>
> >> if (config->hw_fcs_strip)
> >> offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> >> + else
> >> + offloads |= DEV_RX_OFFLOAD_KEEP_CRC;
> >> +
> >
> > I think it should be:
> > if (config->hw_fcs_strip) {
> > offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> > offloads |= DEV_RX_OFFLOAD_KEEP_CRC;
> > }
> >
> > The hw_fcs_strip is the capability from device which allows the PMD to
> toggle the CRC stripping.
>
> From below logic, (how "crc_present" set), hw_fcs_strip looks like capability
> from device that says keeping CRC is supported. If so it the original code was
> not clear to me, why to report CRC stripping only if HW supports keeping
> CRC?
What we report is the option to toggle the CRC stripping by the PMD, and this
is what the hw_fcs_strip capability is about.
The default behavior of HW in case this option is not supported is to remove
the CRC.
I>
> Following makes more sense to me, based on below code, report CRC
> stripping capability by default and report KEEP CRC capability if device
> supports it:
>
> offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> if (config->hw_fcs_strip)
> offloads |= DEV_RX_OFFLOAD_KEEP_CRC;
>
> What do you think?
Yes it is better. Thanks.
>
> >
> >> if (config->hw_csum)
> >> offloads |= (DEV_RX_OFFLOAD_IPV4_CKSUM |
> >> DEV_RX_OFFLOAD_UDP_CKSUM |
> >> @@ -1419,17 +1422,17 @@ mlx5_rxq_new(struct rte_eth_dev *dev,
> >> uint16_t idx, uint16_t desc,
> >> /* Configure VLAN stripping. */
> >> tmpl->rxq.vlan_strip = !!(offloads &
> DEV_RX_OFFLOAD_VLAN_STRIP);
> >> /* By default, FCS (CRC) is stripped by hardware. */
> >> - if (offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
> >> - tmpl->rxq.crc_present = 0;
> >> - } else if (config->hw_fcs_strip) {
> >> - tmpl->rxq.crc_present = 1;
> >> - } else {
> >> - DRV_LOG(WARNING,
> >> - "port %u CRC stripping has been disabled but will"
> >> - " still be performed by hardware, make sure
> >> MLNX_OFED"
> >> - " and firmware are up to date",
> >> - dev->data->port_id);
> >> - tmpl->rxq.crc_present = 0;
> >> + tmpl->rxq.crc_present = 0;
> >> + if (rte_eth_dev_is_keep_crc(offloads)) {
> >> + if (config->hw_fcs_strip) {
> >> + tmpl->rxq.crc_present = 1;
> >> + } else {
> >> + DRV_LOG(WARNING,
> >> + "port %u CRC stripping has been disabled but
> >> will"
> >> + " still be performed by hardware, make sure
> >> MLNX_OFED"
> >> + " and firmware are up to date",
> >> + dev->data->port_id);
> >> + }
> >> }
> >> DRV_LOG(DEBUG,
> >> "port %u CRC stripping is %s, %u bytes will be subtracted
> from"
