search

intel / openlldp

Other
54 stars 42 forks source link

lldp_dcbx_nl: Fix Copy-Paste Typo #101

Open liuhangbin opened 2 months ago

liuhangbin commented 2 months ago

It should check the RX PGID when configuring RX PG per TC settings.

Fixes: da0da5e98508 ("lldpad: dcbx, map PG up2tc config onto hardware limits")

penguin359 commented 2 months ago

This looks like a legit typo that should be fixed, although how this affects the resulting priority groups is less clear to me. It seems like this should present a very obvious change in the behavior of the NIC under test if it supports hardware PGs.

@apconole do you have access to any actual DCBX-capable switches to test on? I might be able to get this scenario set up in our datacenter lab sandbox and get the software validation team to validate the bug and fix from this PR. If this is a bug, it dates back to at least 2011 which makes me question whether this is a bug that's gone unnoticed or is actually intended behavior. This will need a closer look to say one way or the other.

apconole commented 1 month ago

It would be good to get a test run in. I'm inclined to believe that this functionality wasn't covered anywhere, to be honest. I don't have access to any DCB-x switches to test, though.

penguin359 commented 1 month ago

It just seems odd that this line hasn't been touched since 2012, and was originally tx when it was added in 2011, that this bug wasn't caught when I was involved in the extensive DCBX validation and hardware-based priority groups and PFC/ETS that we did on-site here in Oregon until 2019. Now, that has moved elsewhere and I don't see it anymore, but this seems like it should be something that would have affected our testing if the code is used.