November 2017 - flashrom-gerrit

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

13 Nov '17

Nico Huber 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 -- 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: 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-Comment-Date: Mon, 13 Nov 2017 18:55:34 +0000 Gerrit-HasComments: No

1 0

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

13 Nov '17

Nico Huber 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 Code-Review+2 (3 comments) https://review.coreboot.org/#/c/22344/5/linux_spi.c File linux_spi.c: https://review.coreboot.org/#/c/22344/5/linux_spi.c@143 PS5, Line 143: if (feof(fp)) > 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. feof() is fine. stat() seems overkill and I'm not sure how well it is implemented for sysfs ;) > > 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. Sounds good, especially if we hit the same pattern (e.g. reading an unsigned) from a file again. https://review.coreboot.org/#/c/22344/5/linux_spi.c@154 PS5, Line 154: msg_pwarn("Buffer size %ld from %s seems wrong.\n", tmp, BUF_SIZE_FROM_SYSFS); > Yeah, that's certainly an assumption that I suppose language lawyers may ta Same for the part of the string we didn't read, e.g. what if we don't hit EOF after 10 chars oO https://review.coreboot.org/#/c/22344/5/linux_spi.c@163 PS5, Line 163: > Should I follow-up with a patch to use sysconf(_SC_PAGESIZE) instead, as su Not a big issue, IMO. But if you do, I'm happy to review. -- 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 18:55:07 +0000 Gerrit-HasComments: Yes

1 0

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

13 Nov '17

Nico Huber 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: (1 comment) https://review.coreboot.org/#/c/22383/1/spi25.c File spi25.c: https://review.coreboot.org/#/c/22383/1/spi25.c@384 PS1, Line 384: iff > if `iff` is short for `if and only if`... I can write it out ofc. -- 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: 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-Comment-Date: Mon, 13 Nov 2017 09:00:26 +0000 Gerrit-HasComments: Yes

1 0

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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2017