[OpenOCD-devel] [PATCH]: c3aa419 stlink: fix execution order in stlink_config_trace()

[<ge...@op...>]

This is an automated email from Gerrit.

Antonio Borneo (bor...@gm...) just uploaded a new patch set to Gerrit, which you can find at http://openocd.zylin.com/5934

-- gerrit

commit c3aa419f6569a915c848aa6e80b580d41ee74f18
Author: Antonio Borneo <bor...@gm...>
Date:   Thu Nov 12 15:27:01 2020 +0100

    stlink: fix execution order in stlink_config_trace()
    
    The change [1] guarantees that the value pointed by 'prescaler'
    gets always set, even when the adapter does not support the
    specific mode requested (e.g. sync), or during trace disabling.
    This works fine with the code in armv7m_trace_tpiu_config(), but
    requires all the parameters to be valid also to disable the trace
    (with 'enable==false'), otherwise returns error on incorrect
    parameters or even causes segmentation fault if pointers
    'trace_freq' or 'prescaler' are NULL.
    
    Another problem in stlink_config_trace(), not linked with [1], is
    caused by a tentative to change the settings on an already enabled
    trace; the trace is disabled before the new parameters are fully
    validated and in case of invalid parameters the trace is not
    re-enabled.
    It would be more logical to first check all the parameters, then
    disable the trace, change the settings and re-enable the trace.
    
    Practically revert [1] by checking 'enable==false' at function
    entry, then disable trace and exit without any further check on
    the other parameters.
    For the case 'enable==true', validate all the function parameters
    then disable the trace, update the trace settings and re-enable
    the trace.
    Modify the caller armv7m_trace_tpiu_config() to initialize the
    variable 'prescaler' to a safe value to avoid the issue targeted
    by [1].
    
    [1] commit 38277fa75280 ("jtag/drivers/stlink_usb: fix SWO prescaler")
    
    Change-Id: Ia6530682162ca2c9f5ac64301f2456f70cc07ed2
    Signed-off-by: Antonio Borneo <bor...@gm...>

diff --git a/src/jtag/drivers/stlink_usb.c b/src/jtag/drivers/stlink_usb.c
index c54e2cc..dceeb22 100644
--- a/src/jtag/drivers/stlink_usb.c
+++ b/src/jtag/drivers/stlink_usb.c
@@ -2990,8 +2990,16 @@ static int stlink_config_trace(void *handle, bool enabled,
 {
 	struct stlink_usb_handle_s *h = handle;
 
-	if (enabled && (!(h->version.flags & STLINK_F_HAS_TRACE) ||
-			pin_protocol != TPIU_PIN_PROTOCOL_ASYNC_UART)) {
+	if (!enabled) {
+		stlink_usb_trace_disable(h);
+		return ERROR_OK;
+	}
+
+	assert(trace_freq != NULL);
+	assert(prescaler != NULL);
+
+	if (!(h->version.flags & STLINK_F_HAS_TRACE) ||
+			pin_protocol != TPIU_PIN_PROTOCOL_ASYNC_UART) {
 		LOG_ERROR("The attached ST-LINK version doesn't support this trace mode");
 		return ERROR_FAIL;
 	}
@@ -3000,14 +3008,12 @@ static int stlink_config_trace(void *handle, bool enabled,
 			STLINK_V3_TRACE_MAX_HZ : STLINK_TRACE_MAX_HZ;
 
 	/* Only concern ourselves with the frequency if the STlink is processing it. */
-	if (enabled && *trace_freq > max_trace_freq) {
+	if (*trace_freq > max_trace_freq) {
 		LOG_ERROR("ST-LINK doesn't support SWO frequency higher than %u",
 			  max_trace_freq);
 		return ERROR_FAIL;
 	}
 
-	stlink_usb_trace_disable(h);
-
 	if (!*trace_freq)
 		*trace_freq = max_trace_freq;
 
@@ -3029,8 +3035,7 @@ static int stlink_config_trace(void *handle, bool enabled,
 
 	*prescaler = presc;
 
-	if (!enabled)
-		return ERROR_OK;
+	stlink_usb_trace_disable(h);
 
 	h->trace.source_hz = *trace_freq;
 
diff --git a/src/target/armv7m_trace.c b/src/target/armv7m_trace.c
index 916d1a1..7d3c6c1 100644
--- a/src/target/armv7m_trace.c
+++ b/src/target/armv7m_trace.c
@@ -86,7 +86,7 @@ int armv7m_trace_tpiu_config(struct target *target)
 {
 	struct armv7m_common *armv7m = target_to_armv7m(target);
 	struct armv7m_trace_config *trace_config = &armv7m->trace_config;
-	uint16_t prescaler;
+	uint16_t prescaler = TPIU_ACPR_MAX_SWOSCALER + 1;
 	int retval;
 
 	target_unregister_timer_callback(armv7m_poll_trace, target);

--