Skip to content

SerialFlash: enlarge chip ID buffer #207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

SerialFlash: enlarge chip ID buffer #207

wants to merge 1 commit into from

Conversation

eriknyquist
Copy link
Contributor

SerialFlashChip::readID() writes up to 5 bytes into
the buffer provided, but the caller only allocates
3 bytes. Increase buffer size to 5 bytes.

@eriknyquist
Copy link
Contributor Author

@calvinatintel please don't merge yet-- I'd like to run a Klocwork scan first.

@eriknyquist
Copy link
Contributor Author

@SidLeung @bigdinotech ready for review

@eriknyquist
Copy link
Contributor Author

Some context; the offending code is only executed when a Spansion device is detected. The 101's onboard SPI Flash (the one which the SerialFlash library will use) is a Winbond, so the only way a user could see this bug while using a 101 is by modifying the SerialFlash library to use SPI (instead of SPI1) and connecting a Spansion device to it. In my opinion, this doesn't affect Bellatrix, what do you guys think?

@bbaltz505
Copy link
Contributor

Maybe just add a comment then.

@SidLeung
Copy link
Contributor

SidLeung commented Jun 3, 2016

@eriknyquist It's not going to affect Bellatrix and the chances of having someone using SPI on a Spansion device is small. Fix it, push it, and release it on the next round, Castor. BTW, is there a reason between 3 and 5? If so, would you please put it in as comment in the code too? Thanks.

@eriknyquist
Copy link
Contributor Author

@bbaltz505 I was trying to suggest that this commit doesn't need to go into Bellatrix. Do you mean add a comment to this PR?

@bbaltz505
Copy link
Contributor

@eriknyquist , I meant if you were to touch the code, just add a comment. No need to reopen it for Bellatrix though - given your explanation.

@eriknyquist
Copy link
Contributor Author

I sent Paul a PR for the same.
PaulStoffregen/SerialFlash#28

Since Paul's library works out of the box on a 101 now, I think we want to take it out of the next release and just direct users to download the latest version through the IDE library manager. So I'm not even sure if this PR should be merged?

@eriknyquist
Copy link
Contributor Author

eriknyquist commented Jun 3, 2016

@SidLeung missed your earlier comment, yes the reason for 5 is in the SerialFlashChip::readID function; if the device is a Spansion, then buf[3] and buf[4] are written, but in all other cases only the first 3 bytes are used. I added a comment.

SerialFlashChip::readID() writes up to 5 bytes into
the buffer provided, but the caller only allocates
3 bytes. Increase buffer size to 5 bytes.
@eriknyquist
Copy link
Contributor Author

eriknyquist commented Jun 3, 2016

No Klocwork issues in this PR, thanks for the help there @bbaltz505 :)
@calvinatintel ready to merge but not for Bellatrix- I'll mark it for Castor.

@eriknyquist eriknyquist added this to the Castor milestone Jun 3, 2016
@eriknyquist eriknyquist added the bug label Jun 3, 2016
@eriknyquist
Copy link
Contributor Author

@calvinatintel ready to merge for Castor

@calvinatintel
Copy link
Contributor

cherry-picked

@PaulStoffregen
Copy link

Also applied upstream :) PaulStoffregen/SerialFlash@05965b5

@eriknyquist
Copy link
Contributor Author

Thanks @PaulStoffregen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants