mctpd: Send Discovery Notify on Endpoint role set #15
Conversation
|
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?) |
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 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 For example, is there a way to know where a link comes from, is it a EDIT: for the PCIe binding driver, I'm afraid it is not something I have the permission to share just yet. |
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 |
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?
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. |
I agree with above point, maybe we don't need public the
I'm asking my manager for this. Currently, we do internal review to make the clean code before create MR to kernel.
Agree. So We don't need support |
khangng-ampere
force-pushed
the
notify-discovery
branch
from
238125d to
a15115a
Compare
khangng-ampere
changed the title
NotifyDiscovery D-Bus method
|
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. |
|
Nice one. I'll take a look shortly. |
|
Hi @jk-ozlabs, do you have any concerns regarding this? |
|
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. |
| 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); |
There was a problem hiding this comment.
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.
| @@ -2954,6 +3000,16 @@ static int bus_link_set_prop(sd_bus *bus, | |||
| } | |||
| lmUserData->role = role.role; | |||
|
|
|||
| // 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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Normally, extended addressing is used to respond to requests. We can take the physical address len from the requester's sockaddr. However, when we want to send extended address on an interface, we do not have that information in linkmap. This stores interface address from netlink inside linkmap. Tested: Discovery Notify message is filled with correct address (see next commit) Signed-off-by: Khang Nguyen Duy <[email protected]>
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)
Signed-off-by: Khang Nguyen Duy <[email protected]>
khangng-ampere
force-pushed
the
notify-discovery
branch
from
a15115a to
73e2567
Compare
|
|
||
| dest->ifindex = ifindex; | ||
| mctp_nl_ifaddr_byindex(ctx->nl, dest->ifindex, &dest->hwaddr_len); | ||
| memset(dest->hwaddr, 0, sizeof dest->hwaddr); |
There was a problem hiding this comment.
A zero hwaddr is specific to PCIe, right? Do we also need to know the physical broadcast address for this link type?
There was a problem hiding this comment.
@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:00for "default route", which corresponds to Route-to-Root-Complex,0xFF:FFfor "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)
There was a problem hiding this comment.
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...
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.
(My Root Complex does not handle Discovery Notify message yet, but
can confirm that the message is sent)