-
Notifications
You must be signed in to change notification settings - Fork 5
RDKB-60656 : Available memory check for firmware downloads #23
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.
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
Conversation
b0bed3b to
bb2a7d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds a memory availability check before initiating firmware downloads to prevent failures on devices with insufficient memory.
Changes:
- Adds a pre-download memory check using an external library function
can_proceed_fw_download() - Links against a new external library
-lfw_download_chkto provide memory checking functionality - Returns failure with error logging when insufficient memory is detected
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| source/TR-181/integration_src.shared/cosa_device_info_apis.c | Added header include and memory check before firmware download thread creation |
| source/TR-181/integration_src.shared/Makefile.am | Added linker flag for new external library dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(can_proceed_fw_download() == 0){ | ||
| CcspTraceError(("CosaDmlDIDownloadNow : Not enough memory to proceed firmware download\n")); | ||
| return ANSC_STATUS_FAILURE; | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests exist for CosaDmlDIDownloadNow() but there is no test coverage for the new memory check failure path. Add tests to verify the behavior when can_proceed_fw_download() returns 0, ensuring the function properly returns ANSC_STATUS_FAILURE and logs the appropriate error message.
| } */ | ||
|
|
||
| if(can_proceed_fw_download() == 0){ | ||
| CcspTraceError(("CosaDmlDIDownloadNow : Not enough memory to proceed firmware download\n")); |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message lacks specific information about how much memory is available versus how much is required. Consider enhancing the error message to include actual memory values, which would be more helpful for debugging and troubleshooting in production environments.
| #include "safec_lib_common.h" | ||
| #include <syscfg/syscfg.h> | ||
| #include "secure_wrapper.h" | ||
| #include "fw_download_check.h" |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header file "fw_download_check.h" is included but doesn't appear to exist in the repository. This will cause compilation failures. Ensure the header file is either added to the repository or is available in an external dependency that's properly configured in the build system.
|
|
||
| } */ | ||
|
|
||
| if(can_proceed_fw_download() == 0){ |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function can_proceed_fw_download() is called but its implementation is not found in the codebase. This will cause linker errors unless it's provided by the external library. The function should be declared in the fw_download_check.h header file with clear documentation of its expected return values (the code assumes 0 means insufficient memory, non-zero means sufficient memory).
|
|
||
| libCcspCMAgent_integration_src_shared_la_SOURCES = cosa_x_cisco_com_cablemodem_apis.c cosa_device_info_apis.c cosa_x_rdkcentral_com_cablemodem_apis.c | ||
| libCcspCMAgent_integration_src_shared_la_LDFLAGS = -lccsp_common -lcm_mgnt -lsysevent -lsecure_wrapper | ||
| libCcspCMAgent_integration_src_shared_la_LDFLAGS = -lccsp_common -lcm_mgnt -lsysevent -lsecure_wrapper -lfw_download_chk |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library "-lfw_download_chk" is added as a dependency but there's no verification that this library exists or is available in the build environment. Consider adding appropriate configuration checks (e.g., PKG_CHECK_MODULES or AC_CHECK_LIB in configure.ac) to ensure the library is available before attempting to link against it.
| libCcspCMAgent_integration_src_shared_la_LDFLAGS = -lccsp_common -lcm_mgnt -lsysevent -lsecure_wrapper -lfw_download_chk | |
| libCcspCMAgent_integration_src_shared_la_LDFLAGS = -lccsp_common -lcm_mgnt -lsysevent -lsecure_wrapper @FW_DOWNLOAD_CHK_LIBS@ |
|
|
||
| } */ | ||
|
|
||
| if(can_proceed_fw_download() == 0){ |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value convention of can_proceed_fw_download() is unclear and potentially confusing. The function returns 0 to indicate failure (not enough memory), which is contrary to common C conventions where 0 typically indicates success. Consider using a more explicit return type (e.g., a boolean or named enum) or reversing the logic to align with standard conventions, and add a comment explaining the return value semantics.
| if(can_proceed_fw_download() == 0){ | |
| /* can_proceed_fw_download() returns non-zero when it is safe to proceed, | |
| * and 0 on failure (e.g. not enough memory). */ | |
| if (!can_proceed_fw_download()){ |
|
|
||
| } */ | ||
|
|
||
| if(can_proceed_fw_download() == 0){ |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memory check is performed after several other validation steps and resource allocations. If the intent is to prevent firmware download when memory is insufficient, this check should be moved earlier in the function (before cm_hal_Set_HTTP_Download_Url and cm_hal_Set_HTTP_Download_Interface calls) to avoid unnecessary operations when the download will fail anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adding t2 init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
source/TR-181/integration_src.shared/cosa_device_info_apis.c:507
- The return value of pthread_create is assigned to variable 'res' but is never checked. If the memory check passes but thread creation fails, the function still returns ANSC_STATUS_SUCCESS, which is misleading. This issue exists in the original code as well, but the new memory check makes it more critical - if memory is low enough to fail thread creation, it should be caught. Consider checking 'res' after the memory check and returning ANSC_STATUS_FAILURE if thread creation fails.
pthread_t FWDL_Thread;
res = pthread_create(&FWDL_Thread, NULL, FWDL_ThreadFunc, "FWDL_ThreadFunc");
if(res != 0)
{
CcspTraceError(("Create FWDL_Thread error %d\n", res));
}
return ANSC_STATUS_SUCCESS;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //#include <docsis_ext_interface.h> | ||
| #include "safec_lib_common.h" | ||
| #include "syscfg/syscfg.h" | ||
| #include "telemetry_busmessage_sender.h" |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of telemetry_busmessage_sender.h include and the corresponding t2_init call on line 964 appear unrelated to the stated PR objective of adding available memory check for firmware downloads. If telemetry is needed for reporting memory check failures or firmware download metrics, this should be mentioned in the PR description. Otherwise, this change should be in a separate PR to maintain clear separation of concerns.
| CcspTraceInfo(("pthread create docsis registration\n")); | ||
| pthread_create(&docsisclbk_tid, NULL, GWP_docsisregistration_threadfunc, NULL); | ||
| #endif | ||
| t2_init("CcspCMAgent"); |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of t2_init call appears unrelated to the stated PR objective of adding available memory check for firmware downloads. If telemetry is needed for reporting memory check failures or firmware download metrics, this should be mentioned in the PR description. Otherwise, this change should be in a separate PR to maintain clear separation of concerns.
| if(can_proceed_fw_download() == 0){ | ||
| CcspTraceError(("CosaDmlDIDownloadNow : Not enough memory to proceed firmware download\n")); | ||
| return ANSC_STATUS_FAILURE; | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new memory check for firmware downloads lacks test coverage. The existing test cases for CosaDmlDIDownloadNow (lines 785 and 852 in CcspCMAgentDeviceInfoDmlTest.cpp) do not mock or test the can_proceed_fw_download() function. Add test cases to verify that the function correctly fails when can_proceed_fw_download() returns 0, and succeeds when it returns non-zero.
apattu200
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok for me
RDKB-60656 : Available memory check for firmware downloads Reason for change: Before firmware download, we need to check if the device have enough memory Test Procedure: 1. while firmware download, available memory check logs should be seen. 2. If available memory < required memory, firmware download should not start. Risks: medium Priority: P1 **Gerrit meta-layer changes:** https://gerrit.teamccp.com/#/q/topic:RDKB-60656+(status:open+OR+status:merged) **List all dependent GitHub PRs:** rdkcentral/provisioning-and-management#178 rdkcentral/utopia#182 rdkcentral/cable-modem-agent#23 rdkcentral/miscellaneous-broadband#37 https://github.com/rdk-gdcs/apparmor-profiles/pull/49
RDKB-60656 : Available memory check for firmware downloads Reason for change: Before firmware download, we need to check if the device have enough memory Test Procedure: 1. while firmware download, available memory check logs should be seen. 2. If available memory < required memory, firmware download should not start. Risks: medium Priority: P1 **Gerrit meta-layer changes:** https://gerrit.teamccp.com/#/q/topic:RDKB-60656+(status:open+OR+status:merged) **List all dependent GitHub PRs:** rdkcentral/xconf-client#13 rdkcentral/utopia#182 rdkcentral/cable-modem-agent#23 rdkcentral/miscellaneous-broadband#37 https://github.com/rdk-gdcs/apparmor-profiles/pull/49
RDKB-60656 : Available memory check for firmware downloads Reason for change: Before firmware download, we need to check if the device have enough memory Test Procedure: 1. while firmware download, available memory check logs should be seen. 2. If available memory < required memory, firmware download should not start. Risks: medium Priority: P1 **Gerrit meta-layer changes:** https://gerrit.teamccp.com/#/q/topic:RDKB-60656+(status:open+OR+status:merged) **List all dependent GitHub PRs:** rdkcentral/xconf-client#13 rdkcentral/provisioning-and-management#178 rdkcentral/cable-modem-agent#23 rdkcentral/miscellaneous-broadband#37 https://github.com/rdk-gdcs/apparmor-profiles/pull/49
RDKB-60656 : Available memory check for firmware downloads
Reason for change: Before firmware download, we need to check if the device have enough memory
Test Procedure: 1. while firmware download, available memory check logs should be seen.
2. If available memory < required memory, firmware download should not start.
Risks: medium
Priority: P1
Gerrit meta-layer changes:
https://gerrit.teamccp.com/#/q/topic:RDKB-60656+(status:open+OR+status:merged)
List all dependent GitHub PRs:
rdkcentral/xconf-client#13
rdkcentral/provisioning-and-management#178
rdkcentral/utopia#182
rdkcentral/miscellaneous-broadband#37
https://github.com/rdk-gdcs/apparmor-profiles/pull/49