mctpd: add AssignBridgeStatic method#148
Conversation
|
I'm going to need more context than that commit message. What is the aim here? |
src/mctpd.c
Outdated
| 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
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 5 31
yisb 30 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>
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?
|
|
||
| peer->pool_size = pool_size; | ||
| if (peer->pool_size) | ||
| peer->pool_start = pool_start; |
There was a problem hiding this comment.
How are you ensuring that this does not conflict with existing allocations?
Also, check that pool_start == static_eid + 1 here, in order to be compliant with the spec.
| different from `static-EID`, or if `static-EID` is already assigned to another | ||
| endpoint. | ||
|
|
||
| #### `.AssignBridgeStatic`: `ayy` → `yisb` |
There was a problem hiding this comment.
This argument signature does not match the implementation
| pool-start-EID argument: | ||
|
|
||
| ``` | ||
| AssignBridgeStatic <hwaddr> <static-EID> <pool-size> <pool-start-EID> |
There was a problem hiding this comment.
I think this may read a little better with the two EIDs in order:
AssignBridgeStatic <hwaddr> <static-EID> <pool-start-EID> <pool-size>
|
|
||
| `<pool-size>`: The number of EIDs to be allocated to the bridge's EID pool. | ||
|
|
||
| `<pool-start-EID>`: The starting EID for the range of EIDs being allocated. |
There was a problem hiding this comment.
We will need to document the requirement that this EID is contiguous with the bridge EID.
| } | ||
|
|
||
| // 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. |
2 participants
Add AssignBridgeStatic method to assign static EID with bridge pool allocation.
Tested:
Signed-off-by: Potin Lai potin.lai@quantatw.com