RDKB-66077: Ensure the client is disconnected by ethactive flag - #70
Open
rajkamal-cv wants to merge 6 commits into
Open
RDKB-66077: Ensure the client is disconnected by ethactive flag#70rajkamal-cv wants to merge 6 commits into
rajkamal-cv wants to merge 6 commits into
Conversation
1. Use the ethactive flag, to confirm the client is disconnected. 2. Skip FAILED/INCOMPLETE ARP states alone to get valid connectivity of the clients 3. Have a single debounce to check the client is really disconnected.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Ethernet associated-device monitoring to reduce false disconnect events by incorporating the HAL eth_Active flag, broadening neighbor-state validation beyond REACHABLE, and adding a debounce before treating “0 associated devices” as a true all-clients-disconnected event.
Changes:
- Debounce “0 associated devices” events via
ETH_ZERO_COUNT_THRESHOLDconsecutive polls before callingDeleteAllHosts. - Update
ValidateClient()ARP/neigh filtering to consider most neighbor states as valid (excluding onlyFAILED/INCOMPLETE). - Gate
ValidateClient()usage on the HALeth_Activeflag (only validate wheneth_Active == 0and not in extender mode).
Comments suppressed due to low confidence (1)
source/TR-181/board_sbapi/eth_hal_interface.c:409
- The zero-count debounce counter isn't reset when CcspHalExtSw_getAssociatedDevice() fails. If a zero-count poll occurs, then a HAL failure occurs, and then another zero-count poll occurs, the code will treat those as consecutive and may call DeleteAllHosts earlier than intended. Reset uiZeroCountPolls on HAL query failure so the debounce truly reflects consecutive successful polls.
CcspTraceDebug(("<EthMonThrd> Iteration Start\n") );
//Get Associated Device Details from HAL. Do nothing if failure case
if(-1 == CcspHalExtSw_getAssociatedDevice( &ulTotalEthDeviceCount, &pstRecvEthDevice ))
{
CcspTraceInfo(("%s %d - Fail to get AssociatedDevice details\n" ,__FUNCTION__,__LINE__ ) );
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1. Add a per-host consecutive-miss counter (ETH_HOST_MISS_THRESHOLD). 2. In the Host(-) loop, defer DeleteHost until a client is missing from the HAL associated-device list for N consecutive polls (covers count 2->1 switch-FDB aging, not caught by the count==0 debounce). 3. Store the counter inline in the hash node (eth_node_t wrapper); no ABI change to eth_device_t and no separate bookkeeping table.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
source/TR-181/board_sbapi/eth_hal_interface.c:250
- CcspHalExtSw_AddHost() now allocates sizeof(eth_node_t) but still memcpy_s()'s into the raw pointer with a dest size of sizeof(eth_device_t). This works by relying on eth_device_t being the first member, but it's type-unsafe and easy to break later. Copy explicitly into the dev member of eth_node_t, then alias the node as an eth_device_t* for the hash table.
pstEthLocalHost = malloc(sizeof(eth_node_t));
if (pstEthLocalHost == NULL)
{
CcspTraceInfo(("%s %d - pstEthLocalHost Null\n" ,__FUNCTION__,__LINE__ ));
return -1;
Comment on lines
411
to
415
| static BOOL isDeleteAllDone = FALSE; | ||
| static INT uiZeroCountPolls = 0; | ||
|
|
||
|
|
||
| CcspTraceDebug(("<EthMonThrd> Iteration Start\n") ); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
source/TR-181/board_sbapi/eth_hal_interface.c:413
uiZeroCountPollsis meant to debounce consecutive zero-device polls, but it is not reset whenCcspHalExtSw_getAssociatedDevice()fails. A transient HAL failure between polls will preserve the previous counter value and can causeDeleteAllHoststo trigger after fewer thanETH_ZERO_COUNT_THRESHOLDactual zero-count polls.
static BOOL isDeleteAllDone = FALSE;
static INT uiZeroCountPolls = 0;
1. Gate the Host(-) disconnect on ValidateClient(): a host missing from the HAL associated-device list is retained if ip-neigh (not FAILED/INCOMPLETE) or a dnsmasq lease still shows it present. This covers sustained switch-FDB aging of an idle-but-connected client, which the miss-count debounce alone could not (HAL dropped the client for >threshold consecutive polls while it was still REACHABLE/leased). 2. Disconnect only when the client is absent from the HAL list AND unvalidated, for ETH_HOST_MISS_THRESHOLD consecutive polls (debounce kept as secondary guard). 3. Remove the count==0 special case (ETH_ZERO_COUNT_THRESHOLD/DeleteAllHosts): an empty HAL list now flows through the same per-client validate+debounce path, so a transient count==0 no longer bulk-deletes present clients.
Comment on lines
+78
to
+80
| /* Accept any known neighbour state; reject only FAILED/INCOMPLETE, since an | ||
| * idle client decays REACHABLE->STALE within ~30s but the poll is 180s. */ | ||
| v_secure_system("ip nei show | grep -v brlan1 | grep -i %s | grep -iEv 'FAILED|INCOMPLETE' > " ARP_CACHE, mac); |
Comment on lines
+491
to
+495
| /* Non-extender only. Trust the HAL: an eth_Active==TRUE device is on | ||
| * the switch FDB, so don't disconnect it on the flaky ip-neigh/lease | ||
| * heuristic (idle clients often show STALE/absent). Fall back to | ||
| * ValidateClient() only when the HAL says inactive; real departures | ||
| * still hit the Host(-) loop / count==0 path. */ |
Comment on lines
+446
to
+449
| /* No count==0 special case: when the HAL list is empty the Host(+) loop | ||
| * simply runs zero times and the Host(-) loop below reconciles every | ||
| * known host through the same per-client ValidateClient + miss debounce, | ||
| * so a transient count==0 no longer bulk-deletes still-present clients. */ |
Comment on lines
+549
to
+553
| /* Host missing from this poll's HAL list. The FDB-offload list is | ||
| * unreliable for idle clients (can stay dropped for several polls), | ||
| * so confirm with an independent signal before disconnecting: | ||
| * ValidateClient() (ip neigh not FAILED/INCOMPLETE, or dnsmasq lease). | ||
| * Only when that also says gone do we debounce, then DeleteHost. */ |
1. Host(+) loop: only fall back to ValidateClient() when the HAL reports the device eth_Active == FALSE. A HAL-active client (present in the switch FDB) is no longer disconnected by the flaky ip-neigh/lease heuristic. 2. Host(-) loop: add a per-host consecutive-miss debounce. A client that drops out of the HAL associated-device list is disconnected only after ETH_HOST_MISS_THRESHOLD consecutive missed polls, covering transient switch-FDB aging that drops a single idle client (e.g. count 2->1). 3. Store the miss counter inline in the hash node (eth_node_t wrapper, with eth_device_t as the first member); no change to the HAL eth_device_t struct and no separate bookkeeping table.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
source/TR-181/board_sbapi/eth_hal_interface.c:447
- The PR description says DeleteAllHosts should be debounced (require 2 consecutive polls where ulTotalEthDeviceCount==0). The current code still deletes all hosts on the first zero-count poll, which can reintroduce false disconnect storms when the HAL transiently returns an empty list.
//if 0 then delete all nodes and send disconnected notification to Ethernet
if( 0 == ulTotalEthDeviceCount )
{
source/TR-181/board_sbapi/eth_hal_interface.c:251
- CcspHalExtSw_AddHost() now allocates an eth_node_t, but memcpy_s still uses
sizeof(eth_device_t)as the destination size. While this happens to work today, using the actual allocated size avoids accidental future truncation or memcpy_s failures if the copy size changes.
pstEthLocalHost = malloc(sizeof(eth_node_t));
if (pstEthLocalHost == NULL)
{
CcspTraceInfo(("%s %d - pstEthLocalHost Null\n" ,__FUNCTION__,__LINE__ ));
return -1;
}
//Copy received host details
rc = memcpy_s(pstEthLocalHost, sizeof(eth_device_t), pstEthHost, sizeof(eth_device_t));
if(rc != EOK)
Comment on lines
+507
to
+511
| /* Non-extender only. Trust the HAL: an eth_Active==TRUE device is on | ||
| * the switch FDB, so don't disconnect it on the flaky ip-neigh/lease | ||
| * heuristic (idle clients often show STALE/absent). Fall back to | ||
| * ValidateClient() only when the HAL says inactive; real departures | ||
| * still hit the Host(-) loop / count==0 path. */ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RDKB-66077 : Prevent false ethernet-client disconnects
Reason for change:
Post 8.6p1s2, ethernet clients flap Disconnected/Connected (SYS_INFO_ETHClientDisConn
spike) while still connected - link up, ping passes. The monitor thread overrides the
HAL eth_Active flag with an ip-neigh/lease check that fails for idle clients (ARP
STALE/FAILED), and it deletes all hosts on a single transient count==0 poll.
Fix:
Real departures still detected via Host(-) loop, debounced count==0, and link-down FDB flush.
Test Procedure:
Risks: Low
Priority: P1