open-power / pdbg

PowerPC FSI Debugger
Apache License 2.0
16 stars 39 forks source link

libpdbg: skip thread state updates during chipop failure #68

Closed ojayanth closed 2 years ago

ojayanth commented 2 years ago

Incase chip-op failure pdbg captures last chip-op FFDC information. Currently sbefifo back-end mode to get thread status, pdbg makes another chip-op. This will override the actual error.

This commits helps to return correct return code for thread start/stop/sreset chip-ops by skipping thread status update call during failure.

npiggin commented 2 years ago

I don't mind going down this route, but I would prefer if we did it consistently. That is, remove such status updates from all the other paths on error as well (e.g., sbefifo_thread_start). Probably also in the scom processor backends as well to make things consistent.

We might want an error state flag in the thread (or possibly in the pdbg_target?), basically saying we've had an error and don't know what the state is. That's doesn't have to come now though, it would probably be part of a wider ranging error handling cleanup.

ojayanth commented 2 years ago

I don't mind going down this route, but I would prefer if we did it consistently. That is, remove such status updates from all the other paths on error as well (e.g., sbefifo_thread_start). Probably also in the scom processor backends as well to make things consistent. This need additional commits based on use case.

We might want an error state flag in the thread (or possibly in the pdbg_target?), basically saying we've had an error and don't know what the state is. That's doesn't have to come now though, it would probably be part of a wider ranging error handling cleanup. BMC application already maintain, SBE states, which helps to avoid mutiple errors. We can't alwys' truct sbe mainitained state register, incase of sbe failure/hung scenario the status may not be updated. Current direction followed here is for long runni g app' maintain SBE state and intiate chip-op if SBE is in good state.

ojayanth commented 2 years ago

Accidentally closed this PR.

npiggin commented 2 years ago

Thanks! This is along the lines of what I'd like to see. Could you do the same for the single-thread operations (e.g., sbefifo_thread_start) as well as the pib ones? Could roll them into the same patches if you like, probably don't need 3 more patches.

Minor nit, pdbg coding style has space between keyword and parenthesis 'if (' and generally forego braces for single-line, single-statement control flow cases.

ojayanth commented 2 years ago

Thanks! This is along the lines of what I'd like to see. Could you do the same for the single-thread operations (e.g., sbefifo_thread_start) as well as the pib ones? Could roll them into the same patches if you like, probably don't need 3 more patches. single-thread operation also call's the refer File : libpdbg/sbefifo.c functions: sbefifo_pib and sbefifo_thread

Minor nit, pdbg coding style has space between keyword and parenthesis 'if (' and generally forego braces for single-line, single-statement control flow cases.

Done

ojayanth commented 2 years ago

Thanks! This is along the lines of what I'd like to see. Could you do the same for the single-thread operations (e.g., sbefifo_thread_start) as well as the pib ones? Could roll them into the same patches if you like, probably don't need 3 more patches. single-thread operation also call's the refer File : libpdbg/sbefifo.c functions: sbefifo_pib and sbefifo_thread.

Updated , missing places. Minor nit, pdbg coding style has space between keyword and parenthesis 'if (' and generally forego braces for single-line, single-statement control flow cases.

Done

npiggin commented 2 years ago

Looks good. For each patch,

Reviewed-by: Nicholas Piggin npiggin@gmail.com