Move to libpldm APIs for Allocating Instance IDs and change Transport APIs #213
Conversation
|
Can one of the admins verify this patch? |
Lakshmi-y
force-pushed
the
libpldm-mctp
branch
3 times, most recently
from
eb2049a to
e580391
Compare
|
I created one commit for both instance ID API and transport API changes. |
|
Journalctl out: |
… APIs Replace the current APIs with libpldm APIs for allocating instance IDs directly. This eliminates the need for remote D-Bus calls to the pldm daemon. Replace pldm transport APIs with libpldm pldm_transport APIs. This change migrates the application off the deprecated "requester" APIs in libpldm. Since we currently lack the infrastructure to obtain the correct TIDs, use the EID as the TID in the EID-to-TID mapping to maintain the current functionality. Change-Id: I5854aa022582ceae6a1843983fc90181b7e6d55f Signed-off-by: Lakshmi Yadlapati <[email protected]>
-Added support for kernel MCTP (AF_MCTP) to enable MCTP communication using AF_MCTP sockets. - Introduced a new configuration option 'transport-implementation' The 'transport-implementation' option can be set to either: - 'mctp-demux': Uses the existing mctp-demux transport method. - 'af-mctp': Uses the new kernel AF_MCTP transport method. Change-Id: I6b6b4d85bfec4b6efa630448f0c51b71b1150efa Signed-off-by: Lakshmi Yadlapati <[email protected]>
| @@ -43,6 +60,19 @@ class PldmFramework | |||
| /** Host mctp eid */ | |||
| static constexpr auto mctpEid = (types::Byte)9; | |||
|
|
|||
| /** @brief Hardcoded TID */ | |||
| using TerminusID = uint8_t; | |||
There was a problem hiding this comment.
Please move this to types.hpp
| */ | ||
| int openPldm(); | ||
|
|
||
| /** @brief Opens the MCTP socket for sending and receiving messages. |
There was a problem hiding this comment.
Format: Move this to next line please.
| @@ -1,3 +1,4 @@ | |||
|
|
|||
There was a problem hiding this comment.
Please remove extra line.
| method.append(mctpEid); | ||
| auto reply = bus.call(method); | ||
| reply.read(instanceId); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
There was a problem hiding this comment.
Could you please add a comment about why this 100ms sleep is required? And how did we reach to this number?
| @@ -1,2 +1,8 @@ | |||
| option('tests', type: 'feature', value: 'enabled', description: 'Build tests.',) | |||
| option('system-vpd-dependency', type: 'feature', description: 'Enable/disable system vpd dependency.', value: 'disabled') | |||
| option( | |||
There was a problem hiding this comment.
So, which one needs to be used and based on what criteria is it decided?
| @@ -135,6 +142,11 @@ class PldmFramework | |||
| */ | |||
| int openMctpDemuxTransport(); | |||
|
|
|||
| /** @brief Opens the MCTP AF_MCTP for sending and receiving messages. | |||
| * | |||
There was a problem hiding this comment.
what is AF_MCTP? what it does? and why it is required here?
can you please add those details in doxygen.
| @@ -34,6 +34,12 @@ systemd_system_unit_dir = systemd.get_variable('systemdsystemunitdir') | |||
|
|
|||
| service_file = 'service_files/com.ibm.panel.service' | |||
|
|
|||
| if(get_option('transport-implementation')) == 'mctp-demux' | |||
| add_project_arguments('-DPLDM_TRANSPORT_WITH_MCTP_DEMUX', language : 'cpp') | |||
There was a problem hiding this comment.
can you please add a comment here on what does the options DPLDM_TRANSPORT_WITH_MCTP_DEMUX and DPLDM_TRANSPORT_WITH_AF_MCTP do?
| @@ -72,9 +81,10 @@ int PldmFramework::openPldm() | |||
| return fd; | |||
| } | |||
|
|
|||
| int PldmFramework::openMctpDemuxTransport() | |||
There was a problem hiding this comment.
you have created the line 75 in your first commit and edited it in second commit(in the same PR). can you please check if these changes between two commits are required?
Replace the current APIs with libpldm APIs for allocating instance IDs directly. This eliminates the need for remote D-Bus calls to the pldm daemon.
Replace pldm transport APIs with libpldm pldm_transport APIs. This change migrates the application off the deprecated "requester" APIs in libpldm. Since we currently lack the infrastructure to obtain the correct TIDs, use the EID as the TID in the EID-to-TID mapping to maintain the current functionality.
Change-Id: I5854aa022582ceae6a1843983fc90181b7e6d55f