Skip to content

mctpd: Support MCTP Discovery Notify command #165

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
chajasmine-bit wants to merge 1 commit into
base: main
Choose a base branch
from
+405 −5
Open

mctpd: Support MCTP Discovery Notify command #165

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
1 change: 1 addition & 0 deletions src/mctp-netlink.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,7 @@ int mctp_nl_route_add(struct mctp_nl *nl, uint8_t eid, unsigned int extent,
return -1;
}
msg.nh.nlmsg_type = RTM_NEWROUTE;
msg.nh.nlmsg_flags |= NLM_F_CREATE | NLM_F_REPLACE;

@jk-ozlabs jk-ozlabs Aug 7, 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 can't see why we need different flags for the route creation for the discovery notify path. If this is needed, could you explain this in the commit message, or in a comment here?


if (mtu != 0) {
/* Nested
Expand Down
268 changes: 265 additions & 3 deletions src/mctpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ enum discovery_state {
DISCOVERY_UNDISCOVERED,
};

#define PEER_FLAG_ASSIGN_PENDING (1 << 0)

struct discovery_notify_ctx {
struct ctx *ctx;
dest_phys phys;
mctp_eid_t existing_eid;
struct discovery_notify_ctx *next;
};

struct link {
enum discovery_state discovered;
bool published;
Expand All @@ -153,6 +162,9 @@ struct link {
sd_bus_slot *slot_busowner;
sd_event_source *role_defer;

uint64_t last_discovery_time_us;
uint32_t discovery_count;

struct ctx *ctx;
};

Expand Down Expand Up @@ -226,6 +238,7 @@ struct peer {
struct {
sd_event_source **sources;
} bridge_ep_poll;
uint32_t flags;

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

Why a new flag set for just one flag?

};

struct msg_type_support {
Expand Down Expand Up @@ -314,6 +327,7 @@ struct ctx {
// bus owner/bridge polling interval in usecs for
// checking endpoint's accessibility.
uint64_t endpoint_poll;
struct discovery_notify_ctx *pending_discoveries;

// interface configuration (from config file), to be matched and
// applied on new interface events
Expand Down Expand Up @@ -364,6 +378,10 @@ static int add_peer(struct ctx *ctx, const dest_phys *dest, mctp_eid_t eid,
static int add_peer_from_addr(struct ctx *ctx,
const struct sockaddr_mctp_ext *addr,
struct peer **ret_peer);
static int change_peer_eid(struct peer *peer, mctp_eid_t new_eid);
static int endpoint_assign_eid(struct ctx *ctx, sd_bus_error *berr,
const dest_phys *dest, struct peer **ret_peer,
mctp_eid_t static_eid, bool assign_bridge);
static int remove_peer(struct peer *peer);
static int remove_bridged_peers(struct peer *bridge);
static int query_peer_properties(struct peer *peer);
Expand Down Expand Up @@ -750,9 +768,15 @@ static int reply_message(struct ctx *ctx, int sd, const void *resp,

if (reply_addr.smctp_addr.s_addr == 0 ||
reply_addr.smctp_addr.s_addr == 0xff) {
bug_warn("reply_message can't take EID %d",
reply_addr.smctp_addr.s_addr);
return -EPROTO;
/* Validate physical address metadata before falling back to physical send */
if (addr && addr->smctp_ifindex > 0 &&
addr->smctp_halen <= sizeof(addr->smctp_haddr)) {
return reply_message_phys(ctx, sd, resp, resp_len,
addr);
}
warnx("reply_message: EID %d specified without valid physical address info",
reply_addr.smctp_addr.s_addr);
return -EINVAL;
}

len = mctp_ops.mctp.sendto(sd, resp, resp_len, 0,
Expand Down Expand Up @@ -1329,6 +1353,209 @@ handle_control_endpoint_discovery(struct ctx *ctx, int sd,
return reply_message_phys(ctx, sd, resp, sizeof(*resp), addr);
}

/*
* Rate limiting is evaluated per-link (interface) rather than per-physical-MAC address
* for performance and simplicity. On shared multi-drop buses (e.g. I2C/SMBus), a noisy
* endpoint could temporarily consume the 5 req/sec quota; healthy adjacent endpoints will
* naturally retransmit Discovery Notify upon timeout.
*/
Comment on lines +1356 to +1361

@jk-ozlabs jk-ozlabs Aug 7, 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 comment is more of an argument against a per-link rate-limit. Can you detail why per-link instead of per-endpoint? If it's just for simplicity of the implementation, then say so.

But is it more difficult to track per physaddr anyway?

static bool link_check_rate_limit(struct link *link_data)
{
struct timespec ts;
uint64_t now_us;

if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0)
return true;

now_us = (uint64_t)ts.tv_sec * 1000000ULL + ts.tv_nsec / 1000ULL;
if (now_us - link_data->last_discovery_time_us > 1000000ULL) {
link_data->last_discovery_time_us = now_us;
link_data->discovery_count = 0;
}

if (link_data->discovery_count >= 5) {
return false;
}

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

Minor, but please try to be consistent with your coding style (comparing to the if block on line 1367)

I have a slight preference for no-braces on single-statement blocks.


link_data->discovery_count++;
return true;
}

