Skip to content

Conversation

@igoropaniuk
Copy link
Contributor

Unify the timeout value (set default to 30000ms), propagate it through all architectural layers that use the same QDL structure, and make it configurable via a CLI parameter so it can be increased when needed.

Unify the timeout value (set default to 30000ms), propagate it through
all architectural layers that use the same QDL structure, and make it
configurable via a CLI parameter so it can be increased when needed.

Fixes: linux-msm#163
Signed-off-by: Igor Opaniuk <[email protected]>
@andersson
Copy link
Collaborator

I guess there's might not really be a point to optimize the failure case and keep a lower non-SPI-NOR timeout, we should after all not hit that case...

I don't like the idea of adding the -w option, and I definitely don't like the idea that one of our platforms require that you pass -w with some number. We should make sure that the tool work out of the box, rather than depend on tribal knowledge.
I've reached out to storage team to see what impacts this time and see if we can make a more educated decision (30 was chosen because it was 2x the worst case I measured).

@igoropaniuk
Copy link
Contributor Author

@andersson I don’t see a better solution at the moment. We’ve already encountered this issue twice (as far as I recall), and while we could continue increasing the timeout, that would only mask the underlying problem. What alternative solution would you recommend?

Current solution at least provides the user with a means to quickly adjust the timeout without examining the code

@andersson
Copy link
Collaborator

@andersson I don’t see a better solution at the moment. We’ve already encountered this issue twice (as far as I recall), and while we could continue increasing the timeout, that would only mask the underlying problem. What alternative solution would you recommend?

Current solution at least provides the user with a means to quickly adjust the timeout without examining the code

As written, this PR doesn't solve issue #163 . It retains the default timeout at 30s (and moves non-SPI-NOR to the same), which is known to fail, and then assume that the user will know that they need to pass a higher value.

Regarding "masking underlying problem", I don't know what problems that would be, and you just 30x the non-SPI-NOR timeout.

In my view, we have two options; either we try to figure out how the time relate to memory type, size etc and then encode that, or we pick a larger value that works for everyone.
In the interest of time, and as there's a desire to get a new release out there, I think simply bumping the 30s timeout is reasonable.

@JohnSagaQuic
Copy link

Just share some info here. I think either using 30s timeout, or using the -w option as the code implemented here are fine. PCAT tool use the option "Firehose data reception timeout"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants