mctpd: Add AttemptDiscoveryNotify D-Bus method for endpoint role#159
mctpd: Add AttemptDiscoveryNotify D-Bus method for endpoint role#159msnidhin wants to merge 1 commit into
Conversation
msnidhin
force-pushed
the
discovery_notify
branch
2 times, most recently
from
6036f4f to
d5934f4
Compare
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
The implementation looks good here (some fairly minor comments inline) but there are some items that I don't think we had covered on the earlier discussion.
As a naïve approach, we could do the Discovery Notify automatically when we're bringing-up an interface that is one of the Discovery-Notify-requiring types. However, that's not always possible, because:
- we don't always know when that is required (as DSP0233 specifies)
- we don't always know how to address the bus owner
Could you elaborate on your use-case here to see how we should be handling the former? If it's just a matter of knowing the bus-owner address, this could possibly be designed as more of an interface to supply the bus-owner address, and then we would decide on sending the Discovery Notify as appropriate. If that's not feasible, that's fine, but I would like to know the factors involved there.
| 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. |
There was a problem hiding this comment.
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?
| .AttemptDiscoveryNotify method ay - - | ||
| ``` | ||
|
|
||
| #### `.AttemptDiscoveryNotify`: `ay` |
There was a problem hiding this comment.
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?
| struct mctp_ctrl_resp *rsp; | ||
|
|
||
| if (exp_size <= sizeof(*rsp)) { | ||
| if (exp_size < sizeof(*rsp)) { |
There was a problem hiding this comment.
I'd prefer this as a separate fix.
| struct mctp_ctrl_cmd_discovery_notify req = { 0 }; | ||
| struct mctp_ctrl_resp_discovery_notify *resp = NULL; | ||
| dest_phys desti = { 0 }, *dest = &desti; | ||
| struct link *link = data; | ||
| struct ctx *ctx = link->ctx; | ||
| struct mctp_ctrl_cmd cmd = { 0 }; | ||
| uint8_t iid; | ||
| int rc; |
There was a problem hiding this comment.
Minor, but if you're reworking, can you reverse-christmas-tree these?
|
|
||
| rc = message_read_hwaddr(call, dest); | ||
| if (rc < 0) | ||
| goto err; |
There was a problem hiding this comment.
This is taking the mctp_ctrl_cmd_free path, but the surrounding conditionals do not. You don't have cmd->resp set here, so I don't think you need the cleanup.
| return sd_bus_reply_method_return(call, ""); | ||
| err: | ||
| mctp_ctrl_cmd_free(&cmd); | ||
| set_berr(ctx, rc, berr); |
There was a problem hiding this comment.
What's the significance of returning an error in the API here? Do we need to fail this at all?
There was a problem hiding this comment.
(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?)
|
|
||
| Bridge endpoints should be initialised with `AssignEndpoint` instead. | ||
|
|
||
| ### Endpoint interface: `au.com.codeconstruct.MCTP.Endpoint1` interface (on interface objects) |
There was a problem hiding this comment.
drop the "(on interface objects)"; this is covered by the "Endpoint Interface" term there (it's a bit confusing on the two meanings of "interface" here, but I don't think a third reference helps).
Expose AttemptDiscoveryNotify on au.com.codeconstruct.MCTP.Endpoint1 on the per-interface D-Bus path when the link role is ENDPOINT_ROLE_ENDPOINT. The method accepts a physical hardware address and sends a physical-addressed Discovery Notify control message (opcode 0x0D) to the target, allowing an endpoint to proactively trigger discovery by the bus owner. Also fix mctp_ctrl_validate_response to use strict less-than when comparing exp_size against sizeof(struct mctp_ctrl_resp). The previous <= incorrectly rejected responses whose complete structure is exactly the base response size, such as Discovery Notify. Signed-off-by: Nidhin MS <nidhin.ms@intel.com>
2 participants
Expose AttemptDiscoveryNotify on au.com.codeconstruct.MCTP.Endpoint1 on the per-interface D-Bus path when the link role is ENDPOINT_ROLE_ENDPOINT.
The method accepts a physical hardware address and sends a physical-addressed Discovery Notify control message (opcode 0x0D) to the target, allowing an endpoint to proactively trigger discovery by the bus owner.
Also fix mctp_ctrl_validate_response to use strict less-than when comparing exp_size against sizeof(struct mctp_ctrl_resp). The previous <= incorrectly rejected responses whose complete structure is exactly the base response size, such as Discovery Notify.