Hello again :-)
I have reviewed the patch.
The old source code had 3 functions ft2232_init() that can call
ft2232_init_libftdi() or ft2232_init_ftd2xx() depending on a ft2232
ftdi communication library available. This is pretty simple and
coherent.
Yes simple, but bugged !!!
The new source code (after patching) have 6 functions doing almost
the same: ft2232_init_ftd2xx(), ft2232_open_ftd2xx(),
ft2232_init_libftdi(), ft2232_open_libftdi(), ft2232_init_sub(void),
ft2232_init() and the header update ;-)
Yes, this is to decouple the open and init (open the handle, init the
associated specific layout).
(more smaller function ... as good OO coding !)
The ft2232_init_sub(void) and ft2232_init() could be merged again in a
ft2232_open if needed
But yes, we have all different point of view regarding what is open what
is init ...
The objective is to have something like :
open
init
deinit
close
and not as the actual
init
quit
As I understand the problem is with the ft2232_buffer=malloc() within
ft2232_init() that can fail after malloc causing function return
without ft2232_buffer=free().
The current behavior is as follows: Program returns error and finish
execution because it cannot run without interface. Operating system
makes memory page free on process termination.
The proposed solution is to free() allocated memory before
ft2232_init() returns.
I can understand that leaving allocated memory is bad. However there
is a simpler solution to the problem (patch attached) by only changing
few lines of existing code and leaving driver infrastructure/layout
common for all source code:
ft2232_buffer_size = 0;
ft2232_buffer = malloc(FT2232_BUFFER_SIZE); <-- this is the bad malloc
* for (;;){
* if (layout->init() != ERROR_OK) break; <-- we jump out on failure
if (ft2232_device_is_highspeed())
{
#ifndef BUILD_FT2232_HIGHSPEED
#if BUILD_FT2232_FTD2XX == 1
LOG_WARNING("High Speed device found - You need a newer
FTD2XX driver (version 2.04.16 or later)");
#elif BUILD_FT2232_LIBFTDI == 1
LOG_WARNING("High Speed device found - You need a newer
libftdi version (0.16 or later)");
#endif
#endif
/* make sure the legacy mode is disabled */
* if (ft2232h_ft4232h_clk_divide_by_5(false) !=
ERROR_OK) break;
}
buf[0] = 0x85; /* Disconnect TDI/DO to TDO/DI for Loopback */
if ((retval = ft2232_write(buf, 1, &bytes_written)) !=
ERROR_OK)
{
LOG_ERROR("couldn't write to FT2232 to disable
loopback");
* break;
}
#if BUILD_FT2232_FTD2XX == 1
retval = ft2232_purge_ftd2xx();
#elif BUILD_FT2232_LIBFTDI == 1
retval = ft2232_purge_libftdi();
#endif
* if (retval != ERROR_OK) break;
* return ERROR_OK; <-- do not run infinite loop, return ok if no error
}
* free(ft2232_buffer); <-- and land here with freeing bad memory
* return ERROR_JTAG_INIT_FAILED;
In my opinion patch brings only unnecessary confusion and source code
complication, sorry, there is simpler solution. This program does not
need doubling function number and making code even more difficult to
read and understand as it is already enough complicated. Using more
function(void) and global structures only make things worst :-(
%./openocd -f interface/kt-link.cfg -f board/stm3210b_eval.cfg
Open On-Chip Debugger 0.5.0-dev-00895-g796086c-dirty (2011-06-01-20:16)
Licensed under GNU GPL v2
For bug reports, read
http://openocd.berlios.de/doc/doxygen/bugs.html
Info : only one transport option; autoselect 'jtag'
1000 kHz
adapter_nsrst_delay: 100
jtag_ntrst_delay: 100
cortex_m3 reset_config sysresetreq
Info : max TCK change to: 30000 kHz
Info : clock speed 1000 kHz
Info : JTAG tap: stm32.cpu tap/device found: 0x3ba00477 (mfg: 0x23b,
part: 0xba00, ver: 0x3)
Info : JTAG tap: stm32.bs tap/device found: 0x16410041 (mfg: 0x020,
part: 0x6410, ver: 0x1)
Info : stm32.cpu: hardware has 6 breakpoints, 4 watchpoints
%./openocd -f interface/jtagkey2p.cfg -f board/stm3210b_eval.cfg
Open On-Chip Debugger 0.5.0-dev-00895-g796086c-dirty (2011-06-01-20:16)
Licensed under GNU GPL v2
For bug reports, read
http://openocd.berlios.de/doc/doxygen/bugs.html
Info : only one transport option; autoselect 'jtag'
1000 kHz
1000 kHz
adapter_nsrst_delay: 100
jtag_ntrst_delay: 100
cortex_m3 reset_config sysresetreq
Info : max TCK change to: 30000 kHz
Info : clock speed 1000 kHz
Info : JTAG tap: stm32.cpu tap/device found: 0x3ba00477 (mfg: 0x23b,
part: 0xba00, ver: 0x3)
Info : JTAG tap: stm32.bs tap/device found: 0x16410041 (mfg: 0x020,
part: 0x6410, ver: 0x1)
Info : stm32.cpu: hardware has 6 breakpoints, 4 watchpoints
^C
It would be nice however to add Ctrl+C signal capture to gracefully
shutdown the program with proper deinitialization... maybe simple tcl
"quit" mapping would do the job :-)
The shutdown at upper level already exist and works great, but the quit
in ft2232 is bugged ! Sebastien has patch to correct this too .
TCL will not do the job here. This should be corrected in the ft2232.c
directly.
Best regards :-)
Tomek
_______________________________________________
Openocd-development mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/openocd-development
