Skip to content

Conversation

@purdeaandrei
Copy link
Contributor

@purdeaandrei purdeaandrei commented Sep 26, 2024

This PR depends on PR #18
IMPORTANT: I have not tested this change,
Note that I have tested similar changes in glasgow firmware, with success (will send a PR for that soon).

Note that this also potentially fixes any errors going unnoticed.
If we set HSNAK when we arm the buffer, the host will complete its status stage as soon as it sends the data stage, and will not wait for eeprom_write to finish. The STALL_EP0() call that we execute on line 154, will not have an effect, because the following setup packet will clear it (this is how the FX2 works), so this error would go undetected by the host!

Note that this change should still be tested before being merged, because before this change,
the firmware was not sensitive to potential spurious errors returned by eeprom_write.
If eeprom_write had a spurious errors problem, then this PR could introduce a regression,
so I suggest not merging this PR without testing it first.

Note that STALL_EP0() already sets HSNAK, so it's not necessary to call ACK_EP0(); on failure.

@purdeaandrei purdeaandrei force-pushed the f_fix_potential_race_condition_in_boot_cypress branch from 6070c58 to be6013d Compare September 26, 2024 22:12
@whitequark
Copy link
Member

You should be able to easily test the change with a Glasgow, since for the purpose of the bootloader it's just like any other devboard with a 2-address-byte EEPROM. Sounds good?

@purdeaandrei
Copy link
Contributor Author

Closing this, since the fix is now included in PR 18.
Note that I have successfully tested the fix in PR 18.

@purdeaandrei purdeaandrei deleted the f_fix_potential_race_condition_in_boot_cypress branch October 6, 2025 02:28
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.

2 participants