Skip to content

mctpd: Add AttemptDiscoveryNotify D-Bus method for endpoint role #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
msnidhin wants to merge 1 commit into
base: main
Choose a base branch
from
+158 −10
Open

mctpd: Add AttemptDiscoveryNotify D-Bus method for endpoint role #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion docs/mctpd.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ configured value is `Unknown`. Other platform setup infrastructure may use
this to configure the initial MCTP state of the platform.

When the interface `Role` is `BusOwner`, the MCTP interface object will
also host the `BusOwner1` dbus interface:
also host the `BusOwner1` dbus interface. When the `Role` is `Endpoint`,
the MCTP interface object will also host the `Endpoint1` dbus interface.

The `NetworkId` property represents the network on which this interface is
present.
Expand Down Expand Up @@ -217,6 +218,43 @@ reports as a bridge.

Bridge endpoints should be initialised with `AssignEndpoint` instead.

### Endpoint interface: `au.com.codeconstruct.MCTP.Endpoint1` interface

When the interface `Role` is `Endpoint`, the MCTP interface object also hosts
the `au.com.codeconstruct.MCTP.Endpoint1` D-Bus interface. This exposes
endpoint-role functions on the per-interface D-Bus path.

```
NAME TYPE SIGNATURE RESULT/VALUE FLAGS
au.com.codeconstruct.MCTP.Interface1 interface - - -
.NetworkId property u 1 emits-change
.Role property s "Endpoint" emits-change writable
au.com.codeconstruct.MCTP.Endpoint1 interface - - -
.AttemptDiscoveryNotify method ay - -
```

#### `.AttemptDiscoveryNotify`: `ay`

@jk-ozlabs jk-ozlabs Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only applicable for certain transport binding types, we may want to put this into an interface that is specific to those transports, rather than Endpoint1 which should be common across all. Any thoughts on that?

@msnidhin msnidhin Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Like only if link->phys_binding == MCTP_PHYS_BINDING_PCIE_VDM or link->phys_binding == MCTP_PHYS_BINDING_I3C ?

also what interface name to be used ?


Some physical transports (such as MCTP-over-I3C) require the endpoint to
send a Discovery Notify to announce its presence before the bus owner will
enumerate it. On those interfaces this method can be called before the
endpoint will be assigned an EID.
Comment on lines +238 to +241

@jk-ozlabs jk-ozlabs Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't exactly correct; in most cases, the I3C peer has explicit knowledge of the presence of the new MCTP endpoint.

I'm not clear on your specific use-case here, could you expand this on the scenario where this is required?

@msnidhin msnidhin Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. In my use case i3c address assignment happened early in the boot. At that time the mctp stack in endpoint mode was not ready. It took some time for mctpd service to start. After that I want to send discovery notify and announce endpoint presence.

@jk-ozlabs jk-ozlabs Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so it would work to have mctpd automatically sent the Dicovery Notify when the local interface is found? (or when the role is set to Endpoint?)

That way you don't need the dbus API at all, nor external infrastructure to call it.

Or does this need to be sequenced with other things?

@msnidhin msnidhin Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. sending the command when Role is set to endpoint also works.
But as discussed before mctpd won't know the physical address of the BO. Some external apps like mctpreactor should supply this.

@jk-ozlabs jk-ozlabs Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that as an endpoint, we should be able to address the BO without a-prior knowledge of any physical addresses. The IBI mechanism for I3C should mean that no phys addresses are required for target-to-primary commuinication, right?

Given there is no upstream I3C target implementation though, we don't have any specifics of that.

@msnidhin msnidhin Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. The use case is the red network in this image from DSP0233.
Section 5.1.3 MCTP Bus Owner for I3C bus
image
MCTP BO is an I3C target. I know the PID of the BO and need to send Discovery Notify.
And I didn't find any method to send this command without using the address of the BO.

@jk-ozlabs jk-ozlabs Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that the endpoint must already know the phys address of the bus owner in that case (because, as the I3C controller, it assigned the I3C address to it!)

But since I have no idea what your MCTP-over-I3C transport binding implementation looks like, I can't help with how you could hook that into mctpd. I would suggest that mctpd could handle this entirely internally.

Please propose your binding implementation upstream, so we can reason on a good solution for this.

@jk-ozlabs jk-ozlabs Jul 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my confusion there - it had occurred to me that you are using the upstream MCTP-over-I3C-(controller) binding, just in MCTP endpoint mode (ie., implementing the Bridge 2 in your diagram).

Your unique case here is that the transport itself cannot provide the bus owner physaddr, as it could be any one of the I3C targets connected to the controller.

In which case, I think a "set the bus owner" interface would be more appropriate, and would be applicable to the Endpoint1 interface in general. For transport types that know the bus owner physaddr (or do not need one to address the BO), this would be populated by the transport binding (typically, via netlink link config). For your case, it would be through the new interface to set the bus owner.

The flow would be:

When all of the following are true:

  • the interface has been discovered
  • the interface role is set as an endpoint
  • we know the bus owner physaddr

Then we start the Discovery Notify process:

  • send a discovery notify to the BO
  • on timeout (or non-fatal errors) retry after some reasonable delay
  • on actual discovery, cease the Discovery notify process

That then solves the below issue around having the dbus caller have to handle messaging errors; setting the bus owner may start the discovery process, rather than having the caller be responsible for making progress.

I'd need to confirm this fits with the full discovery process required for other binding specs, but that's a job for Monday.


`AttemptDiscoveryNotify <hwaddr>`

- `<hwaddr>` Physical hardware address of the bus owner. For MCTP-over-I3C
this is the 6-byte Provisional ID (PID) of the bus owner.

An example for MCTP-over-I3C, sending a Discovery Notify using the bus owner's
6-byte PID `00 6c 90 01 23 45`:

```shell
busctl call au.com.codeconstruct.MCTP1 \
/au/com/codeconstruct/mctp1/interfaces/mctpi3c0 \
au.com.codeconstruct.MCTP.Endpoint1 \
AttemptDiscoveryNotify ay 6 0x00 0x6c 0x90 0x01 0x23 0x45
```

## Network objects: `/au/com/codeconstruct/networks/<net>`

