mctpd: Support MCTP Discovery Notify command by chajasmine-bit · Pull Request #165 · CodeConstruct/mctp

chajasmine-bit · 2026-07-30T08:05:18Z

Implement support for the MCTP Discovery Notify control command in mctpd. When an MCTP endpoint issues a Discovery Notify control request to the Bus Owner, mctpd immediately acknowledges the request over the physical socket and defers EID assignment to the main systemd event loop.

This avoids blocking the event thread during control message processing and safely handles EID re-assignments via change_peer_eid(), keeping D-Bus object paths and netlink kernel routing tables synchronized. Also include unit test coverage for Discovery Notify in the test suite.

Assisted-by: Antigravity:Gemini-Next

jk-ozlabs · 2026-08-05T02:28:48Z

I see there's been a few updates; let me know when this is stable and you'd like a review.

chajasmine-bit · 2026-08-05T04:42:31Z

I see there's been a few updates; let me know when this is stable and you'd like a review.

The code is ready for review, please take a look at this. Thanks.

jk-ozlabs

Thanks for the contribution. I have a few comments.

On the commit message: it reads like a marketing pitch rather than an explanation of the design, or rationale for the implementation. While you don't need to describe the individual changes, I would appreciate some background on the approach, and non-obvious parts of the implementation (like, why the new NLM_F flags?)

In MCTP networks, endpoints broadcast or send a Discovery Notify control
request (0x0D) to inform the Bus Owner when they boot up, reset, or are
hot-plugged. Currently, mctpd relies on active bus scanning or static
configurations,

or primarily: hot-plug events, where the transport provides them

leaving newly online endpoints undiscovered until the next poll cycle.

What poll cycle?

chajasmine-bit · 2026-08-12T02:22:35Z

Hi Jeremy, thanks for catching the issues from the previous code and I've learned a lot from your feedback. I’ve made the updates, so please take a look when you have a chance.

jk-ozlabs · 2026-08-13T13:49:04Z

Will do, thanks for the updates! I'm travelling at the moment, so might be a couple of days before I can get to it, but I'll get a review done shortly.

msnidhin · 2026-08-14T07:59:17Z

Hi.
What is the guideline for Discovery Notify vs the dbus-sensors MCTPI3CTarget reactor flow?
Do we need to support both modes?

amboar · 2026-08-17T01:04:08Z

Hi. What is the guideline for Discovery Notify vs the dbus-sensors MCTPI3CTarget reactor flow? Do we need to support both modes?

I expect we need to support both modes (DSP0233 v1.0.1 § 5.1.3):

An I3C Target that acts as an MCTP Bus Owner cannot be added to an I3C bus using the I3C hot-join mechanism.

chajasmine-bit · 2026-08-20T05:24:24Z

Hi @jk-ozlabs and @amboar, could you please let me know if my understanding is correct, or if any further changes are needed for this commit?

jk-ozlabs · 2026-08-20T05:43:55Z

I think we've converged on the desired design here, I just need to do an actual review. I will get on to that shortly, but there are a couple of other fixes that I need to look at first.

jk-ozlabs · 2026-08-24T05:03:08Z

+		}
+		warnx("reply_message: EID %d specified without valid physical address info",
+		      reply_addr.smctp_addr.s_addr);
+		return -EINVAL;


What is this change needed for?

This seems to be adding a physical-addressed reply fallback, but that's explicitly not what reply_message() is for (as suggested by the function comment), as we have reply_message_phys() for that.

This commit only adds a new call directly to reply_message_phys() , so why the new fallback?

jk-ozlabs · 2026-08-24T05:07:35Z

+		    oldest_time) {
+			oldest_time =
+				link_data->rate_limits[i].last_discovery_time_us;
+			oldest_idx = i;


Why store a pointer for the matching entry, but an index for the oldest? Can this be made consistent?

jk-ozlabs · 2026-08-24T05:16:13Z

+				warnx("Failed to defer EID assignment event for %s",
+				      dest_phys_tostr(&phys));
+			}
+			ctx->pending_discoveries = dctx->next;


Might be easier to add to the pending list on success, rather than adding and the removing on failure.

jk-ozlabs · 2026-08-24T05:16:53Z

+		return rc;
+	}
+
+	/* Deduplicate incoming Discovery Notify for both existing and new physical endpoints */


Minor, but please try to keep under 80 cols where possible. The C strings going over length are okay though.

jk-ozlabs · 2026-08-24T05:26:49Z

+			warnx("Discovery Notify received for existing peer %s (EID %d); tearing down for re-discovery",
+			      peer_tostr(peer), peer->eid);
+		}
+		remove_peer(peer);


More of a question than a change request, but: Is it possible that we could be doing this non-destructively? (say, if the re-discovered endpoint has the same UUID, and ends up with the same EID allocated)

My concern is that if we see a stray Discovery Notify (for whatever reason) that will interrupt connectivity to the endpoint. As I mentioned earlier, we may be able to use the recovery path for this.

