Add mdio IPC client library by jiahua-wang · Pull Request #1134 · sonic-net/sonic-sairedis

jiahua-wang · 2022-09-28T23:54:21Z

Signed-off-by: Jiahua Wang jiahua.wang@broadcom.com
This PR is to replace sonic-net/sonic-buildimage#11284

Why I did it
The syncd has a MDIO IPC server to help the access of external PHYs on the NPU MDIO bus. A MDIO IPC client library is required to communicate to the MDIO IPC server. The gbsyncd will pass the function pointers for the function in the client library to the PAI.

How I did it
MDIO IPC client functions are added to connect to the MDIO IPC server socket, to send request and to wait for response IPC messages. The client functions are made into a .so library. The .so library is packaged in the syncd debian package.

How to verify it
The client functions are also available in as a .a library. The unit test code can use the code to test both IPC server and client.

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

kcudnik · 2022-09-29T09:37:23Z

please add more description about this PR

jiahua-wang · 2022-09-29T22:20:16Z

please add more description about this PR

Description added

jimmyzhai · 2022-10-10T08:05:31Z

syncd/MdioIpcClient.cpp

+
+#include "MdioIpcClient.h"
+
+#define SAI_IPC_SOCK_DIR_SYNCD      "/var/run/sswsyncd"


Same macros have been defined in MdioIpcServer.cpp, better to put them in a shared head file.

will do that

jimmyzhai · 2022-10-10T08:10:18Z

syncd/MdioIpcClient.cpp

