- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: spi25: Integrate 4BA support
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/22384 ) Change subject: spi25: Integrate 4BA support ...................................................................... Patch Set 1: I haven't gone thru this patch again (I suspect it's fine), but wanted to add response to your comment in 22020. The W25Q256FV is an example of a chip which supports: - Native 4BA instructions - Extended address register: 3BA commands used with a separate register to hold the most significant address byte. - 4BA address mode: 3BA commands re-purposed to clock in 4 address bytes instead of 3. -- To view, visit https://review.coreboot.org/22384 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I644600beaab9a571b97b67f7516abe571d3460c1 Gerrit-Change-Number: 22384 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 13 Nov 2017 01:47:46 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[master]: spi25: Use common code for nbyte read/write and block erase
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/22383 ) Change subject: spi25: Use common code for nbyte read/write and block erase ...................................................................... Patch Set 1: Code-Review+2 (2 comments) Very minor spelling error, otherwise LGTM. https://review.coreboot.org/#/c/22383/1/spi25.c File spi25.c: https://review.coreboot.org/#/c/22383/1/spi25.c@326 PS1, Line 326: spi_poll_wip Thanks for updating this. https://review.coreboot.org/#/c/22383/1/spi25.c@384 PS1, Line 384: iff if -- To view, visit https://review.coreboot.org/22383 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1ae48acbfbd427a30bcd64bdc080dc3dc20503 Gerrit-Change-Number: 22383 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 13 Nov 2017 01:24:12 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[master]: spi25: Introduce spi_simple_write_cmd()
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/22382 ) Change subject: spi25: Introduce spi_simple_write_cmd() ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/22382 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib244356fa471e15863b52e6037899d19113cb4a9 Gerrit-Change-Number: 22382 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 13 Nov 2017 01:17:56 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[master]: linux_spi: Dynamically detect max buffer size
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/22441 ) Change subject: linux_spi: Dynamically detect max buffer size ...................................................................... Patch Set 1: Code-Review-2 Awaiting +2 on https://review.coreboot.org/#/c/22344/ -- To view, visit https://review.coreboot.org/22441 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Gerrit-Change-Number: 22441 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 13 Nov 2017 00:53:19 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: linux_spi: Dynamically detect max buffer size
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/22344 ) Change subject: linux_spi: Dynamically detect max buffer size ...................................................................... Patch Set 6: Cherry-picked to master here: https://review.coreboot.org/#/c/22441/ If you're OK with this patch I will abandon this one and merge the one on master instead. -- To view, visit https://review.coreboot.org/22344 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Gerrit-Change-Number: 22344 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: david.hendricks(a)gmail.com Gerrit-Comment-Date: Mon, 13 Nov 2017 00:52:54 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[master]: linux_spi: Dynamically detect max buffer size
by build bot (Jenkins) (Code Review) 13 Nov '17

13 Nov '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/22441 ) Change subject: linux_spi: Dynamically detect max buffer size ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/757/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/867/ : SUCCESS -- To view, visit https://review.coreboot.org/22441 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Gerrit-Change-Number: 22441 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 13 Nov 2017 00:51:29 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[master]: linux_spi: Dynamically detect max buffer size
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/22441 Change subject: linux_spi: Dynamically detect max buffer size ...................................................................... linux_spi: Dynamically detect max buffer size Read max buffer size from sysfs if available. Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Signed-off-by: Keno Fischer <keno(a)juliacomputing.com> Signed-off-by: David Hendricks <dhendricks(a)fb.com> --- M linux_spi.c 1 file changed, 45 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/41/22441/1 diff --git a/linux_spi.c b/linux_spi.c index e51fbc4..611559a 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -42,6 +42,8 @@ */ static int fd = -1; +#define BUF_SIZE_FROM_SYSFS "/sys/module/spidev/parameters/bufsiz" +static size_t max_kernel_buf_size; static int linux_spi_shutdown(void *data); static int linux_spi_send_command(struct flashctx *flash, unsigned int writecnt, @@ -127,8 +129,45 @@ return 1; } - register_spi_master(&spi_master_linux); + /* Read max buffer size from sysfs, or use page size as fallback. */ + FILE *fp; + fp = fopen(BUF_SIZE_FROM_SYSFS, "r"); + if (!fp) { + msg_pwarn("Cannot open %s: %s.\n", BUF_SIZE_FROM_SYSFS, strerror(errno)); + goto out; + } + char buf[10]; + memset(buf, 0, sizeof(buf)); + if (!fread(buf, 1, sizeof(buf) - 1, fp)) { + if (feof(fp)) + msg_pwarn("Cannot read %s: file is empty.\n", BUF_SIZE_FROM_SYSFS); + else + msg_pwarn("Cannot read %s: %s.\n", BUF_SIZE_FROM_SYSFS, strerror(errno)); + goto out; + } + + long int tmp; + errno = 0; + tmp = strtol(buf, NULL, 0); + if ((tmp < 0) || errno) { + msg_pwarn("Buffer size %ld from %s seems wrong.\n", tmp, BUF_SIZE_FROM_SYSFS); + } else { + msg_pdbg("%s: Using value from %s as max buffer size.\n", __func__, BUF_SIZE_FROM_SYSFS); + max_kernel_buf_size = (size_t)tmp; + } + +out: + if (fp) + fclose(fp); + + if (!max_kernel_buf_size) { + msg_pdbg("%s: Using page size as max buffer size.\n", __func__); + max_kernel_buf_size = (size_t)getpagesize(); + } + + msg_pdbg("%s: max_kernel_buf_size: %zu\n", __func__, max_kernel_buf_size); + register_spi_master(&spi_master_linux); return 0; } @@ -179,17 +218,16 @@ return 0; } -static int linux_spi_read(struct flashctx *flash, uint8_t *buf, - unsigned int start, unsigned int len) +static int linux_spi_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - return spi_read_chunked(flash, buf, start, len, - (unsigned int)getpagesize()); + /* Read buffer is fully utilized for data. */ + return spi_read_chunked(flash, buf, start, len, max_kernel_buf_size); } static int linux_spi_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) { - return spi_write_chunked(flash, buf, start, len, - ((unsigned int)getpagesize()) - 4); + /* 5 bytes must be reserved for longest possible command + address. */ + return spi_write_chunked(flash, buf, start, len, max_kernel_buf_size - 5); } #endif // CONFIG_LINUX_SPI == 1 -- To view, visit https://review.coreboot.org/22441 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Gerrit-Change-Number: 22441 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>

1 0

Change in flashrom[staging]: linux_spi: Dynamically detect max buffer size
by build bot (Jenkins) (Code Review) 13 Nov '17

13 Nov '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/22344 ) Change subject: linux_spi: Dynamically detect max buffer size ...................................................................... Patch Set 6: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/756/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/866/ : SUCCESS -- To view, visit https://review.coreboot.org/22344 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Gerrit-Change-Number: 22344 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: david.hendricks(a)gmail.com Gerrit-Comment-Date: Mon, 13 Nov 2017 00:50:35 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: linux_spi: Dynamically detect max buffer size
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

Hello david.hendricks(a)gmail.com, Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/22344 to look at the new patch set (#6). Change subject: linux_spi: Dynamically detect max buffer size ...................................................................... linux_spi: Dynamically detect max buffer size Read max buffer size from sysfs if available. Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Signed-off-by: Keno Fischer <keno(a)juliacomputing.com> Signed-off-by: David Hendricks <dhendricks(a)fb.com> --- M linux_spi.c 1 file changed, 45 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/22344/6 -- To view, visit https://review.coreboot.org/22344 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Gerrit-Change-Number: 22344 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: david.hendricks(a)gmail.com

1 0

Change in flashrom[staging]: linux_spi: Dynamically detect max buffer size
by David Hendricks (Code Review) 13 Nov '17

13 Nov '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/22344 ) Change subject: linux_spi: Dynamically detect max buffer size ...................................................................... Patch Set 5: (5 comments) Yeah, I was on the fence whether to ask Keno to make the changes. Since my updates to the original patch are straight-forward and don't change the intended behavior I figured it would save everyone time to hijack it rather than go back-and-forth. I suppose that's another one of the finer points of community engagement we should think about and document as a guideline on the wiki. https://review.coreboot.org/#/c/22344/3/linux_spi.c File linux_spi.c: https://review.coreboot.org/#/c/22344/3/linux_spi.c@199 PS3, Line 199: The implementation currently does not support requests that > No, I meant in the kernel. And the command is never written into Ah, I understand what you are saying now. The only time we can hit this limit is for writes. So the old code is correct and we should not subtract the maximum command size (5) from the read buffer length. I reverted the change to the read function and updated the comment to clarify the assumptions made here. https://review.coreboot.org/#/c/22344/5/linux_spi.c File linux_spi.c: https://review.coreboot.org/#/c/22344/5/linux_spi.c@136 PS5, Line 136: msg_pwarn("%s: Cannot open %s: %s.\n", __func__, BUF_SIZE_FROM_SYSFS, strerror(errno)); > I was about to say, "function names in warnings?". But hey, we do it You're right - Function names in warnings are kind of ugly and probably not needed. I like to use them in debug prints, but they are excessive for a relatively benign and easy to trace warning such as this. https://review.coreboot.org/#/c/22344/5/linux_spi.c@143 PS5, Line 143: msg_pwarn("%s: Cannot read %s: %s.\n", __func__, BUF_SIZE_FROM_SYSFS, strerror(errno)); > Nit, in the unlikely event that we are presented an empty file, Good point, however I think either if the kernel provides the file (e.g. the above fopen() works) then it will have an integer to read from. In any case, I went ahead and added an feof() to check for this condition. I was thinking of using stat(), but that would add a few more lines and seemed redundant with fopen() error handling in this case. LMK if you have a preference. BTW - It may be worth moving all this logic to a file handling library some day, especially as we improve support for Linux interfaces that involve searching thru sysfs and proc and potentially opening/closing a lot of files. https://review.coreboot.org/#/c/22344/5/linux_spi.c@154 PS5, Line 154: max_kernel_buf_size = (size_t)tmp; > Nit, possible overflow. Very theoretic, don't know any environment Yeah, that's certainly an assumption that I suppose language lawyers may take issue with. But for this use case it seems like a reasonably safe conversion since we're expecting the kernel to give us a small integer. If we add a generic file library then we definitely should add extra paranoid checking. For example we might check that the length of the string read from the file can't form a number that exceeds the size of the integer type. https://review.coreboot.org/#/c/22344/5/linux_spi.c@163 PS5, Line 163: max_kernel_buf_size = (size_t)getpagesize(); > getpagesize() is kind of deprecated, but we've used it before anyway. Should I follow-up with a patch to use sysconf(_SC_PAGESIZE) instead, as suggested by the man page for getpagesize()? -- To view, visit https://review.coreboot.org/22344 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic541e548ced8488f074d388f1c92174cad123064 Gerrit-Change-Number: 22344 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: david.hendricks(a)gmail.com Gerrit-Comment-Date: Mon, 13 Nov 2017 00:50:13 +0000 Gerrit-HasComments: Yes

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit