mctpd: add AssignBridgeStatic method#148
Conversation
|
I'm going to need more context than that commit message. What is the aim here? |
| if (static_eid) { | ||
| new_eid = static_eid; | ||
| } else { | ||
| new_eid = alloc.start; | ||
| } |
There was a problem hiding this comment.
You can't just redefine the allocated range like this, those EIDs at [static_eid, static_eid + alloc.extent] may not be available.
There was a problem hiding this comment.
Hi @jk-ozlabs,
Thank you for your suggestion in Discord.
I have updated the change to introduce a new method called AssignBridgeStatic.
potinlai
changed the title
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
A few things inline, and the commit message needs more explanation. Rather than a chunk of 'Tested' output, can you include some details about the use case and what is implemented here?
| } | ||
|
|
||
| // Return existing record. | ||
| peer_path = path_from_peer(peer); |
There was a problem hiding this comment.
Is this valid here, and is the check sufficient? The existing peer may not be the bridge you expect...
|
Also, could you add some test cases here? We want to check the successful call, but also conflicts with existing EID allocations. |
Hi @jk-ozlabs, Could you please advise on the correct way to run this test? |
|
The binary doesn't need to run on the BMC; the idea behind the test infrastructure is that the The Testing section in the README has some details, but you just run the tests locally using something like: $ python3 -m venv env
$ ./env/bin/pip install -r tests/requirements.txt
$ export PATH=$PWD/env/bin:$PATH
$ meson setup obj
$ ninja -C obj test |
test cases updated. |
|
Just traveling at the moment, but I'll take a look at the updates shortly. Thanks! |
The `AssignEndpointStatic` method does not support EID allocation for bridged endpoints. Switch to `AssignBridgeStatic` to allow the MCU to allocate EID bridge pool for downstream devices. This change depends on the following patch: [1] CodeConstruct/mctp#148 Change-Id: I4c42273928646ec76c2ec7ca4fb06e5aa0947c78 Signed-off-by: Potin Lai <potin.lai@quantatw.com>
|
There was a pending question above:
Could you give us some details for your use-case? particularly around the requirement for pool addressing. This will dictate what future changes might be applicable, and how we should structure the interface to suit those. |
There are 7 SMA devices in system, and each SMA has its own downstream devices.
Below is MCTP tree on my BMC. And below is introspect details of EID 30 . |
|
Thanks, but I was more after your current requirements around the EID allocation pool. Particularly: do you require the pool to be static? It will need to be at the moment, but future mctp spec versions will relax the requirement that the pool is contiguous with the bridge EID. In that case, the caller may choose a non-contiguous pool address, or mctpd may be able to choose a pool range that is free. The former is a bit tricky, as the caller has no idea whether the remote endpoint would support non-contiguous pools, so I suspect the latter option would be more popular. To accommodate that, I think we would want to allow the |
Yes, In my use case, I expected the pool is static arranged, so each downstream devices will have same EID assigned.
That’s a great point about allowing pool_start to be zero for mctpd-managed allocation.
I can add extra |
It is unlikely that they support non-contiguous pools, the spec change is quite recent. I am not proposing that we implement that at this stage - only that the APIs we introduce now would be able to support it. So, the only behaviour we need now is for the pool-start argument, if passed as zero, is set to the bridge-eid+1. Everything else can be a later change.
Sounds great. |
potinlai
force-pushed
the
main
branch
2 times, most recently
from
edf0504 to
6903064
Compare
Thank you for the feedback. |
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
The core code looks good, just a few comments on the doc and tests.
Also, could you rebase on top of current main? Just to make sure that we haven't created conflicts in the meantime.
| peer->eid, req_pool_size, peer->pool_size); | ||
| } else { | ||
| // peer will likely have requested less than the available range | ||
| peer->pool_size = req_pool_size; |
There was a problem hiding this comment.
Does this mean we should also return the pool size? Or would the caller not really care?
Add AssignBridgeStatic method to assign static EID with bridge pool allocation.
Tested:
```
root@bmc:~# busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/interfaces/mcu1u2u1u4u1 au.com.codeconstruct.MCTP.BusOwner1 AssignBridgeStatic ayyyy 0 30 31 5
yyisb 30 31 1 "/au/com/codeconstruct/mctp1/networks/1/endpoints/30" true
root@bmc:~# busctl introspect au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/networks/1/endpoints/30
NAME TYPE SIGNATURE RESULT/VALUE FLAGS
au.com.codeconstruct.MCTP.Bridge1 interface - - -
.PoolEnd property y 35 const
.PoolStart property y 31 const
au.com.codeconstruct.MCTP.Endpoint1 interface - - -
.Recover method - - -
.Remove method - - -
.SetMTU method u - -
.Connectivity property s "Available" emits-change
org.freedesktop.DBus.Introspectable interface - - -
.Introspect method - s -
org.freedesktop.DBus.Peer interface - - -
.GetMachineId method - s -
.Ping method - - -
org.freedesktop.DBus.Properties interface - - -
.Get method ss v -
.GetAll method s a{sv} -
.Set method ssv - -
.PropertiesChanged signal sa{sv}as - -
xyz.openbmc_project.Common.UUID interface - - -
.UUID property s "8b7e3d86-5fa5-298c-a9d6-a2d5d4baa6fa" const
xyz.openbmc_project.MCTP.Endpoint interface - - -
.EID property y 30 const
.NetworkId property u 1 const
.SupportedMessageTypes property ay 4 1 5 126 127 const
.VendorDefinedMessageTypes property a(yvq) 0 const
```
Signed-off-by: Potin Lai <potin.lai@quantatw.com>
Add test cases to verify successful static EID and pool assignment, zero pool start auto-allocation, and rejection on conflict with an existing EID allocation. Signed-off-by: Freddie Jheng <Freddie.Jheng@quantatw.com>
4 participants
Add AssignBridgeStatic method to assign static EID with bridge pool allocation.
Tested:
Signed-off-by: Potin Lai potin.lai@quantatw.com