From ba320d7f08e8a0b2c5981907ab7a636bc575859c Mon Sep 17 00:00:00 2001 From: Faizan Ali Date: Thu, 27 Aug 2026 13:12:39 +0530 Subject: [PATCH 1/2] mctpd: fix EID recovery for bridged (downstream) endpoints For downstream peers (whose EID falls within a bridge's pool range), endpoint_recover must probe via right probe path 1. In case of probe failure, diagnose the bridge chain reaching to endpoint and flag point of failure. 2. In case probe is successful, compare the UUID returned by the downstream device against the stored UUID to detect device exchange. A UUID mismatch points to replacement of old device thus remove and republish new endpoint with same EID. Introduce peer->gateway to track the next-hop bridge for each downstream peer. For a single-level bridge this is set by peer_endpoint_poll() when the downstream endpoint is discovered. Signed-off-by: Faizan Ali --- src/mctpd.c | 200 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 147 insertions(+), 53 deletions(-) diff --git a/src/mctpd.c b/src/mctpd.c index e74f118..d4a235e 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -224,6 +224,9 @@ struct peer { uint8_t pool_size; uint8_t pool_start; + // Next hop toward the bus owner for downstream peer + struct peer *gateway; + struct { sd_event_source **sources; } bridge_ep_poll; @@ -2954,7 +2957,7 @@ static int query_get_peer_uuid_by_phys(struct ctx *ctx, const dest_phys *dest, return rc; } -static int query_get_peer_uuid(struct peer *peer) +static int query_get_peer_uuid(struct peer *peer, uint8_t uuid[16]) { struct mctp_ctrl_resp_get_uuid *resp = NULL; struct mctp_ctrl_cmd_get_uuid req; @@ -2985,10 +2988,7 @@ static int query_get_peer_uuid(struct peer *peer) goto out; resp = cmd.resp; - rc = peer_set_uuid(peer, resp->uuid); - if (rc < 0) - goto out; - rc = 0; + memcpy(uuid, resp->uuid, 16); out: mctp_ctrl_cmd_free(&cmd); @@ -3458,10 +3458,19 @@ static int query_peer_properties(struct peer *peer) } } - rc = query_get_peer_uuid(peer); - if (rc < 0 && peer->ctx->verbose) { - errno = -rc; - warn("Error getting UUID for %s", peer_tostr(peer)); + { + uint8_t uuid[16]; + memset(uuid, 0, sizeof(uuid)); + rc = query_get_peer_uuid(peer, uuid); + if (rc < 0) { + if (peer->ctx->verbose) { + errno = -rc; + warn("Error getting UUID for %s", + peer_tostr(peer)); + } + } else { + rc = peer_set_uuid(peer, uuid); + } } // TODO: emit property changed? Though currently they are all const. @@ -3751,35 +3760,47 @@ static int peer_endpoint_recover(sd_event_source *s, uint64_t usec, * response reporting the current EID. This is the test recommended by 8.17.6 * of DSP0236 v1.3.1. */ - rc = query_get_endpoint_id(ctx, &peer->phys, &peer->recovery.eid, - &peer->recovery.endpoint_type, - &peer->recovery.medium_spec, /*peer=*/NULL, - /*retry=*/false); - if (rc < 0) { - goto reschedule; - } + if (peer->gateway) { + /* + * For downstream endpoint recovery : + * + * If EID probe fails, check if the bridges are still responsive by walking up the bridge chain diagnosting point of exchange. + * In case we've got a response, verify UUID to detect device exchange at the same pool EID. + * for non matching UUID, remove old peer and publish the new peer with the same EID. + */ - /* - * If we've got a response there are two scenarios: - * - * 1. The device responds with the EID that we expect it to have - * 2. The device responds with an unexpected EID, e.g. 0 - * - * For scenario 1 we're done as the device is responsive and has the expected - * address. For scenario 2, we may not yet consider the EID assignment as - * expired, so check the UUID for a match. If the UUID matches we reassign the - * expected EID to the device. If the UUID does not match we allocate a new - * EID for the exchanged device, given it is responsive. - */ - if (peer->recovery.eid != peer->eid) { static const uint8_t nil_uuid[16] = { 0 }; bool uuid_matches_peer = false; bool uuid_matches_nil = false; uint8_t uuid[16] = { 0 }; - mctp_eid_t new_eid; - rc = query_get_peer_uuid_by_phys(ctx, &peer->phys, uuid); - if (!rc && peer->uuid) { + rc = query_get_endpoint_id(ctx, &peer->phys, + &peer->recovery.eid, + &peer->recovery.endpoint_type, + &peer->recovery.medium_spec, peer, + /*retry=*/false); + if (rc < 0) { + struct peer *gw = peer->gateway; + while (gw) { + int gw_rc = query_get_endpoint_id( + ctx, &gw->phys, &peer->recovery.eid, + &peer->recovery.endpoint_type, + &peer->recovery.medium_spec, + gw->gateway ? gw : NULL, + /*retry=*/false); + if (gw_rc < 0) + warnx("Recovery: EID %d unreachable; bridge EID %d failed to respond", + peer->eid, gw->eid); + gw = gw->gateway; + } + goto reschedule; + } + + rc = query_get_peer_uuid(peer, uuid); + if (rc < 0) + goto reschedule; + + if (peer->uuid) { static_assert(sizeof(uuid) == sizeof(nil_uuid), "Unsynchronized UUID sizes"); uuid_matches_peer = @@ -3788,43 +3809,114 @@ static int peer_endpoint_recover(sd_event_source *s, uint64_t usec, memcmp(uuid, nil_uuid, sizeof(uuid)) == 0; } - if (rc || !uuid_matches_peer || + if (!uuid_matches_peer || (uuid_matches_nil && !MCTPD_RECOVER_NIL_UUID)) { - /* It's not known to be the same device, allocate a new EID */ + /* + * The bridge has re-assigned this EID to a new device. + * Remove the old peer and register the new one with the + * same EID. + */ dest_phys phys = peer->phys; + mctp_eid_t eid = peer->eid; + uint32_t net = peer->net; + struct peer *gateway = peer->gateway; + struct peer *new_peer; assert(sd_event_source_get_enabled( peer->recovery.source, NULL) == 0); remove_peer(peer); - /* - * The representation of the old peer is now gone. Set up the new peer, - * after which we immediately return as there's no old peer state left to - * maintain. - */ - return endpoint_assign_eid(ctx, NULL, &phys, &peer, 0, - false); + rc = add_peer(ctx, &phys, eid, net, &new_peer, + /*allow_bridged=*/true); + if (rc < 0) + return rc; + + new_peer->gateway = gateway; + return setup_added_peer(new_peer); } - /* Confirmation of the same device, apply its already allocated EID */ - rc = endpoint_send_set_endpoint_id(peer, &new_eid, NULL); + } else { + // Directly-connected endpoint. + rc = query_get_endpoint_id(ctx, &peer->phys, + &peer->recovery.eid, + &peer->recovery.endpoint_type, + &peer->recovery.medium_spec, + /*peer=*/NULL, /*retry=*/false); if (rc < 0) { goto reschedule; } - if (new_eid != peer->eid) { - rc = change_peer_eid(peer, new_eid); - if (rc < 0) { - goto reclaim; + /* + * If we've got a response there are two scenarios: + * + * 1. The device responds with the EID that we expect it to have + * 2. The device responds with an unexpected EID, e.g. 0 + * + * For scenario 1 we're done as the device is responsive and has the expected + * address. For scenario 2, we may not yet consider the EID assignment as + * expired, so check the UUID for a match. If the UUID matches we reassign the + * expected EID to the device. If the UUID does not match we allocate a new + * EID for the exchanged device, given it is responsive. + */ + + if (peer->recovery.eid != peer->eid) { + static const uint8_t nil_uuid[16] = { 0 }; + bool uuid_matches_peer = false; + bool uuid_matches_nil = false; + uint8_t uuid[16] = { 0 }; + mctp_eid_t new_eid; + + rc = query_get_peer_uuid_by_phys(ctx, &peer->phys, + uuid); + if (!rc && peer->uuid) { + static_assert(sizeof(uuid) == sizeof(nil_uuid), + "Unsynchronized UUID sizes"); + uuid_matches_peer = memcmp(uuid, peer->uuid, + sizeof(uuid)) == 0; + uuid_matches_nil = memcmp(uuid, nil_uuid, + sizeof(uuid)) == 0; + } + + if (rc || !uuid_matches_peer || + (uuid_matches_nil && !MCTPD_RECOVER_NIL_UUID)) { + /* It's not known to be the same device, allocate a new EID */ + dest_phys phys = peer->phys; + + assert(sd_event_source_get_enabled( + peer->recovery.source, NULL) == + 0); + remove_peer(peer); + /* + * The representation of the old peer is now + * gone. Set up the new peer, after which we + * immediately return as there's no old peer + * state left to maintain. + */ + return endpoint_assign_eid(ctx, NULL, &phys, + &peer, 0, false); } - /* change_peer_eid() leaves the peer unpublished; the - * object path moves with the EID. The properties were - * queried when the peer was first set up, so - * publishing here re-registers the full interface - * set. */ - rc = publish_peer(peer); + + /* Confirmation of the same device, apply its already allocated EID */ + rc = endpoint_send_set_endpoint_id(peer, &new_eid, + NULL); if (rc < 0) { goto reschedule; } + + if (new_eid != peer->eid) { + rc = change_peer_eid(peer, new_eid); + if (rc < 0) { + goto reclaim; + } + /* change_peer_eid() leaves the peer unpublished; the + * object path moves with the EID. The properties were + * queried when the peer was first set up, so + * publishing here re-registers the full interface + * set. */ + rc = publish_peer(peer); + if (rc < 0) { + goto reschedule; + } + } } } @@ -6225,6 +6317,8 @@ static int peer_endpoint_poll(sd_event_source *s, uint64_t usec, void *userdata) &peer, true); if (rc < 0) goto exit; + // TODO: Need to add Query HOP logic to update immediate bridge peer's gateway chain + peer->gateway = bridge; } rc = setup_added_peer(peer); From 1416ab61b693f4237c609211cf08ef8bbe95498d Mon Sep 17 00:00:00 2001 From: Faizan Ali Date: Thu, 27 Aug 2026 13:12:39 +0530 Subject: [PATCH 2/2] tests: add downstream endpoint recovery tests Add tests cases covering recovery of bridged (downstream) endpoints Signed-off-by: Faizan Ali --- tests/test_mctpd.py | 275 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 275 insertions(+) diff --git a/tests/test_mctpd.py b/tests/test_mctpd.py index 4f75954..13d65d7 100644 --- a/tests/test_mctpd.py +++ b/tests/test_mctpd.py @@ -30,6 +30,7 @@ MCTPD_MCTP_I = 'au.com.codeconstruct.MCTP.BusOwner1' MCTPD_ENDPOINT_I = 'au.com.codeconstruct.MCTP.Endpoint1' MCTPD_ENDPOINT_BRIDGE_I = 'au.com.codeconstruct.MCTP.Bridge1' +OPENBMC_UUID_I = 'xyz.openbmc_project.Common.UUID' DBUS_OBJECT_MANAGER_I = 'org.freedesktop.DBus.ObjectManager' DBUS_PROPERTIES_I = 'org.freedesktop.DBus.Properties' @@ -2396,3 +2397,277 @@ async def test_iface_config_match_path_none(dbus, sysnet, nursery): res = await mctpd.stop_mctpd() assert res == 0 + + +async def _setup_bridge_with_downstream( + dbus, sysnet, nursery, num_downstream=1 +): + """Start mctpd, assign a bridge endpoint, wait for downstream discovery.""" + _DOWNSTREAM_POLL_MS = 2500 + config = f""" + [bus-owner] + endpoint_poll_ms = {_DOWNSTREAM_POLL_MS} + """ + mctpd = MctpdWrapper(dbus, sysnet, config=config) + await mctpd.start_mctpd(nursery) + + iface = mctpd.system.interfaces[0] + bridge_ep = mctpd.network.endpoints[0] + mctp = await mctpd_mctp_iface_obj(dbus, iface) + + downstream_eps = [] + for _ in range(num_downstream): + bep = Endpoint(iface, bytes(), types=[0]) + mctpd.network.add_endpoint(bep) + bridge_ep.add_bridged_ep(bep) + downstream_eps.append(bep) + + # Subscribe to InterfacesAdded AFTER assign_endpoint returns so we don't + # accidentally match the bridge's own Endpoint1 signal (emitted before its + # separate Bridge1 signal, so it would pass the downstream filter). + (_, _, _, new) = await mctp.call_assign_endpoint(bridge_ep.lladdr) + assert new + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + downstream_paths = [] + discovered = trio.Semaphore(initial_value=0) + + def ep_added(ep_path, content): + if ( + MCTPD_ENDPOINT_I in content + and MCTPD_ENDPOINT_BRIDGE_I not in content + ): + downstream_paths.append(ep_path) + discovered.release() + + await mctp_objmgr.on_interfaces_added(ep_added) + + with trio.move_on_after(_DOWNSTREAM_POLL_MS / 1000 * 3) as scope: + for _ in range(num_downstream): + await discovered.acquire() + + assert not scope.cancelled_caught, ( + "Timed out waiting for downstream EP discovery" + ) + + return mctpd, bridge_ep, downstream_eps, downstream_paths + + +async def test_recover_downstream_present( + dbus, sysnet, nursery, autojump_clock +): + """Test Downstream peer still alive: recovery transitions Connectivity to Available.""" + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + recovered = trio.Semaphore(initial_value=0) + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Available': + recovered.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await recovered.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer did not recover to Available" + ) + + res = await mctpd.stop_mctpd() + assert res == 0 + + +async def test_recover_downstream_removed( + dbus, sysnet, nursery, autojump_clock +): + """Test if downstream peer gone while bridge is up: peer is removed after all retries.""" + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + degraded = trio.Semaphore(initial_value=0) + removed = trio.Semaphore(initial_value=0) + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Degraded': + degraded.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + def on_ep_removed(ep_path, interfaces): + if ep_path == ds_path and MCTPD_ENDPOINT_I in interfaces: + removed.release() + + await mctp_objmgr.on_interfaces_removed(on_ep_removed) + + # Disconnect downstream — bridge mock stops routing EID-addressed packets to it. + bridge_ep.bridged_eps.remove(downstream_eps[0]) + + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await removed.acquire() + await degraded.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer was not removed after losing connectivity" + ) + + res = await mctpd.stop_mctpd() + assert res == 0 + + +async def test_recover_downstream_bridge_unreachable( + dbus, sysnet, nursery, autojump_clock +): + """Test if bridge unreachable: downstream peer is removed when physical probe fails.""" + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + degraded = trio.Semaphore(initial_value=0) + removed = trio.Semaphore(initial_value=0) + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Degraded': + degraded.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + def on_ep_removed(ep_path, interfaces): + if ep_path == ds_path and MCTPD_ENDPOINT_I in interfaces: + removed.release() + + await mctp_objmgr.on_interfaces_removed(on_ep_removed) + + mctpd.network.endpoints.remove(bridge_ep) + + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await removed.acquire() + await degraded.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer was not removed after bridge became unreachable" + ) + + res = await mctpd.stop_mctpd() + assert res == 0 + + +async def test_recover_downstream_exchange( + dbus, sysnet, nursery, autojump_clock +): + """Test if bridge reassigns pool EID to a new device, old peer removed, new peer added.""" + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + degraded = trio.Semaphore(initial_value=0) + removed = trio.Semaphore(initial_value=0) + added = trio.Semaphore(initial_value=0) + added_uuid = [] + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Degraded': + degraded.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + def on_ep_removed(ep_path, interfaces): + if ep_path == ds_path and MCTPD_ENDPOINT_I in interfaces: + removed.release() + + def on_ep_added(ep_path, content): + if ep_path == ds_path and MCTPD_ENDPOINT_I in content: + if OPENBMC_UUID_I in content and 'UUID' in content[OPENBMC_UUID_I]: + added_uuid.append(content[OPENBMC_UUID_I]['UUID'].value) + added.release() + + await mctp_objmgr.on_interfaces_removed(on_ep_removed) + await mctp_objmgr.on_interfaces_added(on_ep_added) + + old_ds = downstream_eps[0] + new_ds = Endpoint(old_ds.iface, bytes(), types=[0]) + new_ds.eid = old_ds.eid # same pool EID, fresh UUID + + bridge_ep.bridged_eps.remove(old_ds) + bridge_ep.bridged_eps.append(new_ds) + mctpd.network.add_endpoint(new_ds) + + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await removed.acquire() + await added.acquire() + await degraded.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer exchange did not emit Remove + Add" + ) + + # Verify the re-added peer carries the new device's UUID, not the old one. + assert added_uuid, "InterfacesAdded did not include a UUID property" + assert added_uuid[0] == str(new_ds.uuid), ( + f"New peer UUID {added_uuid[0]!r} does not match " + f"replacement device UUID {new_ds.uuid!r}" + ) + + res = await mctpd.stop_mctpd() + assert res == 0