Add support of mdio access using sai switch api and unix socket IPC s… #1071
Conversation
…erver Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>
|
|
| #ifdef MDIO_ACCESS_USE_NPU | ||
| sai_object_id_t mdioSwitchId = SAI_NULL_OBJECT_ID; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
why you suddenly need this instead of gSwitchId? also i strongly advise to not use global objects, this file is added only for thrift support
There was a problem hiding this comment.
current logic in syncd is able to handle multiple switches and it should be treated as such
There was a problem hiding this comment.
this mdioSwitchId needs to be passed to this class that you will create in the constructor, then remove global definition
| #ifdef MDIO_ACCESS_USE_NPU | ||
| extern int startIpcMdioProcessingThread(sai_switch_api_t *sai_switch_api); | ||
| extern void stopIpcMdioProcessingThread(void); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
move this to separate file
also why those 2 functions needs to be explicitly called instead on internals SAI when calling create_switch ?
| @@ -4494,6 +4499,13 @@ void Syncd::run() | |||
| // notification queue is created before we create switch | |||
| m_processor->startNotificationsProcessingThread(); | |||
|
|
|||
| #ifdef MDIO_ACCESS_USE_NPU | |||
| if (m_commandLineOptions->m_globalContext == 0) | |||
There was a problem hiding this comment.
do you need explicit context context zero here?
| @@ -4472,6 +4472,11 @@ static void timerWatchdogCallback( | |||
| SWSS_LOG_ERROR("main loop execution exceeded %ld ms", span/1000); | |||
| } | |||
|
|
|||
| #ifdef MDIO_ACCESS_USE_NPU | |||
| extern int startIpcMdioProcessingThread(sai_switch_api_t *sai_switch_api); | |||
There was a problem hiding this comment.
don'y pass sai_switch_api, since this suggest that you may call some create functions and it will get out of sync with redisdb
There was a problem hiding this comment.
those functions needs to be object methods
There was a problem hiding this comment.
you can pass std::shared_ptr as api
| @@ -0,0 +1,440 @@ | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
if you want to keep this file, make this to a proper class, and then instantiate object and call explicitly functions of that object
| } | ||
|
|
||
| /* Convert to lowercase string */ | ||
| static char *strlower(char *s) |
There was a problem hiding this comment.
use c++ functions instead of reinventing this
| #if 0 | ||
| printf("CMD: "); | ||
| for (j = 0; j < argc; ++j) | ||
| printf("[%s],", argv[j]); | ||
| printf("\n"); | ||
| #endif |
| taskAlive = 0; | ||
| pthread_join(taskId, (void **)&err); | ||
| SWSS_LOG_NOTICE("IPC task thread is stopped\n"); | ||
| return; |
| }; | ||
|
|
||
| static void *syncd_ipc_task_main(void *ctx) | ||
| { |
There was a problem hiding this comment.
all functions are missing SWSS_LOG_ENTER()
| else | ||
| { | ||
| ret = sai_switch_api->switch_mdio_read(mdioSwitchId, mdio_addr, reg_addr, 1, &val); | ||
| sprintf(resp, "%d 0x%x\n", ret, val); |
There was a problem hiding this comment.
output to a unknown buffer size out the function scope, use std::string to produce response or log errors here instead of passing it through the stack
| static sai_status_t syncd_ipc_cmd_mdio_common(char *resp, int argc, char *argv[], sai_switch_api_t *sai_switch_api) | ||
| { | ||
| int ret = 0; | ||
| uint32_t mdio_addr = 0, reg_addr = 0, val = 0; |
There was a problem hiding this comment.
declare 1 variable perl line
There was a problem hiding this comment.
I am rewriting the code in c++ class. I will try to address most of the comments in new code.
|
This PR is replaced by #1080 |
…erver
Signed-off-by: Jiahua Wang jiahua.wang@broadcom.com