static bool is_discovery_pending(struct ctx *ctx, int ifindex,
const dest_phys *phys)
{
struct discovery_notify_ctx *d;
struct peer *peer;

peer = find_peer_by_phys(ctx, phys);
if (peer && (peer->flags & PEER_FLAG_ASSIGN_PENDING))
return true;

for (d = ctx->pending_discoveries; d; d = d->next) {
if (d->phys.ifindex == ifindex && match_phys(&d->phys, phys))
return true;
}
return false;
}

static void remove_pending_discovery(struct ctx *ctx,
struct discovery_notify_ctx *dctx)
{
struct discovery_notify_ctx **pp = &ctx->pending_discoveries;
while (*pp) {
if (*pp == dctx) {
*pp = dctx->next;
break;
}
pp = &(*pp)->next;
}
}

static int deferred_assign_eid_cb(sd_event_source *s, void *userdata)
{
struct discovery_notify_ctx *dctx = userdata;
struct peer *peer = NULL;
mctp_eid_t target_eid;
bool assign_bridge;
int rc;

(void)s; /* Floating event source unref is managed automatically by sd_event */

@jk-ozlabs jk-ozlabs Aug 7, 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 seems unnecessary?


peer = find_peer_by_phys(dctx->ctx, &dctx->phys);
target_eid = peer ? peer->eid : dctx->existing_eid;
assign_bridge =
peer ? (peer->pool_size > 0 || peer->slot_bridge != NULL) :
false;

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

You're using a bunch of separate ternary operations here, just consolidate to an if block.

In doing that, I think it would be worthwhile adding a comment about the semantics of the peer already being present, which is a bit of a mystery here.


rc = endpoint_assign_eid(dctx->ctx, NULL, &dctx->phys, &peer,
target_eid, assign_bridge);
Comment on lines +1430 to +1431

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

Is this sufficient? Are we sure that we should be re-assigning the old EID here?

As above: if we receive a Discovery Notify for a pre-existing peer, it seems that we have lost state synchronisation with that peer, which may be more like a recovery operation rather than a simple EID assignment. Your change doesn't really explain what we intend to do in that case.


/* Fallback to dynamic allocation if target EID collides (-EEXIST) */
if (rc == -EEXIST && target_eid != 0) {
if (dctx->ctx->verbose) {
warnx("EID %d occupied for %s, retrying dynamic EID allocation",
target_eid, dest_phys_tostr(&dctx->phys));
}
rc = endpoint_assign_eid(dctx->ctx, NULL, &dctx->phys, &peer, 0,
assign_bridge);
}

peer = find_peer_by_phys(dctx->ctx, &dctx->phys);
if (peer) {
peer->flags &= ~PEER_FLAG_ASSIGN_PENDING;
}

if (rc == 0) {
if (peer && peer->pool_size > 0) {
endpoint_allocate_eids(peer);
}
} else if (dctx->ctx->verbose) {
warnx("Deferred EID assignment failed for %s: %s",
dest_phys_tostr(&dctx->phys), strerror(-rc));
}

/* Keep pending context linked until execution completes to prevent deduplication races */

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

The wording of comment doesn't make sense here. Perhaps something like:

/* We have kept the context linked until this point, to prevent deduplication races, but
 * we are safe to unlink now.
 */

remove_pending_discovery(dctx->ctx, dctx);
free(dctx);
return 0;
}

