mctpd: Send Discovery Notify on Endpoint role set #15

khangng-ampere · 2023-10-06T02:58:44Z

Implement the first step of partial discovery, which is notifying the
bus owner of our presence on the bus.

Tested: Discovery Notify message is sent when setting role on interface.

Jul 15 08:33:42 evb-endpoint mctpd[3322]: mctpd: read_message got from sockaddr_mctp_ext eid 8 net 1 type 0x00 if 2 hw len 2 0x80:08 len 3
Jul 15 08:33:42 evb-endpoint mctpd[3322]: mctpd: Failure completion code 0x05 from physaddr if 2 hw len 2 0x00:00
Jul 15 08:33:42 evb-endpoint mctpd[3322]: mctpd: Warning: discovery notify on interface 'mctppcie0' failed: Connection refused

(My Root Complex does not handle Discovery Notify message yet, but
can confirm that the message is sent)

jk-ozlabs · 2024-01-19T06:06:25Z

The code looks fine, but why is this triggered by a dbus method? Shouldn't mctpd be in control of when the Discovery Notify messages are sent in the first place?

Even if not, this seems like the wrong abstraction to be exposing over dbus. What's the actual state/event that we want to expose in this scenario?

(also, can you share the PCIe binding driver?)

khangng-ampere · 2024-01-22T08:35:14Z

The code looks fine, but why is this triggered by a dbus method? Shouldn't mctpd be in control of when the Discovery Notify messages are sent in the first place?

Even if not, this seems like the wrong abstraction to be exposing over dbus. What's the actual state/event that we want to expose in this scenario?

Yeah, I agree, at the time, I wasn't thinking much about when to send the Discovery Notify messages, so I left that to the method caller to decide. I suppose we could do this inside mctpd anyway.

After giving it some thought, I think my main blocker to rework this PR is how to know when the PCI address is gone. This seems like something that netlink would provide, but atm I couldn't figure out how to tell apart media types of a link.

For example, is there a way to know where a link comes from, is it a mctp-i2c link or mctp-serial link?

EDIT: for the PCIe binding driver, I'm afraid it is not something I have the permission to share just yet.

ThuBaNguyen · 2024-02-20T11:37:18Z

The code looks fine, but why is this triggered by a dbus method? Shouldn't mctpd be in control of when the Discovery Notify messages are sent in the first place?

Even if not, this seems like the wrong abstraction to be exposing over dbus. What's the actual state/event that we want to expose in this scenario?

(also, can you share the PCIe binding driver?)

The D-Bus method provides the method consumer ability to decide the time which the BO can start initialize/discovery the BMC thru PCIe medium interface. Example, when the BMC boot up with the host is On, if BMC is set as Endpoint the other service can detect the available of the host then call Discovery Notify to BO to setEID for BMC.
In theory, BMC does not need to do that but I don't think propose the Discovery Notify will cause any potential problem.

jk-ozlabs · 2024-02-26T02:30:19Z

In theory, BMC does not need to do that but I don't think propose the Discovery Notify will cause any potential problem.

My concern is allowing callers to drive details of the MCTP Control Protocol implementation, rather than having those decisions made by mctpd internally.

However, that could be matter of making this call more of a "notify the bus owner of our presence" operation rather than "send a Notify Discovery" message. For PCIe links, that would be equivalent, but other links (ie SMBus) may be a no-op (or could return an error?).

But still: DSP0238 defines the specific enumeration / discovery process, and section 6.9.1 gives specific events which would trigger the control protocol implementation to send a Notify Discovery message. I suspect that only mctpd would have full knowledge of when these events would occur. Are you proposing external components are tracking the MCTP enumeration state? Otherwise, how do they know when it is suitable to send a Notify Discovery message?

Some knowledge of the interactions to the PCIe binding driver might be helpful here. Can you share that?

Example, when the BMC boot up with the host is On, if BMC is set as Endpoint the other service can detect the available of the host then call Discovery Notify to BO to setEID for BMC.

Right, and that is best handled by mctpd automatically sending a Notify Discovery message when the "discovered" flag is not set, as per 6.9.1.

ThuBaNguyen · 2024-03-18T04:32:11Z

In theory, BMC does not need to do that but I don't think propose the Discovery Notify will cause any potential problem.

My concern is allowing callers to drive details of the MCTP Control Protocol implementation, rather than having those decisions made by mctpd internally.

However, that could be matter of making this call more of a "notify the bus owner of our presence" operation rather than "send a Notify Discovery" message. For PCIe links, that would be equivalent, but other links (ie SMBus) may be a no-op (or could return an error?).

But still: DSP0238 defines the specific enumeration / discovery process, and section 6.9.1 gives specific events which would trigger the control protocol implementation to send a Notify Discovery message. I suspect that only mctpd would have full knowledge of when these events would occur. Are you proposing external components are tracking the MCTP enumeration state? Otherwise, how do they know when it is suitable to send a Notify Discovery message?

I agree with above point, maybe we don't need public the Notify Discovery method to D-Bus.

Some knowledge of the interactions to the PCIe binding driver might be helpful here. Can you share that?

I'm asking my manager for this. Currently, we do internal review to make the clean code before create MR to kernel.

Example, when the BMC boot up with the host is On, if BMC is set as Endpoint the other service can detect the available of the host then call Discovery Notify to BO to setEID for BMC.

Right, and that is best handled by mctpd automatically sending a Notify Discovery message when the "discovered" flag is not set, as per 6.9.1.

Agree.

So We don't need support Notify Discovery to D-Bus method.

khangng-ampere · 2024-07-15T09:22:33Z

Piggyback on recent changes, I think the most suitable time to send Discovery Notify now is when user set link role to Endpoint. I updated my PR.

jk-ozlabs · 2024-07-16T01:05:44Z

Nice one. I'll take a look shortly.

khangng-ampere · 2024-09-05T08:48:40Z

Hi @jk-ozlabs, do you have any concerns regarding this?

jk-ozlabs · 2024-09-05T11:17:42Z

Ah, no concerns, just travelling at the moment. I'll look at merging when I'm back at the deck next week.

I have a v2.0 release queued though, any preference on whether this is merged before or after that?

khangng-ampere · 2024-09-06T03:15:44Z

Ah, no concerns, just travelling at the moment. I'll look at merging when I'm back at the deck next week.

I have a v2.0 release queued though, any preference on whether this is merged before or after that?

Let's do it after. I have a few more PRs to support the rest of the discovery process for endpoints, so they should probably be batched in one release.

jk-ozlabs · 2024-09-08T11:01:43Z

src/mctpd.c

+	req.ctrl_hdr.rq_dgram_inst = RQDI_REQ;
+
+	rc = endpoint_query_phys(ctx, dest, MCTP_CTRL_HDR_MSG_TYPE, &req,
+				 sizeof(req), &buf, &buf_size, &resp_addr);


endpoint_query_phys will wait for a response, but Discovery Notify does not send a response. We'll probably need a different helper for datagram-style messages.

jk-ozlabs · 2024-09-08T11:02:43Z

src/mctpd.c


+	// Announce on the bus we are endpoint, print warning and ignore error if failed
+	if (lmUserData->role == ENDPOINT_ROLE_ENDPOINT) {
+		rc = notify_discovery(ctx, ifindex);


This is done unconditionally, but Discovery Notify messages should only be sent on transport types that require them. We may need a way to query the transport type?

@jk-ozlabs It would be great if we have that now (we need them to get control message timing information for the recovery mechanism on each interface anyway).

I would propose adding such information to when a binding driver register itself to the MCTP networking subsystem inside the kernel (mctp_netdev_ops), like so:

static const struct mctp_netdev_ops mctp_i2c_mctp_ops = { .transport = MCTP_TRANSPORT_SMBUS, .release_flow = mctp_i2c_release_flow, };

And then exposing them to userspace through netlink.

I don't know how long it would take to merge the patch to linux, but in the meanwhile, we may temporarily provide such information in mctpd by basing on the interface name i think. Everything is already somewhat following the convention: mctpi2c%d, mctpserial%d, ...

I could probably open a PR for this if you think this is ok.

Yeah, we'd need the kernel to provide the physical medium identifier. It doesn't belong in the ops struct, but there are certainly ways we can provide that to the struct mctp_dev in the kernel (and then on to the netlink interface as a new attribute; say IFLA_MCTP_PHYS_MEDIUM).

Given that:

the PCIe VDM transport is the only one that requires a Discovery Notify

that transport driver is not yet upstream

none of the currently-upstream transport drivers provide bindings that require Discovery Notify messages

- then I would suggest that we add the infrastructure for IFLA_MCTP_PHYS_MEDIUM, and have your driver provide the attribute. Then, mctpd will only send Discovery Notify messages attribute is present (ie., we know the medium type) and matches the media types that need a Discovery Notify.

Since the Discovery Notify is only required for the non-upstream transport, you'll need kernel patches anyway.

we may temporarily provide such information in mctpd by basing on the interface name i think. Everything is already somewhat following the convention: mctpi2c%d, mctpserial%d, ...

Interface names are not a stable API that we can rely on; they can be renamed arbitrarily.

I'm happy to do the IFLA_MCTP_PHYS_MEDIUM support; drivers will just need a minor change to populate the type value.

I'm happy to do the IFLA_MCTP_PHYS_MEDIUM support; drivers will just need a minor change to populate the type value.

That would be great 😀 Thank you!

src/mctpd.c

src/mctp-netlink.c

jk-ozlabs · 2024-09-10T08:34:28Z

src/mctpd.c

+
+	dest->ifindex = ifindex;
+	mctp_nl_ifaddr_byindex(ctx->nl, dest->ifindex, &dest->hwaddr_len);
+	memset(dest->hwaddr, 0, sizeof dest->hwaddr);


A zero hwaddr is specific to PCIe, right? Do we also need to know the physical broadcast address for this link type?

@jk-ozlabs The MCTP base spec does not specify that, so I think this depends on the link. (And it is the "default" route to the bus owner, not specifically "broadcast").

As for PCIe, you actually need to specify a routing type before sending it to the bus (Route-to-Root-Complex, Broadcast-from-Root-Complex, Route-by-ID), which I don't find a way to put that in the sockaddr, so I pick the following addressing convention:

0x00:00 for "default route", which corresponds to Route-to-Root-Complex,

0xFF:FF for "broadcast route", which corresponds to Broadcast-from-Root-Complex,

The rest corresponds to Route-by-ID.

(Only Route-by-ID routing type will actually use the address, the formers will simply ignore the value per the PCIe or DSP0238 spec)

which I don't find a way to put that in the sockaddr

phys addresses don't belong in the struct sockaddr_mctp, they would go into the smctp_haddr of the struct sockaddr_mctp_ext, which is just bytes. If you need the routing type represented in there as well, that's entirely possible too.

But this sounds like review details on the driver itself, so probably not the best forum here :)

We do need to ensure we're using values that would work for any transport in future though, and an all-zero dest phys may not be suitable for those...

khangng-ampere

Refreshing this 2 years old patch 😀

src/mctpd.c

tests/test_mctpd_endpoint.py

khangng-ampere · 2025-10-28T04:40:21Z

I rebased this PR on my sd-event timer PR to add a retry test. There was a bug that I forgot to rearm the sd_event_source.

This commit adds support for Discovery Notify messages, specified in DSP0236 section 12.14. In our implementation, a Discovery Notify message is sent when mctpd sets an interface role from Unknown to Endpoint. To avoid notify discovery messages getting lost, retry the messages for a few time with delays. Signed-off-by: Khang D Nguyen <[email protected]>

jk-ozlabs · 2025-12-03T07:39:34Z

src/mctpd.c

 	sd_bus_slot *slot_busowner;

+	struct {
+		enum discovery_state flag;


Super minor, but 'flag' implies a boolean, and we're not quite aligned with the "discovered flag" concept in DSP0236. How about just state for this?

Acknowledged

jk-ozlabs · 2025-12-03T07:40:22Z

src/mctpd.c

+		link->discovery.notify_tries_left = 3;
+		link->discovery.notify_retry_delay = 5000000;
+
+		// For PCIe-VDM, we want an all zeroes address for Route-to-Root-Complex.


What is defining these address semantics?

Given there is no convention upstream for the format of VDM addresses, how do we know this is correct?

There is indeed no VDM supports yet in upstream, and the addresses format has only been discussed in email threads and non-upstreamed drivers...

Do you have any suggestions to resolve or move this forward? It would be great if this could be resolved independently from the kernel changes.

Would changing this into using IFLA_BROADCAST for broadcast hardware address (and only support discovery for MCTP bindings that 1) requires discovery in the spec and 2) support this attribute in the binding driver) be more sensible? IFLA_BROADCAST seems to be the most correct place to get this info from.

Oh wait, I got it wrong - Route-to-root-complex is not broadcast (that is Broadcast-from-Root-Complex), but rather default / "gateway"-ish address... Not sure if there is a Linux netlink attribute for that...

There is indeed no VDM supports yet in upstream, and the addresses format has only been discussed in email threads and non-upstreamed drivers...

My concern is that we end up committing to an addressing representation that has not been reviewed. I am aware of some prior discussions there, but nothing that seemed like a definite plan at this stage.

Ideally, we'd at least have some indication of an addressing format upstream (or at least proposed upstream), so that we have some confidence that this is correct.

Would changing this into using IFLA_BROADCAST

That only covers the Broadcast From RC addressing type though? We're after Route to RC here, no?

tests/test_mctpd_endpoint.py

jk-ozlabs · 2025-12-03T07:43:30Z

src/mctpd.c


 	if (phys_binding == MCTP_PHYS_BINDING_PCIE_VDM) {
-		link->discovered = DISCOVERY_UNDISCOVERED;
+		link->discovery.flag = DISCOVERY_UNDISCOVERED;


We want the discovery process (below) to work for any binding type that supports Discovery Notify, not just VDM, right?

Should we move that out to be conditional on link->discovery.flag == DISCOVERY_UNDISCOVERED ?

(there may be some binding-specific addressing info needed, but the rest should be common?)

I see, I will split in to two blocks: one for setting the flag to DISCOVERY_UNDISCOVERED, and one for conditional on the flag to enable the discovery process

khangng-ampere · 2025-12-05T02:09:25Z

I think I will look into replacing PCIe-VDM with I3C in this PR. I3C supports Discovery Notify, already upstreamed, so the address should be standard. The process begins with a Get MCTP Version though, so I will need to tweak it a bit.

khangng-ampere force-pushed the notify-discovery branch from 238125d to a15115a Compare July 15, 2024 09:00

khangng-ampere changed the title ~~mctpd: Add NotifyDiscovery D-Bus method~~ mctpd: Send Discovery Notify on Endpoint role set Jul 15, 2024

jk-ozlabs requested changes Sep 8, 2024

View reviewed changes

khangng-ampere force-pushed the notify-discovery branch from a15115a to 73e2567 Compare September 10, 2024 03:15

jk-ozlabs reviewed Sep 10, 2024

View reviewed changes

khangng-ampere force-pushed the notify-discovery branch from 73e2567 to f807b33 Compare August 29, 2025 06:53

khangng-ampere commented Aug 29, 2025

View reviewed changes

src/mctpd.c Show resolved Hide resolved

tests/test_mctpd_endpoint.py Show resolved Hide resolved

khangng-ampere mentioned this pull request Oct 15, 2025

mctpd: replace sd-event timers #123

Merged

khangng-ampere force-pushed the notify-discovery branch from f807b33 to fce9ccd Compare October 28, 2025 04:38

khangng-ampere force-pushed the notify-discovery branch 4 times, most recently from fc4fba4 to 7d9c672 Compare October 31, 2025 04:03

khangng-ampere force-pushed the notify-discovery branch from 7d9c672 to bb0dff1 Compare November 12, 2025 07:17

jk-ozlabs reviewed Dec 3, 2025

View reviewed changes

mctpd: Send Discovery Notify on Endpoint role set #15

Are you sure you want to change the base?

mctpd: Send Discovery Notify on Endpoint role set #15

Uh oh!

Conversation

khangng-ampere commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jk-ozlabs commented Jan 19, 2024

Uh oh!

khangng-ampere commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThuBaNguyen commented Feb 20, 2024

Uh oh!

jk-ozlabs commented Feb 26, 2024

Uh oh!

ThuBaNguyen commented Mar 18, 2024

Uh oh!

khangng-ampere commented Jul 15, 2024

Uh oh!

jk-ozlabs commented Jul 16, 2024

Uh oh!

khangng-ampere commented Sep 5, 2024

Uh oh!

jk-ozlabs commented Sep 5, 2024

Uh oh!

khangng-ampere commented Sep 6, 2024

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khangng-ampere Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khangng-ampere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

khangng-ampere commented Oct 28, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khangng-ampere commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

khangng-ampere commented Oct 6, 2023 •

edited

Loading

khangng-ampere commented Jan 22, 2024 •

edited

Loading

khangng-ampere Sep 10, 2024 •

edited

Loading