These objects represent MCTP networks which have been added use `mctp link`
Expand Down
104 changes: 95 additions & 9 deletions src/mctpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ struct link {
char *path;
sd_bus_slot *slot_iface;
sd_bus_slot *slot_busowner;
sd_bus_slot *slot_endpoint;
sd_event_source *role_defer;

struct ctx *ctx;
Expand Down Expand Up @@ -385,6 +386,7 @@ static const sd_bus_vtable bus_endpoint_obmc_vtable[];
static const sd_bus_vtable bus_endpoint_cc_vtable[];
static const sd_bus_vtable bus_endpoint_bridge[];
static const sd_bus_vtable bus_endpoint_uuid_vtable[];
static const sd_bus_vtable bus_link_endpoint_vtable[];

__attribute__((format(printf, 1, 2))) static void bug_warn(const char *fmt, ...)
{
Expand Down Expand Up @@ -1658,7 +1660,7 @@ static int mctp_ctrl_validate_response(struct mctp_ctrl_cmd *cmd,
{
struct mctp_ctrl_resp *rsp;

if (exp_size <= sizeof(*rsp)) {
if (exp_size < sizeof(*rsp)) {
warnx("invalid expected response size!");
return -EINVAL;
}
Expand Down Expand Up @@ -4017,6 +4019,70 @@ static int method_register_vdm_type_support(sd_bus_message *call, void *data,
return rc;
}

static int method_attempt_discovery_notify(sd_bus_message *call, void *data,
sd_bus_error *berr)
{
struct mctp_ctrl_resp_discovery_notify *resp = NULL;
struct mctp_ctrl_cmd_discovery_notify req = { 0 };
dest_phys desti = { 0 }, *dest = &desti;
struct mctp_ctrl_cmd cmd = { 0 };
struct link *link = data;
uint8_t iid;
int rc;

struct ctx *ctx = link->ctx;

dest->ifindex = link->ifindex;
if (dest->ifindex <= 0)
return sd_bus_error_setf(berr, SD_BUS_ERROR_INVALID_ARGS,
"Unknown MCTP interface");

rc = message_read_hwaddr(call, dest);
if (rc < 0) {
set_berr(ctx, rc, berr);
return rc;
}

rc = validate_dest_phys(ctx, dest);
if (rc < 0)
return sd_bus_error_setf(berr, SD_BUS_ERROR_INVALID_ARGS,
"Bad physaddr");

iid = mctp_next_iid(ctx);
mctp_ctrl_msg_hdr_init_req(&req.ctrl_hdr, iid,
MCTP_CTRL_CMD_DISCOVERY_NOTIFY);
mctp_ctrl_cmd_init_from_req_type(&cmd, req);

rc = endpoint_query_phys(ctx, dest, &cmd);
if (rc < 0)
goto err;

rc = mctp_ctrl_validate_response(&cmd, sizeof(*resp),
dest_phys_tostr(dest), iid,
MCTP_CTRL_CMD_DISCOVERY_NOTIFY);
if (rc)
goto err;

mctp_ctrl_cmd_free(&cmd);
return sd_bus_reply_method_return(call, "");
err:
mctp_ctrl_cmd_free(&cmd);
set_berr(ctx, rc, berr);

@jk-ozlabs jk-ozlabs Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the significance of returning an error in the API here? Do we need to fail this at all?

@jk-ozlabs jk-ozlabs Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to be clear: yes, we should fail on invalid arguments etc, but do we need to fail on specifics of the peer response - or lack of one?)

@msnidhin msnidhin Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Intention was to retry on failure. Like if BO sends ERR_NOT_READY CC or it is not ready to accept due to some other reasons.

@jk-ozlabs jk-ozlabs Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's requiring that caller to implement too much of the control protocol semantics. Errors / retrying should be handled by mctpd.

return rc;
}

// clang-format off
static const sd_bus_vtable bus_link_endpoint_vtable[] = {
SD_BUS_VTABLE_START(0),
SD_BUS_METHOD_WITH_ARGS("AttemptDiscoveryNotify",
SD_BUS_ARGS("ay", physaddr),
SD_BUS_NO_RESULT,
method_attempt_discovery_notify,
0),
SD_BUS_VTABLE_END,
};
// clang-format on

// clang-format off
static const sd_bus_vtable bus_link_owner_vtable[] = {
SD_BUS_VTABLE_START(0),
Expand Down Expand Up @@ -4214,14 +4280,21 @@ static int link_set_role(sd_event_source *ev, void *userdata)
sd_event_source_unref(link->role_defer);
link->role_defer = NULL;

if (link->role != ENDPOINT_ROLE_BUS_OWNER)
return 0;

rc = sd_bus_add_object_vtable(link->ctx->bus, &link->slot_busowner,
link->path, CC_MCTP_DBUS_IFACE_BUSOWNER,
bus_link_owner_vtable, link);
if (rc)
warnx("adding link owner vtable failed: %d", rc);
if (link->role == ENDPOINT_ROLE_BUS_OWNER) {
rc = sd_bus_add_object_vtable(link->ctx->bus,
&link->slot_busowner, link->path,
CC_MCTP_DBUS_IFACE_BUSOWNER,
bus_link_owner_vtable, link);
if (rc)
warnx("adding link owner vtable failed: %d", rc);
} else if (link->role == ENDPOINT_ROLE_ENDPOINT) {
rc = sd_bus_add_object_vtable(link->ctx->bus,
&link->slot_endpoint, link->path,
CC_MCTP_DBUS_IFACE_ENDPOINT,
bus_link_endpoint_vtable, link);
if (rc)
warnx("adding link endpoint vtable failed: %d", rc);
}

return 0;
}
Expand Down Expand Up @@ -4712,6 +4785,7 @@ static void free_link(struct link *link)
sd_event_source_disable_unref(link->role_defer);
sd_bus_slot_unref(link->slot_iface);
sd_bus_slot_unref(link->slot_busowner);
sd_bus_slot_unref(link->slot_endpoint);
free(link->path);
free(link->sysfs_path);
free(link);
Expand Down Expand Up @@ -4781,6 +4855,8 @@ static int rename_interface(struct ctx *ctx, struct link *link, int ifindex)
link->slot_iface = NULL;
sd_bus_slot_unref(link->slot_busowner);
link->slot_busowner = NULL;
sd_bus_slot_unref(link->slot_endpoint);
link->slot_endpoint = NULL;
free(link->path);

/* set new path and re-add */
Expand All @@ -4794,6 +4870,11 @@ static int rename_interface(struct ctx *ctx, struct link *link, int ifindex)
link->path,
CC_MCTP_DBUS_IFACE_BUSOWNER,
bus_link_owner_vtable, link);
} else if (link->role == ENDPOINT_ROLE_ENDPOINT) {
sd_bus_add_object_vtable(link->ctx->bus, &link->slot_endpoint,
link->path,
CC_MCTP_DBUS_IFACE_ENDPOINT,
bus_link_endpoint_vtable, link);
}

emit_interface_added(link);
Expand Down Expand Up @@ -5167,6 +5248,11 @@ static int add_interface(struct ctx *ctx, int ifindex)
link->path,
CC_MCTP_DBUS_IFACE_BUSOWNER,
bus_link_owner_vtable, link);
} else if (link->role == ENDPOINT_ROLE_ENDPOINT) {
sd_bus_add_object_vtable(link->ctx->bus, &link->slot_endpoint,
link->path,
CC_MCTP_DBUS_IFACE_ENDPOINT,
bus_link_endpoint_vtable, link);
}

