For linux msm/firehose silence timeout warning #72
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
konradybcio
reviewed
Jun 7, 2024
firehose.c
Outdated
| @@ -53,6 +53,11 @@ | |||
| #include "qdl.h" | |||
| #include "ufs.h" | |||
|
|
|||
| enum { | |||
| FIREHOSE_ACK, | |||
There was a problem hiding this comment.
"Clean this up by defining ACK and NAK in the positive value space and
use the negative value space for errno."
almost, but I don't mind ;)
maybe add a =value to the first entry
konradybcio
reviewed
Jun 7, 2024
| return FIREHOSE_ACK; | ||
| if (xmlStrcmp(value, (xmlChar*)"NAK") == 0) | ||
| return FIREHOSE_NAK; | ||
|
|
There was a problem hiding this comment.
should we print whatever came out here?
There was a problem hiding this comment.
I think we should follow up and adjust the error prints throughout the file to give better error messages. Perhaps strerror(ret) vs "device rejected" printing.
konradybcio
reviewed
Jun 7, 2024
| return ret; | ||
| if (ret != FIREHOSE_ACK) { | ||
| fprintf(stderr, "[CONFIGURE] request failed\n"); | ||
| return -1; |
There was a problem hiding this comment.
might as well return ret to propagate errors from 2 levels deeper
There was a problem hiding this comment.
No, because then we might return FIREHOSE_NAK from firehose_run(), I rather keep that in {0,-1} space...
firehose_read() uses -errno to denote errors, -1 to represent NAK, 1 to represent ACK and 0 to represent that we got a LOG and should read again. This choice is unintuitive and choosing to overload errno and NAK on the negative value space isn't awesome. Additionally, firehose_run() does in some cases return -errno and others -1. Clean this up by defining ACK and NAK in the non-negative value space and use the negative value space for errno. Use -EAGAIN to signal that firehose_read() should make another read attempt. Signed-off-by: Bjorn Andersson <[email protected]>
During boot and following the reset, a firehose_read() is performed to drain any LOG messages from the device, but as there's no response to be read these operations runs until the defined timeout happens - at which time "firehose operation timed out" is printed to stderr, confusing users to believe an error condition has occurred. The only operation that not already print an error message is firehose_reset(), so add this and remove the error print from firehose_read(). Signed-off-by: Bjorn Andersson <[email protected]>
The xmlDoc returned by firehose_response_parse() needs to be freed regardless of what message it contains. Signed-off-by: Bjorn Andersson <[email protected]>
andersson
force-pushed
the
for-linux-msm/firehose-silence-timeout-warning
branch
from
aa2cc69
to
4e3943b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.