Skip to content

mctpd: Automatic Discovery Notify 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
Open

mctpd: Automatic Discovery Notify 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
47 changes: 47 additions & 0 deletions docs/mctpd.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,53 @@ 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 - - -
.BusOwner property ay - writable
```

#### `.BusOwner`: `ay`

Sets the physical address of the bus owner for this interface. For transport
types that know the bus owner physaddr (or do not need one to address the bus
owner), this would be populated automatically by the transport binding. For

@jk-ozlabs jk-ozlabs Jul 20, 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 sounds like it should be a (writable) property then? Should clients be able to query the current bus owner value?

@msnidhin msnidhin Jul 27, 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.

Changed to property.
I think getter is needed for non-basic type properties

@jk-ozlabs jk-ozlabs Jul 27, 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 looks good for the interface.

And yes, all good with the explicit get & set methods.

@jk-ozlabs jk-ozlabs Aug 17, 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.

One thought here: what's the API intention for when no bus owner is set? and how do we distinguish that from cases where there is no physical addressing mechanism (eg, USB & serial transports).

We should probably document that here too.

@msnidhin msnidhin Aug 31, 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. I see only I3C and PCIe spec mention abount sending discovery notify to announce endpoint presence.

can I add to doc like this ?
For transport types that do not use physical addressing for bus owner communication (such as USB and serial), this property has no effect; Discovery Notify is only triggered on transports that require it (PCIe VDM and I3C).

@msnidhin msnidhin Sep 18, 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.

Updated docs..

other cases, this property allows the address to be supplied by an external
management application.

On transport types that require a Discovery Notify (PCIe and I3C), setting the
bus owner address will automatically trigger a Discovery Notify to the bus
owner if the interface is in Endpoint role and not yet discovered.

For transport types that do not use physical addressing for bus owner
communication (such as USB and serial), this property has no effect;
Discovery Notify is only triggered on transports that require it (PCIe VDM
and I3C).

`BusOwner <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, setting the bus owner's 6-byte PID
`00 6c 90 01 23 45`:

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

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

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

/* Physical address of the bus owner. */
dest_phys bus_owner;

struct ctx *ctx;
};

Expand Down Expand Up @@ -389,6 +393,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 @@ -4255,6 +4260,138 @@ static int method_register_vdm_type_support(sd_bus_message *call, void *data,
return rc;
}

static bool link_supports_discovery_notify(const struct link *link)
{
return (link->phys_binding == MCTP_PHYS_BINDING_PCIE_VDM ||
link->phys_binding == MCTP_PHYS_BINDING_I3C) &&
link->role == ENDPOINT_ROLE_ENDPOINT &&
link->discovered != DISCOVERY_DISCOVERED;

@jk-ozlabs jk-ozlabs Nov 2, 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.

Firly minor, but: this last conditional doesn't reflect the name of the function; the link supports discovery notify regardless of whether it has been discovered.

I would suggest doing the link->discovered check in link_attempt_discover_notify() instead.

}

static void link_attempt_discovery_notify(struct link *link)
{
struct mctp_ctrl_resp_discovery_notify *resp = NULL;
struct mctp_ctrl_cmd_discovery_notify req = { 0 };
struct mctp_ctrl_cmd cmd = { 0 };
struct ctx *ctx = link->ctx;
unsigned int retry;
uint8_t iid;
int rc;

if (!link_supports_discovery_notify(link))
return;

for (retry = 0; retry < 4; retry++) {
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, &link->bus_owner, &cmd);
if (rc < 0) {
mctp_ctrl_cmd_free(&cmd);
break;
}

rc = mctp_ctrl_validate_response(
&cmd, sizeof(*resp), dest_phys_tostr(&link->bus_owner),
iid, MCTP_CTRL_CMD_DISCOVERY_NOTIFY);
mctp_ctrl_cmd_free(&cmd);
if (rc == 0)
return;
/* retry on non-fatal completion code errors */
if (rc != -EBUSY)

@jk-ozlabs jk-ozlabs Nov 2, 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.

mctp_ctrl_validate_response cannot return EBUSY. What is the intention here?

break;
}

if (rc < 0)
warnx("Discovery Notify on %s failed: %s", link->path,
strerror(-rc));
}

static int bus_link_endpoint_get_prop(sd_bus *bus, const char *path,
const char *interface,
const char *property,
sd_bus_message *reply, void *userdata,
sd_bus_error *berr)
{
struct link *link = userdata;
int rc;

if (strcmp(property, "BusOwner") == 0) {
rc = sd_bus_message_append_array(reply, 'y',
link->bus_owner.hwaddr,
link->bus_owner.hwaddr_len);
} else {
warnx("Unknown property '%s' for %s iface %s", property, path,
interface);
rc = -ENOENT;
}

return rc;
}

static int bus_link_endpoint_set_prop(sd_bus *bus, const char *path,
const char *interface,
const char *property,
sd_bus_message *value, void *userdata,
sd_bus_error *berr)
{
struct link *link = userdata;
struct ctx *ctx = link->ctx;
dest_phys dest = { 0 };
int rc;

if (strcmp(property, "BusOwner") != 0) {
warnx("Unknown property '%s' for %s iface %s", property, path,
interface);
rc = -ENOENT;
goto out;
}

dest.ifindex = link->ifindex;
if (dest.ifindex <= 0) {
sd_bus_error_setf(berr, SD_BUS_ERROR_INVALID_ARGS,
"Unknown MCTP interface");
rc = -EINVAL;
goto out;
}

rc = message_read_hwaddr(value, &dest);
if (rc < 0)
goto out;

rc = validate_dest_phys(ctx, &dest);
if (rc < 0) {
sd_bus_error_setf(berr, SD_BUS_ERROR_INVALID_ARGS,
"Bad physaddr");
goto out;
}

link->bus_owner = dest;
if (link->ctx->verbose)
warnx("BusOwner for %s set to %s", link->path,
dest_phys_tostr(&link->bus_owner));
link_attempt_discovery_notify(link);
rc = 0;
out:
set_berr(ctx, rc, berr);
return rc;
}

// clang-format off
static const sd_bus_vtable bus_link_endpoint_vtable[] = {
SD_BUS_VTABLE_START(0),
SD_BUS_WRITABLE_PROPERTY("BusOwner",
"ay",
bus_link_endpoint_get_prop,
bus_link_endpoint_set_prop,
0,
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 @@ -4467,14 +4604,22 @@ 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);
link_attempt_discovery_notify(link);
}

return 0;
}
Expand Down Expand Up @@ -4965,6 +5110,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 @@ -5034,6 +5180,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 @@ -5047,6 +5195,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 @@ -5420,6 +5573,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 All @@ -5432,6 +5590,8 @@ static int add_interface(struct ctx *ctx, int ifindex)
link->published = false;
}

link_attempt_discovery_notify(link);

return rc;

err_free:
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
7 changes: 7 additions & 0 deletions tests/mctpenv/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ def __init__(

# keyed by (type, type-specific-instance)
self.commands = {}
self.discovered_devices = []

@jk-ozlabs jk-ozlabs Nov 2, 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.

should this be a python set instead? using a list means that checks like you have:

    assert bo.discovered_devices == [iface.lladdr]

will fail after multiple discoveries. With a set, the discover becomes idempotent:

    assert iface.lladdr in bo.discovered_devices


def __str__(self):
return f"uuid {self.uuid} lladdr {self.lladdr}, eid {self.eid}"
Expand Down Expand Up @@ -537,6 +538,12 @@ async def handle_mctp_control(self, sock, addr, data):
)
await sock.send(raddr, data)

elif opcode == 0x0D:
self.discovered_devices.append(self.iface.lladdr)
# 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
Loading
Loading