Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of mdio access using sai switch api and unix socket IPC s… #1071

Closed
wants to merge 1 commit into from
Closed

Add support of mdio access using sai switch api and unix socket IPC s… #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ AX_CODE_COVERAGE
AX_ADD_AM_MACRO_STATIC([])

AM_CONDITIONAL(SONIC_ASIC_PLATFORM_BAREFOOT, test x$CONFIGURED_PLATFORM = xbarefoot)
AM_CONDITIONAL(SONIC_ASIC_PLATFORM_BROADCOM, test x$CONFIGURED_PLATFORM = xbroadcom)

AC_ARG_ENABLE(debug,
[ --enable-debug turn on debugging],
Expand Down
10 changes: 10 additions & 0 deletions syncd/GlobalSwitchId.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ sai_object_id_t gSwitchId = SAI_NULL_OBJECT_ID;

#endif

#ifdef MDIO_ACCESS_USE_NPU
sai_object_id_t mdioSwitchId = SAI_NULL_OBJECT_ID;
#endif

Comment on lines +18 to +21
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current logic in syncd is able to handle multiple switches and it should be treated as such

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this mdioSwitchId needs to be passed to this class that you will create in the constructor, then remove global definition

void GlobalSwitchId::setSwitchId(
_In_ sai_object_id_t switchRid)
{
Expand All @@ -33,4 +37,10 @@ void GlobalSwitchId::setSwitchId(
sai_serialize_object_id(gSwitchId).c_str());
#endif

#ifdef MDIO_ACCESS_USE_NPU
if (mdioSwitchId == SAI_NULL_OBJECT_ID)
{
mdioSwitchId = switchRid;
}
#endif
Comment on lines +40 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this mdioSwitchId needs to be passed to this class that you will create in the constructor, then remove global definition

}
5 changes: 5 additions & 0 deletions syncd/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ libSyncd_a_SOURCES = \
WatchdogScope.cpp \
Workaround.cpp \
ZeroMQNotificationProducer.cpp \
syncd_mdio_ipc.cpp \
syncd_main.cpp

libSyncd_a_CPPFLAGS = $(CODE_COVERAGE_CPPFLAGS)
Expand All @@ -71,6 +72,10 @@ syncd_CXXFLAGS += -DSAITHRIFT=yes
syncd_LDADD += -lrpcserver -lthrift
endif

if SONIC_ASIC_PLATFORM_BROADCOM
libSyncd_a_CXXFLAGS += -DMDIO_ACCESS_USE_NPU
endif

libSyncdRequestShutdown_a_SOURCES = \
RequestShutdown.cpp \
RequestShutdownCommandLineOptions.cpp \
Expand Down
19 changes: 19 additions & 0 deletions syncd/Syncd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those functions needs to be object methods

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can pass std::shared_ptr as api

extern void stopIpcMdioProcessingThread(void);
#endif

Comment on lines +4475 to +4479
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this to separate file
also why those 2 functions needs to be explicitly called instead on internals SAI when calling create_switch ?

void Syncd::run()
{
SWSS_LOG_ENTER();
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need explicit context context zero here?

{
startIpcMdioProcessingThread(sai_metadata_sai_switch_api);
}
#endif

SWSS_LOG_NOTICE("syncd listening for events");

s->addSelectable(m_selectableChannel.get());
Expand Down Expand Up @@ -4678,6 +4690,13 @@ void Syncd::run()

m_manager->removeAllCounters();

#ifdef MDIO_ACCESS_USE_NPU
if (m_commandLineOptions->m_globalContext == 0)
{
stopIpcMdioProcessingThread();
}
#endif

sai_status_t status = removeAllSwitches();

// Stop notification thread after removing switch
Expand Down
Loading