David Hendricks has posted comments on this change. ( https://review.coreboot.org/20513 )
Change subject: 4BA: Basic support for 4-bytes addressing mode extensions
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Either way is fine with me. If you have time and can edit this patch, I'll be happy to re-review and ack. There might be style issues in the follow-up patches, so perhaps doing a single !fixup would be the easiest/quickest thing to do here.
https://review.coreboot.org/#/c/20513/1/spi4ba.c
File spi4ba.c:
https://review.coreboot.org/#/c/20513/1/spi4ba.c@127
PS1, Line 127: + 1) - 1
> > I'd eventually like to switch to using dynamically allocated memory
That sounds good to me as well.
--
To view, visit https://review.coreboot.org/20513
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie72e2a89cd75fb4d09f48e81c4c1d927c317b7a7
Gerrit-Change-Number: 20513
Gerrit-PatchSet: 2
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: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 14 Aug 2017 06:50:13 +0000
Gerrit-HasComments: Yes
Nico Huber has posted comments on this change. ( https://review.coreboot.org/20513 )
Change subject: 4BA: Basic support for 4-bytes addressing mode extensions
......................................................................
Patch Set 2: Code-Review+1
(12 comments)
Just nits, style and non-functional issues we can also handle in a
!fixup, I suppose. I can do that if nobody beats me to it. Or shall
I edit this change and you ack it again?
https://review.coreboot.org/#/c/20513/2/flash.h
File flash.h:
https://review.coreboot.org/#/c/20513/2/flash.h@124
PS2, Line 124: #define FEATURE_4BA_ONLY (1 << 11)
nit, doesn't align well
https://review.coreboot.org/#/c/20513/1/spi4ba.c
File spi4ba.c:
https://review.coreboot.org/#/c/20513/1/spi4ba.c@127
PS1, Line 127: + 1) - 1
> I'd eventually like to switch to using dynamically allocated memory
> so we don't duplicate all this code between 3BA and 4BA versions. I
> implemented that in a pending patch for the CrOS branch:
> https://chromium-review.googlesource.com/c/337202/1 . But that can
> happen later if desired.
Actually we don't need dynamically allocated memory, just the offset
for the memcpy() would have to be adjusted (and the buffer has to big
enough ofc).
But some abstraction like your new_spi_write_command() also crossed
my mind during this review.
https://review.coreboot.org/#/c/20513/2/spi4ba.c
File spi4ba.c:
https://review.coreboot.org/#/c/20513/2/spi4ba.c@46
PS2, Line 46:
nit, spurious space
https://review.coreboot.org/#/c/20513/2/spi4ba.c@74
PS2, Line 74:
same here
https://review.coreboot.org/#/c/20513/2/spi4ba.c@77
PS2, Line 77: msg_cerr("%s failed during command execution\n", __func__);
Don't know the coding style very well, in coreboot a single-
line body shouldn't have braces.
https://review.coreboot.org/#/c/20513/2/spi4ba.c@84
PS2, Line 84: uint8_t databyte)
Inconsistent alignment, sometimes two tabs are used,
sometimes tabs+spaces, here tabs only.
https://review.coreboot.org/#/c/20513/2/spi4ba.c@100
PS2, Line 100: (addr & 0xff),
`>> 0` used in other places
https://review.coreboot.org/#/c/20513/2/spi4ba.c@127
PS2, Line 127: + 1) - 1
I don't get what the +1-1 should tell the reader?
Ok, guess now I understand it:
+1 for the 4th address byte
-1 for the single byte that is included in the 256
Would be more clear if we'd define something like
JEDEC_4_BYTE_ADDR_BYTE_PROGRAM_OUTSIZE (6) and
JEDEC_4_BYTE_ADDR_PROGRAM_OUTSIZE (5).
(This is all ugly shit, IMHO. But not due to this patch but the
spi25 code that obviously was used as a template. The size of the
static parts of the command should be implicitly given by the data,
IMO. Currently the code suffers from redundancy that makes it har-
der to read, i.e. I trust the compiler to do the calculations but
not developers.)
https://review.coreboot.org/#/c/20513/2/spi4ba.c@175
PS2, Line 175: uint8_t *bytes, unsigned int len)
alignment
https://review.coreboot.org/#/c/20513/2/spi4ba.c@193
PS2, Line 193: unsigned int blocklen)
weirdest alignment
https://review.coreboot.org/#/c/20513/2/spi4ba.c@237
PS2, Line 237: 32
Is this specified somewhere? IIRC, for 3BA it was inconsistent
across chips.
https://review.coreboot.org/#/c/20513/2/spi4ba.c@327
PS2, Line 327: }
flashrom the project of redundancy? I just reviewed the same
function three times...
--
To view, visit https://review.coreboot.org/20513
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie72e2a89cd75fb4d09f48e81c4c1d927c317b7a7
Gerrit-Change-Number: 20513
Gerrit-PatchSet: 2
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: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 10 Aug 2017 21:35:34 +0000
Gerrit-HasComments: Yes
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/20784 )
Change subject: rpci: Use pci_dev struct pointer to avoid API breaks
......................................................................
rpci: Use pci_dev struct pointer to avoid API breaks
The pci_dev structure is never meant to be used as is, but always as a
pointer. By using the struct itself in undo_pci_write_data, we are risking
data corruption, or buffer overflows if the structure size changes.
This is especially apparent on my system where flashrom segfaults
because I compile it with pciutils 3.3.0 and I run it on a system
with pciutils 3.5.2. The struture size is different and causes a
struct with the wrong size to be sent to the library, with invalid
internal field values.
This has been discovered and discussed in Change ID 18925 [1]
[1] https://review.coreboot.org/#/c/18925/
Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298
Signed-off-by: Youness Alaoui <kakaroto(a)kakaroto.homelinux.net>
Reviewed-on: https://review.coreboot.org/20784
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
---
M chipset_enable.c
M pcidev.c
M programmer.h
3 files changed, 25 insertions(+), 9 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
diff --git a/chipset_enable.c b/chipset_enable.c
index 20d2662..6a93d0d 100644
--- a/chipset_enable.c
+++ b/chipset_enable.c
@@ -843,6 +843,7 @@
* straints (e.g. on PCI domains, extended PCIe config space).
*/
struct pci_access *const pci_acc = pci_alloc();
+ struct pci_access *const saved_pacc = pacc;
if (!pci_acc) {
msg_perr("Can't allocate PCI accessor.\n");
return ret;
@@ -857,6 +858,9 @@
return ret;
}
+ /* Modify pacc so the rpci_write can register the undo callback with a
+ * device using the correct pci_access */
+ pacc = pci_acc;
enable_flash_ich_report_gcs(spi_dev, pch_generation, NULL);
const int ret_bc = enable_flash_ich_bios_cntl_config_space(spi_dev, pch_generation, 0xdc);
@@ -880,6 +884,7 @@
_freepci_ret:
pci_free_dev(spi_dev);
+ pacc = saved_pacc;
return ret;
}
diff --git a/pcidev.c b/pcidev.c
index 2c78063..f4e5542 100644
--- a/pcidev.c
+++ b/pcidev.c
@@ -160,6 +160,7 @@
return 1;
}
pci_cleanup(pacc);
+ pacc = NULL;
return 0;
}
@@ -259,7 +260,7 @@
};
struct undo_pci_write_data {
- struct pci_dev dev;
+ struct pci_dev *dev;
int reg;
enum pci_write_type type;
union {
@@ -272,22 +273,23 @@
int undo_pci_write(void *p)
{
struct undo_pci_write_data *data = p;
- if (pacc == NULL) {
- msg_perr("%s: Tried to undo PCI writes without a valid PCI context!\n"
- "Please report a bug at flashrom(a)flashrom.org\n", __func__);
+ if (pacc == NULL || data->dev == NULL) {
+ msg_perr("%s: Tried to undo PCI writes without a valid PCI %s!\n"
+ "Please report a bug at flashrom(a)flashrom.org\n",
+ __func__, data->dev == NULL ? "device" : "context");
return 1;
}
msg_pdbg("Restoring PCI config space for %02x:%02x:%01x reg 0x%02x\n",
- data->dev.bus, data->dev.dev, data->dev.func, data->reg);
+ data->dev->bus, data->dev->dev, data->dev->func, data->reg);
switch (data->type) {
case pci_write_type_byte:
- pci_write_byte(&data->dev, data->reg, data->bytedata);
+ pci_write_byte(data->dev, data->reg, data->bytedata);
break;
case pci_write_type_word:
- pci_write_word(&data->dev, data->reg, data->worddata);
+ pci_write_word(data->dev, data->reg, data->worddata);
break;
case pci_write_type_long:
- pci_write_long(&data->dev, data->reg, data->longdata);
+ pci_write_long(data->dev, data->reg, data->longdata);
break;
}
/* p was allocated in register_undo_pci_write. */
@@ -303,7 +305,11 @@
msg_gerr("Out of memory!\n"); \
exit(1); \
} \
- undo_pci_write_data->dev = *a; \
+ if (pacc) \
+ undo_pci_write_data->dev = pci_get_dev(pacc, \
+ a->domain, a->bus, a->dev, a->func); \
+ else \
+ undo_pci_write_data->dev = NULL; \
undo_pci_write_data->reg = b; \
undo_pci_write_data->type = pci_write_type_##c; \
undo_pci_write_data->c##data = pci_read_##c(dev, reg); \
diff --git a/programmer.h b/programmer.h
index ec00bd9..e58fd32 100644
--- a/programmer.h
+++ b/programmer.h
@@ -195,6 +195,11 @@
struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar);
/* rpci_write_* are reversible writes. The original PCI config space register
* contents will be restored on shutdown.
+ * To clone the pci_dev instances internally, the `pacc` global
+ * variable has to reference a pci_access method that is compatible
+ * with the given pci_dev handle. The referenced pci_access (not
+ * the variable) has to stay valid until the shutdown handlers are
+ * finished.
*/
int rpci_write_byte(struct pci_dev *dev, int reg, uint8_t data);
int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data);
--
To view, visit https://review.coreboot.org/20784
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: merged
Gerrit-Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298
Gerrit-Change-Number: 20784
Gerrit-PatchSet: 5
Gerrit-Owner: Youness Alaoui <snifikino(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: Youness Alaoui <snifikino(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>