March 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: writeprotect.c: refactor and fix wp_mode functions
by Nikolai Artemiev (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62643 to look at the new patch set (#6). Change subject: writeprotect.c: refactor and fix wp_mode functions ...................................................................... writeprotect.c: refactor and fix wp_mode functions Use a lookup table in get_wp_mode() and drop the srp_bit_present check, since a chip without SRP is just FLASHROM_WP_MODE_DISABLED. Add a srp_bit_present check to set_wp_mode() if the mode requires it. BUG=b:182223106 BRANCH=none TEST=flashrom --wp-{enable,disable,status} on AMD dut Change-Id: Ib6c347453f9216e5816e4ed35bf9783fd3c720e0 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M writeprotect.c 1 file changed, 13 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/62643/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/62643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6c347453f9216e5816e4ed35bf9783fd3c720e0 Gerrit-Change-Number: 62643 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: libflashrom,linux_mtd: add linux_mtd writeprotect support
by Nikolai Artemiev (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/61897 to look at the new patch set (#33). Change subject: libflashrom,linux_mtd: add linux_mtd writeprotect support ...................................................................... libflashrom,linux_mtd: add linux_mtd writeprotect support BUG=b:182223106 BRANCH=none TEST=WP ops on hana DUT (MT8173) with W25Q32DW flash TEST=flashrom --wp-enable --wp-range <non-empty> succeeds TEST=flashrom --wp-enable --wp-range <empty> fails as expected TEST=flashrom --wp-disable --wp-range <empty> succeeds TEST=flashrom --wp-disable --wp-range <non-empty> fails as expected TEST=flashrom --wp-status succeeds Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M libflashrom.c M linux_mtd.c M programmer.h 3 files changed, 138 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/61897/33 -- To view, visit https://review.coreboot.org/c/flashrom/+/61897 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da Gerrit-Change-Number: 61897 Gerrit-PatchSet: 33 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashchips.c: add writeprotect support for more chips
by Nikolai Artemiev (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62214 to look at the new patch set (#25). Change subject: flashchips.c: add writeprotect support for more chips ...................................................................... flashchips.c: add writeprotect support for more chips Physical chips were tested with an FT232H adapter where possible, others were checked against datasheets. Flashrom Chip name | Chip(s) tested ---------------------------------+---------------------------- AT25SL128A | EN25QH128 | GD25LQ128C/GD25LQ128D/GD25LQ128E | GD25LQ128DSIG GD25LQ64(B) | GD25LQ64CWIG GD25Q127C/GD25Q128C | GD25Q127CSIG, GD25Q128ESIG GD25Q256D | GD25Q256DYIG GD25Q64(B) | GD25Q64CSIG W25Q128.JW.DTR | W25Q128.V..M | W25Q128.W | W25Q256JV_M | W25Q256.V | W25Q64.W | XM25QH128C | XM25QH256C | BUG=b:182223106 BRANCH=none TEST=flashrom --wp-{enable,disable,range,list,status} Change-Id: I7f3d4c4148056098a845b5c64308b0333ebda395 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashchips.c 1 file changed, 138 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/62214/25 -- To view, visit https://review.coreboot.org/c/flashrom/+/62214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7f3d4c4148056098a845b5c64308b0333ebda395 Gerrit-Change-Number: 62214 Gerrit-PatchSet: 25 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: spi25_statusreg: delete read_status_register()
by Nikolai Artemiev (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59529 to look at the new patch set (#64). Change subject: spi25_statusreg: delete read_status_register() ...................................................................... spi25_statusreg: delete read_status_register() BUG=b:195381327,b:153800563 BRANCH=none TEST=flashrom -{r,w,E} Change-Id: I146b4b5439872e66c5d33e156451a729d248c7da Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M chipdrivers.h M it87spi.c M s25f.c M spi25.c M spi25_statusreg.c 5 files changed, 143 insertions(+), 47 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/29/59529/64 -- To view, visit https://review.coreboot.org/c/flashrom/+/59529 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I146b4b5439872e66c5d33e156451a729d248c7da Gerrit-Change-Number: 59529 Gerrit-PatchSet: 64 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: spi25_statusreg: inline spi_write_register_flag()
by Nikolai Artemiev (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Nikolai Artemiev, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59528 to look at the new patch set (#63). Change subject: spi25_statusreg: inline spi_write_register_flag() ...................................................................... spi25_statusreg: inline spi_write_register_flag() Creating the entire SPI command that should be sent to the chip in spi_write_register() is simpler than splitting it across two functions that have to pass multiple parameters between them. Additionally, having separate spi_write_register_flag() function provided little benefit, as it was only ever called from spi_write_register(). BUG=b:195381327,b:153800563 BRANCH=none TEST=flashrom -{r,w,E} TEST=Tested with a W25Q128.W flash on a kasumi (AMD) dut. Read SR1/SR2 with --wp-status and activated various WP ranges that toggled bits in both SR1 and SR2. Change-Id: I4996b0848d0ed09032bad2ab13ab1f40bbfc0304 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M spi25_statusreg.c 1 file changed, 56 insertions(+), 70 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/59528/63 -- To view, visit https://review.coreboot.org/c/flashrom/+/59528 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4996b0848d0ed09032bad2ab13ab1f40bbfc0304 Gerrit-Change-Number: 59528 Gerrit-PatchSet: 63 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: programmer.h: Make pacc global state static local
by Edward O'Callaghan (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk, Peter Marheine. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59278 ) Change subject: programmer.h: Make pacc global state static local ...................................................................... Patch Set 4: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/59278/comment/c0cf11c0_ef1a03f2 PS3, Line 17: flow now. > IMO it becomes harder to read. Is there anything in […] Yes, I finally got a 15 min to spare to rebase it and send it up https://review.coreboot.org/c/flashrom/+/62951 This was the best I could think of at the time to put pcidev all in one place at last. -- To view, visit https://review.coreboot.org/c/flashrom/+/59278 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3bea1b93a7ca81dd42a64eec1e49a10f1fbda850 Gerrit-Change-Number: 59278 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 21 Mar 2022 05:32:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pcidev: Construct a API to handle phantom pci controllers
by Edward O'Callaghan (Code Review) 21 Mar '22

21 Mar '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/62951 ) Change subject: pcidev: Construct a API to handle phantom pci controllers ...................................................................... pcidev: Construct a API to handle phantom pci controllers On Intel the SPI PCI device can habe hiden PCI vendor and device IDs. So we need a PCI access method that works even when the OS doesn't know the PCI device. This however does not generalise since it would bring along other constraints (e.g. on PCI domains, extended PCIe config space). Fortunately the logic exists inside chipset_enable.c however the interaction with the rest of the pcidev workers are quite invasive and hard to reason about. Therefore here we implement a definable API to manage this special casing by having methods define on a handle with a known life-time. BUG=b:220950271 TEST=`sudo ./flashrom -p internal -r /tmp/bios` on Found chipset "Intel Kaby Lake U w/ iHDCP2.2 Prem.". Change-Id: Ic75573a14c9997a2d0347cd683ffc0c78d1b3a4f Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c M pcidev.c M programmer.h 3 files changed, 77 insertions(+), 37 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/62951/1 diff --git a/chipset_enable.c b/chipset_enable.c index 0484299..5e5763e 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -903,9 +903,9 @@ } /* Sunrise Point */ -static int enable_flash_pch100_shutdown(void *const pci_acc) +static int enable_flash_pch100_shutdown(void *const ph_spidev) { - pci_cleanup(pci_acc); + pcidev_phantom_release(ph_spidev); return 0; } @@ -915,31 +915,13 @@ { int ret = ERROR_FATAL; - /* - * The SPI PCI device is usually hidden (by hiding PCI vendor - * and device IDs). So we need a PCI access method that works - * even when the OS doesn't know the PCI device. We can't use - * this method globally since it would bring along other con- - * straints (e.g. on PCI domains, extended PCIe config space). - */ - struct pci_access *const pci_acc = pci_alloc(); - if (!pci_acc) { - msg_perr("Can't allocate PCI accessor.\n"); + struct pcidev_phantom_spi *const ph_spidev = pcidev_phantom_new_spidev(dev, slot, func); + if (!ph_spidev) { + msg_perr("Can't allocate Phanom SPI device.\n"); return ret; } - pci_acc->method = PCI_ACCESS_I386_TYPE1; - pci_init(pci_acc); - register_shutdown(enable_flash_pch100_shutdown, pci_acc); + struct pci_dev *const spi_dev = pcidev_phantom_spidev(ph_spidev); - struct pci_dev *const spi_dev = pci_get_dev(pci_acc, dev->domain, dev->bus, slot, func); - if (!spi_dev) { - msg_perr("Can't allocate PCI device.\n"); - return ret; - } - - /* Modify pacc so the rpci_write can register the undo callback with a - * device using the correct pci_access */ - struct pci_access *const saved_pacc = pcidev_override_pacc(pci_acc); const enum chipbustype boot_buses = 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); @@ -953,6 +935,7 @@ msg_pdbg("SPIBAR = 0x%0*" PRIxPTR " (phys = 0x%08x)\n", PRIxPTR_WIDTH, (uintptr_t)spibar, phys_spibar); /* This adds BUS_SPI */ + register_shutdown(enable_flash_pch100_shutdown, ph_spidev); const int ret_spi = ich_init_spi(spibar, pch_generation); if (ret_spi != ERROR_FATAL) { if (ret_bc || ret_spi) @@ -966,8 +949,8 @@ laptop_ok = 1; _freepci_ret: - pci_free_dev(spi_dev); - pcidev_override_pacc(saved_pacc); + if (ret == ERROR_FATAL) + pcidev_phantom_release(ph_spidev); return ret; } diff --git a/pcidev.c b/pcidev.c index 5673b02..29ae698 100644 --- a/pcidev.c +++ b/pcidev.c @@ -30,11 +30,70 @@ TYPE_UNKNOWN }; -struct pci_access *pcidev_override_pacc(struct pci_access *new_pacc) +struct pcidev_phantom_spi { + struct pci_access *pci_acc_backup; + struct pci_access *pci_acc; + struct pci_dev *pci_dev; +}; + +/** + * @brief The SPI PCI device is usually hidden (by hiding PCI vendor + * and device IDs). So we need a PCI access method that works + * even when the OS doesn't know the PCI device. We can't use + * this method globally since it would bring along other con- + * straints (e.g. on PCI domains, extended PCIe config space). + * + * The returned device handle contains a backup of the previous + * pci_access pointer to restore. + * + * @param pci_dev pointer + * @param pci slot number + * @param pci func number + * @return phantom device handle. + */ +struct pcidev_phantom_spi * pcidev_phantom_new_spidev( + struct pci_dev *const dev, const int slot, const int func) { - struct pci_access *const saved_pacc = pacc; - pacc = new_pacc; - return saved_pacc; + struct pci_access *const pci_acc = pci_alloc(); + if (!pci_acc) { + msg_perr("Can't allocate PCI accessor.\n"); + return NULL; + } + pci_acc->method = PCI_ACCESS_I386_TYPE1; + pci_init(pci_acc); + + struct pci_dev *const spi_dev = pci_get_dev(pci_acc, dev->domain, dev->bus, slot, func); + if (!spi_dev) { + msg_perr("Can't allocate PCI device.\n"); + return NULL; + } + + struct pcidev_phantom_spi *ph_spi = calloc(1, sizeof(struct pcidev_phantom_spi)); + if (!ph_spi) + return NULL; + ph_spi->pci_acc_backup = pacc; + ph_spi->pci_acc = pci_acc; + ph_spi->pci_dev = spi_dev; + + /* Modify pacc so the rpci_write can register the undo callback with a + * device using the correct pci_access */ + pacc = pci_acc; + + return ph_spi; +} + +struct pci_dev * pcidev_phantom_spidev(struct pcidev_phantom_spi *const ph_spidev) +{ + return ph_spidev->pci_dev; +} + +void pcidev_phantom_release(struct pcidev_phantom_spi *const ph_spidev) +{ + pacc = ph_spidev->pci_acc_backup; /* restore pacc */ + + pci_free_dev(ph_spidev->pci_dev); + pci_cleanup(ph_spidev->pci_acc); + free(ph_spidev); } uintptr_t pcidev_readbar(struct pci_dev *dev, int bar) diff --git a/programmer.h b/programmer.h index e744077..8141809 100644 --- a/programmer.h +++ b/programmer.h @@ -128,13 +128,11 @@ struct pci_dev *pcidev_find_vendorclass(uint16_t vendor, uint16_t devclass); struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, uint16_t card_vendor, uint16_t card_device); struct pci_dev *pcidev_find(uint16_t vendor, uint16_t device); -/** - * @brief pcidev_override_pacc() sets a new pacc and returns a backup. - * FIXME: Fix chipset_enable.c life-cycle to remove the need for this override logic. - * @param pci_access pointer to override with. - * @return previous pci_access pointer to restore. - */ -struct pci_access *pcidev_override_pacc(struct pci_access *new_pacc); + +struct pcidev_phantom_spi; +struct pcidev_phantom_spi * pcidev_phantom_new_spidev(struct pci_dev *const dev, const int slot, const int func); +struct pci_dev * pcidev_phantom_spidev(struct pcidev_phantom_spi *const ph_spidev); +void pcidev_phantom_release(struct pcidev_phantom_spi *const ph_spidev); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. -- To view, visit https://review.coreboot.org/c/flashrom/+/62951 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic75573a14c9997a2d0347cd683ffc0c78d1b3a4f Gerrit-Change-Number: 62951 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: programmer.h: Make pacc global state static local
by Edward O'Callaghan (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Peter Marheine. Hello build bot (Jenkins), Nico Huber, Anastasia Klimchuk, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59278 to look at the new patch set (#4). Change subject: programmer.h: Make pacc global state static local ...................................................................... programmer.h: Make pacc global state static local chipset_enable.c has some fragile pci logic to deal with Intel's complexity of finding the SPI controller on the ICH. Here we provide function to allow pushing the scope of pci access global state inwards into a static local of just pcidev. While by no means perfection, this does at least make it easier to reason about 'pacc' control flow now. BUG=none TEST=builds Change-Id: I3bea1b93a7ca81dd42a64eec1e49a10f1fbda850 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c M pcidev.c M programmer.h 3 files changed, 18 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/59278/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/59278 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3bea1b93a7ca81dd42a64eec1e49a10f1fbda850 Gerrit-Change-Number: 59278 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: spi25_statusreg: delete read_status_register()
by Anastasia Klimchuk (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59529 ) Change subject: spi25_statusreg: delete read_status_register() ...................................................................... Patch Set 63: Code-Review+1 (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/59529/comment/ffc37119_7c178cd2 PS63, Line 7: spi25_statusreg: delete read_status_register() Apart from deleting a function, this patch does one more good thing: it propagates error codes (which previously were ignored). This is very worth mentioning in commit message. Something like "As a side effect of using spi_read_register() instead of old function, error codes are propagated and not ignored". And this probably goes after the comment below. https://review.coreboot.org/c/flashrom/+/59529/comment/973b04dc_6c65082e PS63, Line 7: read_status_register Looking at the code, it is `spi_read_status_register()` function which is deleted :) https://review.coreboot.org/c/flashrom/+/59529/comment/1f979a12_b2120d89 PS63, Line 8: Also, it is worth adding one sentence explaining why the function is deleted, something like "The function is no longer needed, since it can be fully replaced by generic spi_read_register()". https://review.coreboot.org/c/flashrom/+/59529/comment/8497c1d7_b743fbe3 PS63, Line 11: flashrom -{r,w,E} Same as for previous patch: maybe a bit more info about chips / or environment where this was tested? File spi25.c: https://review.coreboot.org/c/flashrom/+/59529/comment/4c326302_73c967e1 PS63, Line 307: 500 Maybe I forgot and it was discussed before, but where 500 comes from ? Don't know, but maybe it is worth a comment? -- To view, visit https://review.coreboot.org/c/flashrom/+/59529 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I146b4b5439872e66c5d33e156451a729d248c7da Gerrit-Change-Number: 59529 Gerrit-PatchSet: 63 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 21 Mar 2022 05:23:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg: inline spi_write_register_flag()
by Anastasia Klimchuk (Code Review) 21 Mar '22

21 Mar '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59528 ) Change subject: spi25_statusreg: inline spi_write_register_flag() ...................................................................... Patch Set 62: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/59528/comment/49e02720_c6a621c1 PS62, Line 19: flashrom -{r,w,E} I have one question: to run the code changed by this patch, is any chip fine for that, or should it have specific registers for example? Maybe you can just add info about chips / environment, that would be great! -- To view, visit https://review.coreboot.org/c/flashrom/+/59528 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4996b0848d0ed09032bad2ab13ab1f40bbfc0304 Gerrit-Change-Number: 59528 Gerrit-PatchSet: 62 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Mon, 21 Mar 2022 04:40:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2022