static int handle_control_discovery_notify(struct ctx *ctx, int sd,
const struct sockaddr_mctp_ext *addr,
const uint8_t *buf,
const size_t buf_size)
{
struct mctp_ctrl_resp_discovery_notify respi = { 0 }, *resp = &respi;
struct discovery_notify_ctx *dctx = NULL;
struct mctp_ctrl_msg_hdr *req = NULL;
struct link *link_data;
struct peer *peer = NULL;
dest_phys phys = { 0 };
int rc;

if (buf_size < sizeof(*req)) {
warnx("short Discovery Notify message");
return -ENOMSG;
}
req = (void *)buf;

link_data = mctp_nl_get_link_userdata(ctx->nl, addr->smctp_ifindex);
if (!link_data) {
bug_warn("unconfigured interface %d", addr->smctp_ifindex);
return -ENOENT;
}

if (link_data->role != ENDPOINT_ROLE_BUS_OWNER) {
if (ctx->verbose) {
warnx("Ignoring Discovery Notify on interface %d: not a bus owner",
addr->smctp_ifindex);
}
return 0;
}

if (!link_check_rate_limit(link_data)) {
if (ctx->verbose) {
warnx("Discovery Notify rate limit exceeded on interface %d",
addr->smctp_ifindex);
}
return 0;
}

/* Respond immediately over physical socket to acknowledge Discovery Notify */
mctp_ctrl_msg_hdr_init_resp(&respi.ctrl_hdr, *req);
resp->completion_code = MCTP_CTRL_CC_SUCCESS;

rc = reply_message_phys(ctx, sd, resp, sizeof(*resp), addr);
if (rc < 0) {
warnx("Failed to send Discovery Notify response on ifindex %d: %s",
addr->smctp_ifindex, strerror(-rc));
return rc;
}

phys.ifindex = addr->smctp_ifindex;
phys.hwaddr_len = addr->smctp_halen;
memcpy(phys.hwaddr, addr->smctp_haddr, addr->smctp_halen);

/* Deduplicate incoming Discovery Notify for both existing and new physical endpoints */
if (is_discovery_pending(ctx, addr->smctp_ifindex, &phys)) {
return 0;
}

peer = find_peer_by_phys(ctx, &phys);
if (peer) {
peer->flags |= PEER_FLAG_ASSIGN_PENDING;
}

dctx = calloc(1, sizeof(*dctx));
if (!dctx) {
if (peer)
peer->flags &= ~PEER_FLAG_ASSIGN_PENDING;
return -ENOMEM;
}

dctx->ctx = ctx;
dctx->phys = phys;
dctx->existing_eid = peer ? peer->eid : 0;

dctx->next = ctx->pending_discoveries;
ctx->pending_discoveries = dctx;

if (sd_event_add_defer(ctx->event, NULL, deferred_assign_eid_cb, dctx) <

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

Your implementation keeps a list of deferred discovery operations, but then schedules a new callback event for each. Would it make more sense just to have one callback queued?

0) {
if (ctx->verbose) {
warnx("Failed to defer EID assignment event for %s",
dest_phys_tostr(&phys));
}
remove_pending_discovery(ctx, dctx);
if (peer)
peer->flags &= ~PEER_FLAG_ASSIGN_PENDING;
free(dctx);
return -ENOMEM;
}

return 0;
}

static int handle_control_unsupported(struct ctx *ctx, int sd,
const struct sockaddr_mctp_ext *addr,
const uint8_t *buf, const size_t buf_size)
Expand Down Expand Up @@ -1423,6 +1650,10 @@ static int cb_listen_control_msg(sd_event_source *s, int sd, uint32_t revents,
rc = handle_control_endpoint_discovery(ctx, sd, &addr, buf,
buf_size);
break;
case MCTP_CTRL_CMD_DISCOVERY_NOTIFY:
rc = handle_control_discovery_notify(ctx, sd, &addr, buf,
buf_size);
break;
default:
if (ctx->verbose) {
warnx("Ignoring unsupported command code 0x%02x",
Expand Down Expand Up @@ -2004,6 +2235,7 @@ static int add_peer(struct ctx *ctx, const dest_phys *dest, mctp_eid_t eid,
{
struct peer *peer, **tmp;
struct net *n;
int rc;

n = lookup_net(ctx, net);
if (!n) {
Expand All @@ -2025,6 +2257,26 @@ static int add_peer(struct ctx *ctx, const dest_phys *dest, mctp_eid_t eid,
if (!allow_bridged && is_eid_in_bridge_pool(n, ctx, eid))
return -EEXIST;

/* Re-use existing physical peer mapping safely via change_peer_eid */

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

How is this related to the discovery implementation?

if (!allow_bridged) {
peer = find_peer_by_phys(ctx, dest);
if (peer) {
if (peer->eid == eid) {
*ret_peer = peer;
return 0;
}
rc = change_peer_eid(peer, eid);
if (rc < 0) {
if (rc == -EEXIST)
warnx("add_peer: target EID %d already occupied by different peer",
eid);
return rc;
}
*ret_peer = peer;
return 0;
}
}

if (ctx->num_peers == MAX_PEER_SIZE)
return -ENOMEM;

Expand Down Expand Up @@ -2216,6 +2468,8 @@ static int remove_peer(struct peer *peer)

static void free_peers(struct ctx *ctx)
{
struct discovery_notify_ctx *dctx, *tmp_dctx;

for (size_t i = 0; i < ctx->num_peers; i++) {
struct peer *peer = ctx->peers[i];
free(peer->message_types);
Expand All @@ -2231,6 +2485,14 @@ static void free_peers(struct ctx *ctx)
}

free(ctx->peers);

dctx = ctx->pending_discoveries;
while (dctx) {
tmp_dctx = dctx->next;
free(dctx);
dctx = tmp_dctx;
}
ctx->pending_discoveries = NULL;
}

/* Returns -EEXIST if the new_eid is already used */
Expand Down
Loading
Loading