mod_scmi_clock_config_set_policy doesn't handle different clock id mappings correctly
Created by: christophklee
The code in mod_scmi_clock_config_set_policy is not working correctly, if one uses different mod_scmi_clock_device tables that map their respective clock IDs in a way that the relative IDs overlap, but reference different underlying clocks.
I have provided a simple test case, where the CLOCK_DEV_IDX_PIXEL_2
is not enabled by default, but FAKE_SCMI_AGENT_IDX_CLIENT
references is at relative offset 0. However, the clock 0 CLOCK_DEV_IDX_VPU
is enabled by default by client FAKE_SCMI_AGENT_IDX_OSPM
. As a result, when we run through the code, we use the clock_dev_id of 0, and perform the check that clock_count[clock_dev_id] != 0
which will be true, due to the starts_enabled
being true for CLOCK_DEV_IDX_VPU
of client FAKE_SCMI_AGENT_IDX_OSPM
. We will then skip the handler, and as a result, won't enable the clock CLOCK_DEV_IDX_PIXEL_2
. We can run into the same issue if clocks aren't enabled by default, but two different clients enable a clock with the same relative offset.
I was able to get the test case run correctly, if the clock_dev_idx would be passed to the function itself, and the non-agent related checks would be run against it.
diff --git a/module/scmi_clock/src/mod_scmi_clock.c b/module/scmi_clock/src/mod_scmi_clock.c
index 2db445de..dc0d4d45 100644
--- a/module/scmi_clock/src/mod_scmi_clock.c
+++ b/module/scmi_clock/src/mod_scmi_clock.c
@@ -1149,6 +1149,7 @@ static int scmi_clock_config_set_handler(fwk_id_t service_id,
goto exit;
}
if (policy_status == MOD_SCMI_CLOCK_SKIP_MESSAGE_HANDLER) {
+ printf("Shouldn't have gotten here\n");
return_values.status = (int32_t)SCMI_SUCCESS;
goto exit;
}
diff --git a/module/scmi_clock/test/config_scmi_clock.h b/module/scmi_clock/test/config_scmi_clock.h
index 4ffc2091..71fe8537 100644
--- a/module/scmi_clock/test/config_scmi_clock.h
+++ b/module/scmi_clock/test/config_scmi_clock.h
@@ -15,7 +15,8 @@
#define FAKE_MODULE_IDX 0x5
#define FAKE_SCMI_AGENT_IDX_PSCI 0x1
#define FAKE_SCMI_AGENT_IDX_OSPM 0x2
-#define FAKE_SCMI_AGENT_IDX_COUNT 0x3
+#define FAKE_SCMI_AGENT_IDX_CLIENT 0x3
+#define FAKE_SCMI_AGENT_IDX_COUNT 0x4
/*!
* \brief Clock device indexes.
@@ -25,6 +26,7 @@ enum clock_dev_idx {
CLOCK_DEV_IDX_DPU,
CLOCK_DEV_IDX_PIXEL_0,
CLOCK_DEV_IDX_PIXEL_1,
+ CLOCK_DEV_IDX_PIXEL_2,
CLOCK_DEV_IDX_COUNT
};
@@ -55,12 +57,25 @@ static const struct mod_scmi_clock_device agent_device_table_ospm[] = {
},
};
+static const struct mod_scmi_clock_device agent_device_table_client[] = {
+ {
+ /* PIXEL_2 */
+ .element_id =
+ FWK_ID_ELEMENT_INIT(FAKE_MODULE_IDX, CLOCK_DEV_IDX_PIXEL_2),
+ .starts_enabled = false,
+ },
+};
+
static const struct mod_scmi_clock_agent agent_table[FAKE_SCMI_AGENT_IDX_COUNT] = {
[FAKE_SCMI_AGENT_IDX_PSCI] = { 0 /* No access */ },
[FAKE_SCMI_AGENT_IDX_OSPM] = {
.device_table = agent_device_table_ospm,
.device_count = FWK_ARRAY_SIZE(agent_device_table_ospm),
},
+ [FAKE_SCMI_AGENT_IDX_CLIENT] = {
+ .device_table = agent_device_table_client,
+ .device_count = FWK_ARRAY_SIZE(agent_device_table_client),
+ },
};
struct fwk_module_config config_scmi_clock = {
diff --git a/module/scmi_clock/test/mod_scmi_clock_unit_test.c b/module/scmi_clock/test/mod_scmi_clock_unit_test.c
index 7e346cdc..2d4c0266 100644
--- a/module/scmi_clock/test/mod_scmi_clock_unit_test.c
+++ b/module/scmi_clock/test/mod_scmi_clock_unit_test.c
@@ -144,13 +144,66 @@ void test_function_set_rate(void)
TEST_ASSERT_EQUAL(FWK_SUCCESS, status);
}
+static void set_config(unsigned int agent_id, unsigned int clock_idx,
+ enum clock_dev_idx clock_dev_idx)
+{
+ int status;
+
+ fwk_id_t service_id = FWK_ID_ELEMENT_INIT(FAKE_MODULE_IDX, agent_id);
+
+
+ struct scmi_clock_config_set_a2p payload = {
+ .clock_id = clock_idx,
+ .attributes = UINT32_C(1) << SCMI_CLOCK_CONFIG_SET_ENABLE_POS,
+ };
+
+ mod_scmi_from_protocol_api_get_agent_id_ExpectAnyArgsAndReturn(FWK_SUCCESS);
+ mod_scmi_from_protocol_api_get_agent_id_ReturnThruPtr_agent_id(&agent_id);
+
+ #if defined(BUILD_HAS_MOD_RESOURCE_PERMS)
+ mod_scmi_from_protocol_api_get_agent_id_ExpectAnyArgsAndReturn(FWK_SUCCESS);
+ mod_res_permissions_api_agent_has_resource_permission_ExpectAnyArgsAndReturn(MOD_RES_PERMS_ACCESS_ALLOWED);
+ #endif
+
+ mod_scmi_from_protocol_api_get_agent_id_ExpectAnyArgsAndReturn(FWK_SUCCESS);
+ mod_scmi_from_protocol_api_get_agent_id_ReturnThruPtr_agent_id(&agent_id);
+
+ mod_scmi_from_protocol_api_get_agent_id_ExpectAnyArgsAndReturn(FWK_SUCCESS);
+ mod_scmi_from_protocol_api_get_agent_id_ReturnThruPtr_agent_id(&agent_id);
+
+ fwk_module_is_valid_element_id_ExpectAnyArgsAndReturn(true);
+
+ mod_scmi_from_protocol_api_get_agent_id_ExpectAnyArgsAndReturn(FWK_SUCCESS);
+ mod_scmi_from_protocol_api_get_agent_id_ReturnThruPtr_agent_id(&agent_id);
+
+ fwk_id_get_element_idx_ExpectAnyArgsAndReturn(clock_dev_idx);
+ fwk_id_is_equal_ExpectAnyArgsAndReturn(true);
+
+ /* We shouldn't expect this call */
+ mod_scmi_from_protocol_api_respond_ExpectAnyArgsAndReturn(FWK_SUCCESS);
+
+ status = to_protocol_api->message_handler((fwk_id_t)MOD_SCMI_PROTOCOL_ID_CLOCK, service_id,
+ (const uint32_t *)&payload,
+ payload_size_table[MOD_SCMI_CLOCK_CONFIG_SET],
+ MOD_SCMI_CLOCK_CONFIG_SET);
+
+ TEST_ASSERT_EQUAL(FWK_SUCCESS, status);
+}
+
+void test_function_set_config(void)
+{
+ set_config(FAKE_SCMI_AGENT_IDX_CLIENT, 0, CLOCK_DEV_IDX_PIXEL_2);
+}
+
int scmi_test_main(void)
{
UNITY_BEGIN();
#if defined(BUILD_HAS_MOD_RESOURCE_PERMS)
RUN_TEST(test_function_set_rate);
+ RUN_TEST(test_function_set_config);
#else
RUN_TEST(test_function_set_rate);
+ RUN_TEST(test_function_set_config);
#endif
return UNITY_END();
}