if (link->phys_binding == MCTP_PHYS_BINDING_PCIE_VDM) {
Expand Down
8 changes: 8 additions & 0 deletions tests/mctp_test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@ async def mctpd_mctp_iface_control_obj(dbus, iface):
return await obj.get_interface("au.com.codeconstruct.MCTP.Interface1")


async def mctpd_mctp_iface_endpoint_obj(dbus, iface):
obj = await dbus.get_proxy_object(
'au.com.codeconstruct.MCTP1',
'/au/com/codeconstruct/mctp1/interfaces/' + iface.name,
)
return await obj.get_interface('au.com.codeconstruct.MCTP.Endpoint1')


async def mctpd_mctp_endpoint_obj(dbus, path, iface):
obj = await dbus.get_proxy_object(
'au.com.codeconstruct.MCTP1',
Expand Down
5 changes: 5 additions & 0 deletions tests/mctpenv/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,11 @@ async def handle_mctp_control(self, sock, addr, data):
)
await sock.send(raddr, data)

elif opcode == 0x0D:
# Discovery Notify — respond with completion_code = 0
data = bytes(hdr + [0x00])
await sock.send(raddr, data)

else:
await sock.send(
raddr, bytes(hdr + [0x05])
Expand Down
11 changes: 11 additions & 0 deletions tests/test_mctpd_endpoint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import asyncdbus
from mctp_test_utils import (
mctpd_mctp_iface_control_obj,
mctpd_mctp_iface_endpoint_obj,
mctpd_mctp_endpoint_control_obj,
)
from mctpenv import (
Expand Down Expand Up @@ -218,3 +219,13 @@ async def test_simple(self, dbus, mctpd):
mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x0B)
)
assert rsp.hex(' ') == '00 0b 05'


async def test_attempt_discovery_notify(dbus, mctpd):
"""AttemptDiscoveryNotify sends Discovery Notify to the bus owner."""
iface = mctpd.system.interfaces[0]
bo = mctpd.network.endpoints[0]

ep = await mctpd_mctp_iface_endpoint_obj(dbus, iface)
# mock bus owner responds with completion_code = 0x00
await ep.call_attempt_discovery_notify(bo.lladdr)
Loading