+        fd = open(path, O_DIRECTORY);
+        if (fd < 0)
+        {
+            printf("%s: Program is not run on host\n", __func__);


Use SWSS_LOG_XXX instead of printf

will do that

jimmyzhai · 2022-10-10T08:28:07Z

syncd/MdioIpcClient.cpp

+
+#define CONN_TIMEOUT                25     /* shorter than 30 sec on server side */
+
+static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;


better to use c++ std::mutex

Will change code to use std::mutex

jimmyzhai · 2022-10-10T08:57:43Z

syncd/MdioIpcClient.cpp

+
+    if (number_of_registers > 1)
+    {
+        printf("Multiple register reads are not supported, num_of_registers: %d\n", number_of_registers);


Why not support read/write of multiple registers?

This is an error check for multiple reasons. The caller of the mdio ipc client is the PAI. The mdio read function prototype defined for PAI has already determined the mdio read single register. The mdio ipc server also determines the mdio read/write single register. The IPC message format also determines the mdio access single register.

jimmyzhai · 2022-10-10T09:10:48Z

syncd/MdioIpcClient.cpp

+    {
+        *data = (uint32_t)strtoul(strchrnul(resp, ' ') + 1, NULL, 0);
+        rc = SAI_STATUS_SUCCESS;
+    }


If syncd_mdio_ipc_command(cmd, resp) < 0, for debugging, should at least add log for error code. Better to translate the error code to SAI status code on rc.

Will add error log as the else condition statement.

kcudnik · 2022-10-10T11:39:36Z

you should add unittests for this code

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

kcudnik · 2022-10-24T15:22:23Z

please add unittests to satisfy code coverage

jiahua-wang · 2022-10-24T17:14:55Z

I am working on the unit test.

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

…d unittest Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jiahua-wang · 2023-03-29T22:00:43Z

/azpw run Azure.sonic-sairedis

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

kcudnik · 2023-04-06T06:04:08Z

syncd/Makefile.am

 libSyncd_a_CXXFLAGS += -DMDIO_ACCESS_USE_NPU
 endif

+if SONIC_ASIC_PLATFORM_VS


you are changind default behaviour by this, since by default vs switch is not considered as npu switch

Because some code in MdioIpcServer.cpp is compiled with the condition MDIO_ACCESS_USE_NPU and seems not run in unit test, I tried to add the condition to the vs platform but it did not work. I will revert the change since it does not help the unit test.

…ORM_UNDEFINED, remove SONIC_ASIC_PLATFORM_VS Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

… add -DMDIO_ACCESS_USE_NPU to CXXFLAGS in azure_pipeline Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

kcudnik · 2023-04-11T17:35:19Z

syncd/MdioIpcCommon.h

+#define MDIO_SERVER_TIMEOUT   30     /* sec, connection timeout */
+#define MDIO_CLIENT_TIMEOUT   25     /* shorter than 30 sec on server side */
+
+#define CONN_MAX              18     /* max. number of connections */


prefix this by MDIO_

Will change CONN_MAX to MDIO_CONN_MAX.

…tIpcTestMode() Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jimmyzhai · 2023-04-19T12:06:41Z

syncd/MdioIpcServer.cpp

    /* globalContext == 0 for syncd, globalContext > 0 for gbsyncd */
    MdioIpcServer::m_syncdContext = (globalContext == 0);
+#ifdef MDIO_ACCESS_USE_NPU
+    MdioIpcServer::m_accessUseNPU = true;


Why not directly m_accessUseNPU = true ? It is not defined as a static class member.

Since member m_accessUseNPU is added, macro MDIO_ACCESS_USE_NPU looks redundant and needs to be removed. Its value can be assigned by adding a parameter like accessUseNPU in the constructor.

MDIO_ACCESS_USE_NPU is only defined in syncd/Malefile.am for "make configure PLATFORM=broadcom". It is only tested on broadcom ASIC. When other platforms have tested the feature, the syncd/Makefile.am can be modified to add the feature. Actually, if m_accessUseNPU = true for vs platform, we see vssyncd test logs error messages.

The value of m_accessUseNPU has indicated feature enablement, which could be TRUE if

platform support, currently only broadcom

mock MdioIpcServer instance in unittest

Yes, The value of m_accessUseNPU is TRUE if platform support, currently only broadcom. For mock MdioIpcServer instance in unittest, the value of m_accessUseNPU is changed to be true at run time by the function MdioIpcServer::setIpcTestMode().

kcudnik · 2023-04-19T13:51:06Z

syncd/MdioIpcClient.cpp

+}
+
+/* Function to read data using clause 22 from MDIO interface */
+extern "C" sai_status_t mdio_read_cl22(uint64_t platform_context, uint32_t mdio_addr, uint32_t reg_addr,


all those extern C functions should be declared in extern C in header file, and not explicitly here

which they are, so this extern should be redundant here

I can try to remove the extern C in .c file. I'm not convinced it's good practice to remove the extern "C" from the implementation file. I think it's better to be explicit in the implementation file.

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

linux-foundation-easycla · 2023-04-19T17:52:59Z

The committers listed above are authorized under a signed CLA.

✅ login: jiahua-wang / name: Jiahua Wang (1fd8a92, 1db84cf, 67715d2, 69f9d23, d95c2bb, 70462c7, 9da4562, c789f84, 39f8b59, fcde33b, 1937037, 042b5d6, 5f4e7bc, 8665f8c, e9f6483, cc08e19, 519cb6b, 3f820d7, 11d9882, f0d52f9, fce22a9, 4766672, 12c2718, fe907f7, 4c5a6e4, c175d8a, 118e10d, d34f7ff, abfd217, 84b8dbd, 4e18bb3, 51065d7, b3aa4a2, 9d1f7fe, 7f7c771, e3a33a0, 0afecec)
✅ login: web-flow / name: GitHub Web Flow (67715d2, 9da4562, c789f84)

jiahua-wang · 2023-04-19T18:02:12Z

/azpw run Azure.sonic-sairedis

mssonicbld · 2023-04-19T18:02:14Z

/AzurePipelines run Azure.sonic-sairedis

azure-pipelines · 2023-04-19T18:02:24Z

Azure Pipelines successfully started running 1 pipeline(s).

jiahua-wang · 2023-04-19T21:39:36Z

This PR is replaced by #1230

WillsonHG · 2023-04-20T14:32:38Z

/easycla

Add mdio IPC client library

1fd8a92

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jiahua-wang mentioned this pull request Sep 28, 2022

Add mdio IPC client library sonic-net/sonic-buildimage#11284

Closed

7 tasks

Add -Wno-format-truncation to CPPFLAGS

1db84cf

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jimmyzhai reviewed Oct 10, 2022

View reviewed changes

jiahua-wang added 4 commits October 11, 2022 10:38

Merge branch 'sonic-net:master' into ipc-client

67715d2

Add MdioIpcCommon.h and syncd.links

69f9d23

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add SWSS_LOG_ENTER() but disabled to pass swsslogentercheck.sh

d95c2bb

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add SWSS_LOG_ENTER() but disabled to pass swsslogentercheck.sh

70462c7

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jiahua-wang added 3 commits October 24, 2022 10:15

Merge branch 'sonic-net:master' into ipc-client

9da4562

Merge branch 'sonic-net:master' into ipc-client

c789f84

Merge branch 'master' into ipc-client

39f8b59

zhangyanzhao mentioned this pull request Mar 13, 2023

MDIO IPC Client Library sonic-net/SONiC#1281

Closed

Add unit test for MdioIpcServer class and MdioIpcClient code

fcde33b

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

kcudnik previously approved these changes Mar 16, 2023

View reviewed changes

jiahua-wang added 2 commits March 16, 2023 20:54

Merge branch 'master' into ipc-client

1937037

Add log message to check existence of directory /var/run/sswsyncd

042b5d6

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jiahua-wang dismissed kcudnik’s stale review via 042b5d6 March 16, 2023 21:53

jiahua-wang added 7 commits March 17, 2023 21:16

Change from TEST() to TEST_F()

5f4e7bc

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Merge branch 'master' into ipc-client

8665f8c

Create directory /var/run/sswsyncd with mode 777

e9f6483

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add CAP_DAC_OVERRIDE capability for system directory creation in sync…

cc08e19

…d unittest Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Not to create directory /var/run/sswsyncd

519cb6b

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Create directory /var/run/sswsyncd with mode 755

3f820d7

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add tests_LDFLAGS, add IPC capabilities

11d9882

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jiahua-wang added 4 commits March 30, 2023 19:01

Give enough time for the server thread to finish setting up socket

f0d52f9

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Call setSwitchId()

fce22a9

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Use TEST()

4766672

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add conditional flag SONIC_ASIC_PLATFORM_VS

12c2718

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

kcudnik reviewed Apr 6, 2023

View reviewed changes

jiahua-wang added 3 commits April 10, 2023 22:13

Add conditional flag SONIC_ASIC_PLATFORM_GENERIC and SONIC_ASIC_PLATF…

fe907f7

…ORM_UNDEFINED, remove SONIC_ASIC_PLATFORM_VS Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Remove SONIC_ASIC_PLATFORM_GENERIC and SONIC_ASIC_PLATFORM_UNDEFINED,…

4c5a6e4

… add -DMDIO_ACCESS_USE_NPU to CXXFLAGS in azure_pipeline Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Merge branch 'master' into ipc-client

c175d8a

kcudnik reviewed Apr 11, 2023

View reviewed changes

jiahua-wang added 10 commits April 12, 2023 00:16

Remove -DMDIO_ACCESS_USE_NPU in azure_pipeline, add MdioIpcServer::se…

118e10d

…tIpcTestMode() Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add NPU to dictionary

d34f7ff

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Change sleep 3 to sleep 1

abfd217

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

only use TEST_F

84b8dbd

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Combine 4 TEST() into 1

4e18bb3

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add negtive test

51065d7

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add ipc client timeout test

b3aa4a2

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add ipc connect fail test

9d1f7fe

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Wait for client timeout before another test

7f7c771

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

Add negative tests of more than one register access

e3a33a0

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jimmyzhai reviewed Apr 19, 2023

View reviewed changes

kcudnik reviewed Apr 19, 2023

View reviewed changes

Remove extern "C" from implementation file

0afecec

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>

jiahua-wang closed this Apr 19, 2023

jiahua-wang deleted the ipc-client branch April 24, 2023 20:44


		#include "MdioIpcClient.h"

		#define SAI_IPC_SOCK_DIR_SYNCD "/var/run/sswsyncd"


		#define CONN_TIMEOUT 25 /* shorter than 30 sec on server side */

		static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

Conversation

jiahua-wang commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kcudnik commented Sep 29, 2022

Uh oh!

jiahua-wang commented Sep 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcudnik commented Oct 10, 2022

Uh oh!

kcudnik commented Oct 24, 2022

Uh oh!

jiahua-wang commented Oct 24, 2022

Uh oh!

jiahua-wang commented Mar 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiahua-wang Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla bot commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiahua-wang commented Apr 19, 2023

Uh oh!

mssonicbld commented Apr 19, 2023

Uh oh!

azure-pipelines bot commented Apr 19, 2023

Uh oh!

jiahua-wang commented Apr 19, 2023

Uh oh!

WillsonHG commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jiahua-wang commented Sep 28, 2022 •

edited

Loading

jiahua-wang Apr 19, 2023 •

edited

Loading

linux-foundation-easycla bot commented Apr 19, 2023 •

edited

Loading