Or, if we decide that receiving a Discovery Notify is due cause to re-initialise completely, then that's also OK, but we should probably document that somewhere.

jk-ozlabs · 2026-08-24T05:30:34Z

+
+		process_deferred_discovery(ctx, dctx);
+		free(dctx);
+	}


In your commit message, you have:

- Defer EID assignment/discovery to event loop to keep D-Bus unblocked (ack immediately).

But this loop seems to contradict this objective. An enqueued set of deferred discoveries will now block until all of them are processed, which would seem to have worse dbus behaviour than just processing immediately on Discovery Notify receive.

I do see the point of deferring, in order to meet message response timing, but if you're doing it for dbus-blocking reasons, this seems to be exactly the wrong approach.

Point-to-point physical links (e.g., Serial, USB, KCS) do not provide presence interrupts when an endpoint attaches or reboots, requiring manual intervention to trigger endpoint discovery. Add autonomous periodic probing for point-to-point interfaces in Bus Owner mode. When an interface is brought UP, mctpd periodically sends Get Endpoint ID requests until the endpoint responds, assigns an EID, and publishes the endpoint on D-Bus. Probing automatically pauses when the link goes DOWN and resumes if the peer is removed. Also add configuration options in mctpd.conf for auto-discovery, probe intervals, and static EID reservations, along with corresponding unit tests. Signed-off-by: Jasmine Cha <chajasmine@google.com>

jk-ozlabs

A few requests inline, but some comments overall too:

Point-to-point physical links (e.g., Serial, USB, KCS) do not provide
presence interrupts when an endpoint attaches or reboots,

That's not correct for USB though, where we do have an explicit notification of the link being attached. This seems to be a conflation between peer-to-peer links, and links that have no internal presence mechanism.

However, as you say next, the actual trigger for peer polling is the IFF_UP event. What architecture are you proposing to cause that? We still need an external mechanism for that, right? Or does it make more sense to handle that within mctpd?

(perhaps these are design points that should be covered on #174 still)

jk-ozlabs · 2026-09-01T08:34:37Z

+		}
+		ctx->probe_interval_us = i * 1000;
+	}
+


You have this block almost-exactly repeated for the per-interface configuration. Can you use a common helper?

jk-ozlabs · 2026-09-01T08:35:57Z


+	conf_str = toml_bool_in(interface, "auto_discovery");
+	if (!conf_str.ok)
+		conf_str = toml_bool_in(interface, "auto-discovery");


Why the two forms of configuration name? This isn't done anywhere else.

If we really want to support the two styles, it should be common across all options (and likely using a helper to handle that), not just the new ones you are adding here.

... but I'm not convinced it's needed anyway.

jk-ozlabs · 2026-09-01T08:42:49Z

+* `role`: sets mctpd's role as either `bus-owner` or `endpoint` on this interface.
+* `auto_discovery`: overrides global `auto_discovery` for this interface (`true`/`false`).
+* `probe_interval_ms`: overrides global probe interval in milliseconds (`100` - `60000`).
+* `static_eid`: assigns a fixed static EID (must be in `[8, dynamic_eid_start)`) upon successful probe response, preventing dynamic EID allocation collisions.


Please introduce the concept of static peer EIDs as a separate change, as it will have implications across all of the autodiscovery methods.

Also, the name static_eid is too ambiguous; currently it reads as if that is a static EID that gets assigned to the interface.

Why does the static EID need to be lower than the dynamic EID range, rather than just not within the range?

chajasmine-bit force-pushed the discovery-notify branch 5 times, most recently from 9274ea2 to f6fbc3b Compare August 4, 2026 03:50

jk-ozlabs requested changes Aug 7, 2026

View reviewed changes

chajasmine-bit force-pushed the discovery-notify branch 3 times, most recently from b554b2f to 71201d5 Compare August 11, 2026 14:10

chajasmine-bit requested a review from jk-ozlabs August 12, 2026 02:19

jk-ozlabs requested changes Aug 24, 2026

View reviewed changes

This was referenced Aug 24, 2026

Define / update our peer discovery strategy #174

Open

mctpd: handle Discovery Notify (DSP0236 command 0x0D) #176

Closed

chajasmine-bit force-pushed the discovery-notify branch from 71201d5 to 0946fe2 Compare September 1, 2026 06:19

jk-ozlabs reviewed Sep 1, 2026

View reviewed changes

Uh oh!

Conversation

chajasmine-bit commented Jul 30, 2026

Uh oh!

jk-ozlabs commented Aug 5, 2026

Uh oh!

chajasmine-bit commented Aug 5, 2026

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chajasmine-bit commented Aug 12, 2026

Uh oh!

jk-ozlabs commented Aug 13, 2026

Uh oh!

msnidhin commented Aug 14, 2026

Uh oh!

amboar commented Aug 17, 2026

Uh oh!

chajasmine-bit commented Aug 20, 2026

Uh oh!

jk-ozlabs commented Aug 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants