Hello Nicolas Boichat,
I'd like you to do a code review. Please visit
https://review.coreboot.org/22506
to review the following change.
Change subject: linux_mtd: fix wp range detection in mtd_wp_status
......................................................................
linux_mtd: fix wp range detection in mtd_wp_status
The logic in wp range detection in mtd_wp_status is wrong. The first
block (start == 0) may possible be write-protected, thus we can't use
'if (!start)' for detecting start of write-protection. Since start and
end are both uint32_t, we add two new int variables to flag the status.
Signed-off-by: Wei-Ning Huang <wnhuang(a)google.com>
BUG=chrome-os-partner:40208
TEST=`flashrom --wp-enable --wp-range 0x0 0x200000` then `flashrom --wp-status`
should display: 'WP: write protect range: start=0x00000000, len=0x00200000'
Change-Id: Id8934c2ba997ff8558a9be3a5aa05b56272c9e85
Reviewed-on: https://chromium-review.googlesource.com/330225
Commit-Ready: Wei-Ning Huang <wnhuang(a)chromium.org>
Tested-by: Wei-Ning Huang <wnhuang(a)chromium.org>
Reviewed-by: David Hendricks <dhendrix(a)chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat(a)chromium.org>
---
M linux_mtd.c
1 file changed, 9 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/22506/1
diff --git a/linux_mtd.c b/linux_mtd.c
index c707167..6055aef 100644
--- a/linux_mtd.c
+++ b/linux_mtd.c
@@ -476,6 +476,7 @@
static int mtd_wp_status(const struct flashchip *flash)
{
uint32_t start = 0, end = 0;
+ int start_found = 0, end_found = 0;
unsigned int u;
/* For now, assume only one contiguous region can be locked (NOR) */
@@ -492,14 +493,18 @@
msg_perr("%s: ioctl: %s\n", __func__, strerror(errno));
return 1;
} else if (rc == 1) {
- if (!start)
+ if (!start_found) {
start = erase_info.start;
+ start_found = 1;
+ }
} else if (rc == 0) {
- if (start)
- end = erase_info.start - 1;
+ if (start_found) {
+ end = erase_info.start;
+ end_found = 1;
+ }
}
- if (start && end) {
+ if (start_found && end_found) {
msg_pinfo("WP: write protect range: start=0x%08x, "
"len=0x%08x\n", start, end - start);
/* TODO: Replace this break with "start = end = 0" if
--
To view, visit https://review.coreboot.org/22506
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id8934c2ba997ff8558a9be3a5aa05b56272c9e85
Gerrit-Change-Number: 22506
Gerrit-PatchSet: 1
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nicolas Boichat <drinkcat(a)chromium.org>
Gerrit-Reviewer: Wei-Ning Huang <wnhuang(a)google.com>
Hello Daniel Kurtz,
I'd like you to do a code review. Please visit
https://review.coreboot.org/22505
to review the following change.
Change subject: linux_mtd: fix mtd_wp_status
......................................................................
linux_mtd: fix mtd_wp_status
Fix invalid loop range in mtd_wp_status, which ends the for loop
prematurely.
BUG=chrome-os-partner:40208
TEST=`flashrom --wp-status` should output something like:
'WP: write protect range: start=0x00001000, len=0x0000efff'
Change-Id: Ic582bb034c0d2f811813fb6a25696ccddef43e8f
Reviewed-on: https://chromium-review.googlesource.com/329927
Commit-Ready: Wei-Ning Huang <wnhuang(a)chromium.org>
Tested-by: Wei-Ning Huang <wnhuang(a)chromium.org>
Reviewed-by: Daniel Kurtz <djkurtz(a)chromium.org>
---
M linux_mtd.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/22505/1
diff --git a/linux_mtd.c b/linux_mtd.c
index d8d835d..c707167 100644
--- a/linux_mtd.c
+++ b/linux_mtd.c
@@ -480,7 +480,7 @@
/* For now, assume only one contiguous region can be locked (NOR) */
/* FIXME: use flash struct members instead of raw MTD values here */
- for (u = 0; u < mtd_total_size / mtd_erasesize; u += mtd_erasesize) {
+ for (u = 0; u < mtd_total_size; u += mtd_erasesize) {
int rc;
struct erase_info_user erase_info = {
.start = u,
--
To view, visit https://review.coreboot.org/22505
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic582bb034c0d2f811813fb6a25696ccddef43e8f
Gerrit-Change-Number: 22505
Gerrit-PatchSet: 1
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-Reviewer: Wei-Ning Huang <wnhuang(a)google.com>
Hello Brian Norris,
I'd like you to do a code review. Please visit
https://review.coreboot.org/22504
to review the following change.
Change subject: MTD write-protect support
......................................................................
MTD write-protect support
** WIP: Do not merge (yet) **
This adds MTD write protect functionality.
Caveats:
- --wp-range and --wp-enable must be used simultaneously
- --wp-disable will clear the chip's block protect range by default.
If the user specifies a range using --wp-range in the same
invocation to flashrom, then that range will be cleared instead.
BUG=chrome-os-partner:40208,chromium:523575
BRANCH=none
TEST=needs testing.
Change-Id: Ifed2ad510a70fe1944f93f48e411769977afb5ca
Signed-off-by: David Hendricks <dhendrix(a)chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/275381
Reviewed-by: Brian Norris <briannorris(a)chromium.org>
---
M linux_mtd.c
1 file changed, 150 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/04/22504/1
diff --git a/linux_mtd.c b/linux_mtd.c
index 064065f..d8d835d 100644
--- a/linux_mtd.c
+++ b/linux_mtd.c
@@ -34,6 +34,9 @@
#include "file.h"
#include "flash.h"
#include "programmer.h"
+#if 0
+#include "writeprotect.h"
+#endif
#define LINUX_DEV_ROOT "/dev"
#define LINUX_MTD_SYSFS_ROOT "/sys/class/mtd"
@@ -49,6 +52,10 @@
static unsigned long int mtd_total_size;
static unsigned long int mtd_numeraseregions;
static unsigned long int mtd_erasesize; /* only valid if numeraseregions is 0 */
+
+#if 0
+static struct wp wp_mtd; /* forward declaration */
+#endif
/* read a string from a sysfs file and sanitize it */
static int read_sysfs_string(const char *filename, char *buf, int len)
@@ -171,6 +178,9 @@
flash->chip->block_erasers[0].eraseblocks[0].size = mtd_erasesize;
flash->chip->block_erasers[0].eraseblocks[0].count =
mtd_total_size / mtd_erasesize;
+#if 0
+ flash->chip->wp = &wp_mtd;
+#endif
return 1;
}
@@ -369,3 +379,143 @@
msg_pdbg("%s: %s\n", __func__, ret == 0 ? "success." : "failed.");
return ret;
}
+
+#if 0
+/*
+ * Write-protect functions.
+ */
+static int mtd_wp_list_ranges(const struct flashchip *flash)
+{
+ /* TODO: implement this */
+ msg_perr("--wp-list is not currently implemented for MTD.\n");
+ return 1;
+}
+
+/*
+ * We only have MEMLOCK to enable write-protection for a particular block,
+ * so we need to do force the user to use --wp-range and --wp-enable
+ * command-line arguments simultaneously. (Fortunately, CrOS factory
+ * installer does this already).
+ *
+ * The --wp-range argument is processed first and will set these variables
+ * which --wp-enable will use afterward.
+ */
+static unsigned int wp_range_start;
+static unsigned int wp_range_len;
+static int wp_set_range_called = 0;
+
+static int mtd_wp_set_range(const struct flashchip *flash,
+ unsigned int start, unsigned int len)
+{
+ wp_range_start = start;
+ wp_range_len = len;
+
+ wp_set_range_called = 1;
+ return 0;
+}
+
+static int mtd_wp_enable_writeprotect(const struct flashchip *flash, enum wp_mode mode)
+{
+ struct erase_info_user entire_chip = {
+ .start = 0,
+ .length = mtd_total_size,
+ };
+ struct erase_info_user desired_range = {
+ .start = wp_range_start,
+ .length = wp_range_len,
+ };
+
+ if (!wp_set_range_called) {
+ msg_perr("For MTD, --wp-range and --wp-enable must be "
+ "used simultaneously.\n");
+ return 1;
+ }
+
+ /*
+ * MTD handles write-protection additively, so whatever new range is
+ * specified is added to the range which is currently protected. To be
+ * consistent with flashrom behavior with other programmer interfaces,
+ * we need to disable the current write protection and then enable
+ * it for the desired range.
+ */
+ if (ioctl(dev_fd, MEMUNLOCK, &entire_chip) == -1) {
+ msg_perr("%s: Failed to disable write-protection, ioctl: %s\n",
+ __func__, strerror(errno));
+ return 1;
+ }
+
+ if (ioctl(dev_fd, MEMLOCK, &desired_range) == -1) {
+ msg_perr("%s: Failed to enable write-protection, ioctl: %s\n",
+ __func__, strerror(errno));
+ return 1;
+ }
+
+ return 0;
+}
+
+static int mtd_wp_disable_writeprotect(const struct flashchip *flash)
+{
+ struct erase_info_user erase_info;
+
+ if (wp_set_range_called) {
+ erase_info.start = wp_range_start;
+ erase_info.length = wp_range_len;
+ } else {
+ erase_info.start = 0;
+ erase_info.length = mtd_total_size;
+ }
+
+ if (ioctl(dev_fd, MEMUNLOCK, &erase_info) == -1) {
+ msg_perr("%s: ioctl: %s\n", __func__, strerror(errno));
+ return 1;
+ }
+
+ return 0;
+}
+
+static int mtd_wp_status(const struct flashchip *flash)
+{
+ uint32_t start = 0, end = 0;
+ unsigned int u;
+
+ /* For now, assume only one contiguous region can be locked (NOR) */
+ /* FIXME: use flash struct members instead of raw MTD values here */
+ for (u = 0; u < mtd_total_size / mtd_erasesize; u += mtd_erasesize) {
+ int rc;
+ struct erase_info_user erase_info = {
+ .start = u,
+ .length = mtd_erasesize,
+ };
+
+ rc = ioctl(dev_fd, MEMISLOCKED, &erase_info);
+ if (rc < 0) {
+ msg_perr("%s: ioctl: %s\n", __func__, strerror(errno));
+ return 1;
+ } else if (rc == 1) {
+ if (!start)
+ start = erase_info.start;
+ } else if (rc == 0) {
+ if (start)
+ end = erase_info.start - 1;
+ }
+
+ if (start && end) {
+ msg_pinfo("WP: write protect range: start=0x%08x, "
+ "len=0x%08x\n", start, end - start);
+ /* TODO: Replace this break with "start = end = 0" if
+ * we want to support non-contiguous locked regions */
+ break;
+ }
+ }
+
+ return 0;
+}
+
+static struct wp wp_mtd = {
+ .list_ranges = mtd_wp_list_ranges,
+ .set_range = mtd_wp_set_range,
+ .enable = mtd_wp_enable_writeprotect,
+ .disable = mtd_wp_disable_writeprotect,
+ .wp_status = mtd_wp_status,
+};
+#endif
--
To view, visit https://review.coreboot.org/22504
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifed2ad510a70fe1944f93f48e411769977afb5ca
Gerrit-Change-Number: 22504
Gerrit-PatchSet: 1
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
Hello Shawn Nematbakhsh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/22503
to review the following change.
Change subject: linux_mtd: Suppress message if NOR device not found
......................................................................
linux_mtd: Suppress message if NOR device not found
This just suppresses a message that might cause confusion for
unsuspecting users.
BUG=none
BRANCH=none
TEST=ran on veyron_mickey, "NOR type device not found" message
no longer appears under normal circumstances.
Signed-off-by: David Hendricks <dhendrix(a)chromium.org>
Change-Id: I76d50847e6c4a431d30ad39bcd3994059679172d
Reviewed-on: https://chromium-review.googlesource.com/302145
Commit-Ready: David Hendricks <dhendrix(a)chromium.org>
Tested-by: David Hendricks <dhendrix(a)chromium.org>
Reviewed-by: Shawn N <shawnn(a)chromium.org>
---
M linux_mtd.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/22503/1
diff --git a/linux_mtd.c b/linux_mtd.c
index 07db2e9..064065f 100644
--- a/linux_mtd.c
+++ b/linux_mtd.c
@@ -276,7 +276,7 @@
tmp = (char *)scanft(LINUX_MTD_SYSFS_ROOT, "type", "nor", 1);
if (!tmp) {
- msg_perr("%s: NOR type device not found.\n", __func__);
+ msg_pdbg("%s: NOR type device not found.\n", __func__);
goto linux_mtd_setup_exit;
}
--
To view, visit https://review.coreboot.org/22503
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I76d50847e6c4a431d30ad39bcd3994059679172d
Gerrit-Change-Number: 22503
Gerrit-PatchSet: 1
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Shawn Nematbakhsh <shawnn(a)chromium.org>
Hello build bot (Jenkins), Brian Norris,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/22494
to look at the new patch set (#2).
Change subject: file: read in entire file (up to 4K) instead of stat()'ing for size
......................................................................
file: read in entire file (up to 4K) instead of stat()'ing for size
We use find_string() to check files in both /proc (e.g.,
/proc/device-tree/...) and /sys (e.g., /sys/bus/spi/devices/...).
find_string() currently tries to iteratively read pieces of these files,
using stat() to get the file size. However, this size is inaccurate on
sysfs, as these files are often dynamically generated, so it reports a
size of PAGE_SIZE (i.e., 4096).
Instead of trying to pre-determine the file size, let's read in the
whole file (up to 4KiB - 1) and do a series of string searches to find
any matching strings. The 4KiB limitation should be OK, since we're only
looking for things like modalias and compatible strings.
As a side effect, this eliminates a lot of unnecessary strlen() calls,
and allows us to again avoid hand-rolling strstr().
BUG=chrome-os-partner:51651
BRANCH=none
TEST=`flashrom --wp-status` doesn't show log spam
TEST=`flashrom --read /tmp/flashX` gives same results before/after
Change-Id: I2d053b18ba624f4ce3ebc1956b9e7945b693b0bc
Signed-off-by: Brian Norris <briannorris(a)chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/335353
Reviewed-by: David Hendricks <dhendrix(a)chromium.org>
---
M file.c
1 file changed, 41 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/94/22494/2
--
To view, visit https://review.coreboot.org/22494
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d053b18ba624f4ce3ebc1956b9e7945b693b0bc
Gerrit-Change-Number: 22494
Gerrit-PatchSet: 2
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Hello build bot (Jenkins), Brian Norris,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/22493
to look at the new patch set (#2).
Change subject: file: add new helper functions to grok FDT nodes
......................................................................
file: add new helper functions to grok FDT nodes
This file is intended to help find SPI devices using the FDT.
Here's how it works:
Under /proc/device-tree there is a node called "aliases" where
each file is a device name with an instance (e.g. "spi0"). The
content of the file is the controller's memory-mapped address.
That info can be used to find the appropriate node directory to
search under /proc/device-tree, where we can recursively search
for "compatible" nodes and scan for the matching user-provided
compatible string.
For SPI devices, we need to look for the matching "compatible"
node and then read the corresponding "reg" node to obtain chip-
select number.
BUG=chrome-os-partner:43453
BRANCH=none
TEST=reads now work on danger without specifying a SPI device
Signed-off-by: David Hendricks <dhendrix(a)chromium.org>
Change-Id: I1f771bcca61de71daa54e358e058723d3fd37079
Reviewed-on: https://chromium-review.googlesource.com/310072
Commit-Ready: David Hendricks <dhendrix(a)chromium.org>
Tested-by: David Hendricks <dhendrix(a)chromium.org>
Reviewed-by: Brian Norris <briannorris(a)chromium.org>
---
M file.c
M file.h
2 files changed, 160 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/22493/2
--
To view, visit https://review.coreboot.org/22493
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f771bcca61de71daa54e358e058723d3fd37079
Gerrit-Change-Number: 22493
Gerrit-PatchSet: 2
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Hello build bot (Jenkins), Brian Norris,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/22492
to look at the new patch set (#2).
Change subject: file: Re-implement file parsing function
......................................................................
file: Re-implement file parsing function
This re-does the original file parsing function to be a little bit
more robust. The old version just copied the file content and did
a string compare. The new version will read the file character-by-
character, comparing with the specified string as we go along, until
a match is found.
BUG=none
BRANCH=none
TEST=Works as expected on veyron_mickey to grok thru sysfs. Also
tested with an upcoming FDT-related patch to search thru procfs.
Signed-off-by: David Hendricks <dhendrix(a)chromium.org>
Change-Id: I6e3811acf352d85c3f43192f87388833a997dd2c
Reviewed-on: https://chromium-review.googlesource.com/310653
Reviewed-by: Brian Norris <briannorris(a)chromium.org>
---
M file.c
1 file changed, 58 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/22492/2
--
To view, visit https://review.coreboot.org/22492
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6e3811acf352d85c3f43192f87388833a997dd2c
Gerrit-Change-Number: 22492
Gerrit-PatchSet: 2
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/22491
to look at the new patch set (#2).
Change subject: avoid unnecessary syscalls in scanft()
......................................................................
avoid unnecessary syscalls in scanft()
This adds a test to see if the path given to scanft() is a symlink
or a directory before calling opendir() and readdir(). This cuts
down on syscalls which can be very slow.
BUG=none
BRANCH=none
TEST="flashrom -p host -i RW_ELOG:/tmp/elog.bin -r -V" ran on pit a
few hundred milliseconds faster (without other optimizations applied)
Change-Id: Ib067cfda1ba3edbc0b1bba604fa5eb57c64501f2
Reviewed-on: https://chromium-review.googlesource.com/176580
Commit-Queue: David Hendricks <dhendrix(a)chromium.org>
Tested-by: David Hendricks <dhendrix(a)chromium.org>
Reviewed-by: Julius Werner <jwerner(a)chromium.org>
---
M file.c
1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/22491/2
--
To view, visit https://review.coreboot.org/22491
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib067cfda1ba3edbc0b1bba604fa5eb57c64501f2
Gerrit-Change-Number: 22491
Gerrit-PatchSet: 2
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>