diff mbox

[dpdk-dev,v4,2/5] ixgbe: Bug fix: Properly configure Rx CRC stripping for x540 devices

Message ID 1425823498-30385-3-git-send-email-vladz@cloudius-systems.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Vladislav Zolotarov March 8, 2015, 2:04 p.m. UTC
According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
be configured to the same value as HLREG0.RXCRCSTRP.

Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
but seems harmless.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Thomas Monjalon March 8, 2015, 9:12 p.m. UTC | #1
Hi Vlad,

2015-03-08 16:04, Vlad Zolotarov:
> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
> be configured to the same value as HLREG0.RXCRCSTRP.
> 
> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
> but seems harmless.
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>

You are mixing a fix (this patch) and enhancements (LRO) in the same series.
Could you separate them please, as LRO is not going into 2.0 but this fix
is a good candidate.

Thanks
Vladislav Zolotarov March 9, 2015, 7:08 a.m. UTC | #2
On 03/08/15 23:12, Thomas Monjalon wrote:
> Hi Vlad,
>
> 2015-03-08 16:04, Vlad Zolotarov:
>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
>> be configured to the same value as HLREG0.RXCRCSTRP.
>>
>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
>> but seems harmless.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> You are mixing a fix (this patch) and enhancements (LRO) in the same series.
> Could you separate them please, as LRO is not going into 2.0 but this fix
> is a good candidate.

Pls, note that all patches in this series except for PATCH3 and PATCH5 
are fixing real bugs. I can send them as a separate series if u'd like.
Pls., confirm.

>
> Thanks
Vladislav Zolotarov March 9, 2015, 7:31 a.m. UTC | #3
On 03/08/15 23:12, Thomas Monjalon wrote:
> Hi Vlad,
>
> 2015-03-08 16:04, Vlad Zolotarov:
>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
>> be configured to the same value as HLREG0.RXCRCSTRP.
>>
>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
>> but seems harmless.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> You are mixing a fix (this patch) and enhancements (LRO) in the same series.

I'd also like to clarify that I've "mixed" them because the bugs were 
found in the area of the enhancement therefore I united them in the same 
series.
Frankly, I wasn't thinking about the DPDK versions it's going into when 
I sent the series... ;)

> Could you separate them please, as LRO is not going into 2.0 but this fix
> is a good candidate.
>
> Thanks
Thomas Monjalon March 9, 2015, 7:58 a.m. UTC | #4
2015-03-09 09:08, Vlad Zolotarov:
> 
> On 03/08/15 23:12, Thomas Monjalon wrote:
> > Hi Vlad,
> >
> > 2015-03-08 16:04, Vlad Zolotarov:
> >> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
> >> be configured to the same value as HLREG0.RXCRCSTRP.
> >>
> >> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
> >> but seems harmless.
> >>
> >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> > You are mixing a fix (this patch) and enhancements (LRO) in the same series.
> > Could you separate them please, as LRO is not going into 2.0 but this fix
> > is a good candidate.
> 
> Pls, note that all patches in this series except for PATCH3 and PATCH5 
> are fixing real bugs. I can send them as a separate series if u'd like.
> Pls., confirm.

Yes you're right, patch 1 is also a fix and patch 4 seems to solve other
issues. However, patch 4 makes also some refactoring and seems a bit risky.
We need an ixgbe maintainer to decide wether we can merge it before the
release. Or is it possible to have fixes of the patch 4 without the
refactoring?

Thanks Vlad.
Sorry to request such split but this PMD is sensible and I don't want to
have a risk of making it worst in release 2.0.
Vladislav Zolotarov March 9, 2015, 4:10 p.m. UTC | #5
On 03/09/15 09:58, Thomas Monjalon wrote:
> 2015-03-09 09:08, Vlad Zolotarov:
>> On 03/08/15 23:12, Thomas Monjalon wrote:
>>> Hi Vlad,
>>>
>>> 2015-03-08 16:04, Vlad Zolotarov:
>>>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
>>>> be configured to the same value as HLREG0.RXCRCSTRP.
>>>>
>>>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
>>>> but seems harmless.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> You are mixing a fix (this patch) and enhancements (LRO) in the same series.
>>> Could you separate them please, as LRO is not going into 2.0 but this fix
>>> is a good candidate.
>> Pls, note that all patches in this series except for PATCH3 and PATCH5
>> are fixing real bugs. I can send them as a separate series if u'd like.
>> Pls., confirm.
> Yes you're right, patch 1 is also a fix and patch 4 seems to solve other
> issues. However, patch 4 makes also some refactoring and seems a bit risky.
> We need an ixgbe maintainer to decide wether we can merge it before the
> release. Or is it possible to have fixes of the patch 4 without the
> refactoring?
>
> Thanks Vlad.
> Sorry to request such split but this PMD is sensible and I don't want to
> have a risk of making it worst in release 2.0.

Well, IMHO PATCH4 here has the most important and critical fix among 
other patches - the whole port is going to be configured according to 
the last queue configuration. Consider the case when queues are 
configured differently and the last one meets vector Rx requirements: 
bulk allocation requirements, descriptors number is a power of 2 (there 
are per-port requirements too). But the first queue may be configured to 
have a different number of descriptors, which is NOT a power of two. 
Currently the port's rx_pkt_burst callback will be configured to use a 
vercot Rx callback and we would hit the same issue reported by Stephen 
and "fixed" in 352078e8e.
Vladislav Zolotarov March 9, 2015, 7:28 p.m. UTC | #6
On 03/09/15 09:58, Thomas Monjalon wrote:
> 2015-03-09 09:08, Vlad Zolotarov:
>> On 03/08/15 23:12, Thomas Monjalon wrote:
>>> Hi Vlad,
>>>
>>> 2015-03-08 16:04, Vlad Zolotarov:
>>>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
>>>> be configured to the same value as HLREG0.RXCRCSTRP.
>>>>
>>>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
>>>> but seems harmless.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> You are mixing a fix (this patch) and enhancements (LRO) in the same series.
>>> Could you separate them please, as LRO is not going into 2.0 but this fix
>>> is a good candidate.
>> Pls, note that all patches in this series except for PATCH3 and PATCH5
>> are fixing real bugs. I can send them as a separate series if u'd like.
>> Pls., confirm.
> Yes you're right, patch 1 is also a fix and patch 4 seems to solve other
> issues. However, patch 4 makes also some refactoring and seems a bit risky.
> We need an ixgbe maintainer to decide wether we can merge it before the
> release. Or is it possible to have fixes of the patch 4 without the
> refactoring?

PATCH4 doesn't have any refactoring - it fixes a design bug (actually 
two). The description of the patch is separated into two sections: one 
that describes how I did it and the second (starts with "Bugs fixed:") 
describes which bugs it fixes.

When I re-read its description again now I see that I forgot to mention 
the second issue it fixes: there is the same problem for both Vector Rx 
callback initialization and for Bulk Rx callback initialization. In both 
cases in the current master tree the queue initialization code tries to 
initialize the port property (rx_pkt_bulk callback) which depends on all 
queues properties thus may be done only after all queues properties have 
been analyzed. More than that, some callbacks types may be used only 
when some combination of preconditions is met by all Rx queues: e.g. 
vector Rx may be used only when bulk (checks queue configuration) and 
vector (checks queue and port configuration) preconditions are met. 
Therefore I decided to introduce two port states that are a logical AND 
function of the appropriate queues' states. These states are used by a 
port configuration function that now can properly configure the callbacks.

So, again - no refactoring here - a pure bug fix... ;)

>
> Thanks Vlad.
> Sorry to request such split but this PMD is sensible and I don't want to
> have a risk of making it worst in release 2.0.
Vladislav Zolotarov March 9, 2015, 7:30 p.m. UTC | #7
On 03/09/15 09:58, Thomas Monjalon wrote:
> 2015-03-09 09:08, Vlad Zolotarov:
>> On 03/08/15 23:12, Thomas Monjalon wrote:
>>> Hi Vlad,
>>>
>>> 2015-03-08 16:04, Vlad Zolotarov:
>>>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
>>>> be configured to the same value as HLREG0.RXCRCSTRP.
>>>>
>>>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
>>>> but seems harmless.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>> You are mixing a fix (this patch) and enhancements (LRO) in the same series.
>>> Could you separate them please, as LRO is not going into 2.0 but this fix
>>> is a good candidate.
>> Pls, note that all patches in this series except for PATCH3 and PATCH5
>> are fixing real bugs. I can send them as a separate series if u'd like.
>> Pls., confirm.
> Yes you're right, patch 1 is also a fix and patch 4 seems to solve other
> issues. However, patch 4 makes also some refactoring and seems a bit risky.
> We need an ixgbe maintainer to decide wether we can merge it before the
> release. Or is it possible to have fixes of the patch 4 without the
> refactoring?
>
> Thanks Vlad.
> Sorry to request such split but this PMD is sensible and I don't want to
> have a risk of making it worst in release 2.0.

I've separated the series into two: "bug fixes" and "cleanups + 
refactoring + LRO feature". Both series are already on the list. ;)
diff mbox

Patch

diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
index fe362e4..e370e0a 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
@@ -3680,7 +3680,8 @@  ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 
 	IXGBE_WRITE_REG(hw, IXGBE_RXCSUM, rxcsum);
 
-	if (hw->mac.type == ixgbe_mac_82599EB) {
+	if (hw->mac.type == ixgbe_mac_82599EB ||
+	    hw->mac.type == ixgbe_mac_X540) {
 		rdrxctl = IXGBE_READ_REG(hw, IXGBE_RDRXCTL);
 		if (rx_conf->hw_strip_crc)
 			rdrxctl |= IXGBE_RDRXCTL_CRCSTRIP;