October 2022 - flashrom-gerrit

[S] Change in flashrom[master]: ichspi: Do not attempt to map physical memory for hwseq
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Thomas Heijligen, Jonathon Hall. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68090 ) Change subject: ichspi: Do not attempt to map physical memory for hwseq ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68090 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie698071c3181e988f10b750b0e50c9700efaa1a3 Gerrit-Change-Number: 68090 Gerrit-PatchSet: 3 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Comment-Date: Sat, 08 Oct 2022 18:39:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region to par/spi/opaque_master
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Thomas Heijligen, Jonathon Hall, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master ...................................................................... Patch Set 10: Code-Review+2 (1 comment) File parallel.c: https://review.coreboot.org/c/flashrom/+/67695/comment/662456d9_bc002658 PS10, Line 82: | nit: spacing needed around `|` -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 10 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 08 Oct 2022 18:39:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers/: Make 'fallback_{un}map' the default unless defined
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/67404 ) ( 4 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: drivers/: Make 'fallback_{un}map' the default unless defined ...................................................................... drivers/: Make 'fallback_{un}map' the default unless defined Drop the explicit need to specify the default 'fallback_{un}map' callback function pointer from the 'programmer_entry' struct. This is a reasonable default for every other driver in the tree with only a select few exceptions [atavia, serprog, dummyflasher and internal]. Thus this simplifies driver development and paves way to remove the 'programmer' global handle. Change-Id: I5ea7bd68f7ae2cd4af9902ef07255ab6ce0bfdb3 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/67404 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M atahpt.c M atapromise.c M atavia.c M buspirate_spi.c M ch341a_spi.c M dediprog.c M developerbox_spi.c M digilent_spi.c M drkaiser.c M flashrom.c M ft2232_spi.c M gfxnvidia.c M it8212.c M jlink_spi.c M linux_mtd.c M linux_spi.c M mediatek_i2c_spi.c M mstarddc_spi.c M ni845x_spi.c M nic3com.c M nicintel.c M nicintel_eeprom.c M nicintel_spi.c M nicnatsemi.c M nicrealtek.c M ogp_spi.c M parade_lspcon.c M pickit2_spi.c M pony_spi.c M raiden_debug_spi.c M rayer_spi.c M realtek_mst_i2c_spi.c M satamv.c M satasii.c M serprog.c M stlinkv3_spi.c M usbblaster_spi.c 37 files changed, 33 insertions(+), 80 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Anastasia Klimchuk: Looks good to me, approved diff --git a/atahpt.c b/atahpt.c index fa69137..89b44c2 100644 --- a/atahpt.c +++ b/atahpt.c @@ -120,6 +120,4 @@ .type = PCI, .devs.dev = ata_hpt, .init = atahpt_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/atapromise.c b/atapromise.c index 5de0921..625eb83 100644 --- a/atapromise.c +++ b/atapromise.c @@ -184,6 +184,4 @@ .type = PCI, .devs.dev = ata_promise, .init = atapromise_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/atavia.c b/atavia.c index a0be396..66bca65 100644 --- a/atavia.c +++ b/atavia.c @@ -190,5 +190,4 @@ .devs.dev = ata_via, .init = atavia_init, .map_flash_region = atavia_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/buspirate_spi.c b/buspirate_spi.c index 121c7e5..dbaac4e 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -714,6 +714,4 @@ /* FIXME */ .devs.note = "Dangerous Prototypes Bus Pirate\n", .init = buspirate_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/ch341a_spi.c b/ch341a_spi.c index 48c6420..294a6a6 100644 --- a/ch341a_spi.c +++ b/ch341a_spi.c @@ -525,7 +525,5 @@ .type = USB, .devs.dev = devs_ch341a_spi, .init = ch341a_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, .delay = ch341a_spi_delay, }; diff --git a/dediprog.c b/dediprog.c index 99c91e0..ae9e448 100644 --- a/dediprog.c +++ b/dediprog.c @@ -1319,6 +1319,4 @@ .type = USB, .devs.dev = devs_dediprog, .init = dediprog_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/developerbox_spi.c b/developerbox_spi.c index 04957a3..64b7e8a 100644 --- a/developerbox_spi.c +++ b/developerbox_spi.c @@ -193,6 +193,4 @@ .type = USB, .devs.dev = devs_developerbox_spi, .init = developerbox_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/digilent_spi.c b/digilent_spi.c index 85f3884..6575cbf 100644 --- a/digilent_spi.c +++ b/digilent_spi.c @@ -468,6 +468,4 @@ .type = USB, .devs.dev = devs_digilent_spi, .init = digilent_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/drkaiser.c b/drkaiser.c index 039e686..96f2c99 100644 --- a/drkaiser.c +++ b/drkaiser.c @@ -122,6 +122,4 @@ .type = PCI, .devs.dev = drkaiser_pcidev, .init = drkaiser_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/flashrom.c b/flashrom.c index 3328a71..f74b79a 100644 --- a/flashrom.c +++ b/flashrom.c @@ -205,7 +205,11 @@ void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len) { - void *ret = programmer->map_flash_region(descr, phys_addr, len); + void *ret; + if (programmer->map_flash_region) + ret = programmer->map_flash_region(descr, phys_addr, len); + else + ret = fallback_map(descr, phys_addr, len); msg_gspew("%s: mapping %s from 0x%0*" PRIxPTR " to 0x%0*" PRIxPTR "\n", __func__, descr, PRIxPTR_WIDTH, phys_addr, PRIxPTR_WIDTH, (uintptr_t) ret); return ret; @@ -213,7 +217,10 @@ void programmer_unmap_flash_region(void *virt_addr, size_t len) { - programmer->unmap_flash_region(virt_addr, len); + if (programmer->unmap_flash_region) + programmer->unmap_flash_region(virt_addr, len); + else + fallback_unmap(virt_addr, len); msg_gspew("%s: unmapped 0x%0*" PRIxPTR "\n", __func__, PRIxPTR_WIDTH, (uintptr_t)virt_addr); } @@ -1423,14 +1430,6 @@ msg_gerr("Programmer %s does not have a valid init function!\n", p->name); ret = 1; } - if (p->map_flash_region == NULL) { - msg_gerr("Programmer %s does not have a valid map_flash_region function!\n", p->name); - ret = 1; - } - if (p->unmap_flash_region == NULL) { - msg_gerr("Programmer %s does not have a valid unmap_flash_region function!\n", p->name); - ret = 1; - } } /* It would be favorable if we could check for the correct layout (especially termination) of various diff --git a/ft2232_spi.c b/ft2232_spi.c index 6d7aa6a..aec21bc 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -711,6 +711,4 @@ .type = USB, .devs.dev = devs_ft2232spi, .init = ft2232_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/gfxnvidia.c b/gfxnvidia.c index cab408c..1d484ea 100644 --- a/gfxnvidia.c +++ b/gfxnvidia.c @@ -150,6 +150,4 @@ .type = PCI, .devs.dev = gfx_nvidia, .init = gfxnvidia_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/it8212.c b/it8212.c index 9b4a263..8fe2b59 100644 --- a/it8212.c +++ b/it8212.c @@ -111,6 +111,4 @@ .type = PCI, .devs.dev = devs_it8212, .init = it8212_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/jlink_spi.c b/jlink_spi.c index ef4d9ca..63d15b8 100644 --- a/jlink_spi.c +++ b/jlink_spi.c @@ -536,6 +536,4 @@ .type = OTHER, .init = jlink_spi_init, .devs.note = "SEGGER J-Link and compatible devices\n", - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/linux_mtd.c b/linux_mtd.c index 26244a8..b8aee06 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -557,6 +557,4 @@ .type = OTHER, .devs.note = "Device files /dev/mtd*\n", .init = linux_mtd_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/linux_spi.c b/linux_spi.c index b51d9e9..ee02304 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -251,6 +251,4 @@ .type = OTHER, .devs.note = "Device files /dev/spidev*.*\n", .init = linux_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/mediatek_i2c_spi.c b/mediatek_i2c_spi.c index 727f933..3aa9dd9 100644 --- a/mediatek_i2c_spi.c +++ b/mediatek_i2c_spi.c @@ -542,6 +542,4 @@ .type = OTHER, .devs.note = "Device files /dev/i2c-*\n", .init = mediatek_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/mstarddc_spi.c b/mstarddc_spi.c index 88a4afa..0c11717 100644 --- a/mstarddc_spi.c +++ b/mstarddc_spi.c @@ -255,6 +255,4 @@ .type = OTHER, .devs.note = "MSTAR DDC devices addressable via /dev/i2c-* on Linux.\n", .init = mstarddc_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/ni845x_spi.c b/ni845x_spi.c index b5cfc06..2750334 100644 --- a/ni845x_spi.c +++ b/ni845x_spi.c @@ -635,6 +635,4 @@ .type = OTHER, // choose other because NI-845x uses own USB implementation .devs.note = "National Instruments USB-845x\n", .init = ni845x_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/nic3com.c b/nic3com.c index 8fafef9..efba979 100644 --- a/nic3com.c +++ b/nic3com.c @@ -168,6 +168,4 @@ .type = PCI, .devs.dev = nics_3com, .init = nic3com_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/nicintel.c b/nicintel.c index fa91ff1..47edbb7 100644 --- a/nicintel.c +++ b/nicintel.c @@ -133,6 +133,4 @@ .type = PCI, .devs.dev = nics_intel, .init = nicintel_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/nicintel_eeprom.c b/nicintel_eeprom.c index 01f9075..a254420 100644 --- a/nicintel_eeprom.c +++ b/nicintel_eeprom.c @@ -538,6 +538,4 @@ .type = PCI, .devs.dev = nics_intel_ee, .init = nicintel_ee_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/nicintel_spi.c b/nicintel_spi.c index b99da53..2821d23 100644 --- a/nicintel_spi.c +++ b/nicintel_spi.c @@ -337,6 +337,4 @@ .type = PCI, .devs.dev = nics_intel_spi, .init = nicintel_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/nicnatsemi.c b/nicnatsemi.c index 7236d4d..efa879a 100644 --- a/nicnatsemi.c +++ b/nicnatsemi.c @@ -127,6 +127,4 @@ .type = PCI, .devs.dev = nics_natsemi, .init = nicnatsemi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/nicrealtek.c b/nicrealtek.c index c96f998..103ea9e 100644 --- a/nicrealtek.c +++ b/nicrealtek.c @@ -146,6 +146,4 @@ .type = PCI, .devs.dev = nics_realtek, .init = nicrealtek_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/ogp_spi.c b/ogp_spi.c index 52b1d4e..d85c823 100644 --- a/ogp_spi.c +++ b/ogp_spi.c @@ -178,6 +178,4 @@ .type = PCI, .devs.dev = ogp_spi, .init = ogp_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/parade_lspcon.c b/parade_lspcon.c index 72f7600..a50346e 100644 --- a/parade_lspcon.c +++ b/parade_lspcon.c @@ -507,6 +507,4 @@ .type = OTHER, .devs.note = "Device files /dev/i2c-*.\n", .init = parade_lspcon_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/pickit2_spi.c b/pickit2_spi.c index f550b4f..e8c80af 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -511,6 +511,4 @@ .type = USB, .devs.dev = devs_pickit2_spi, .init = pickit2_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/pony_spi.c b/pony_spi.c index 63359d2..3a85eeb 100644 --- a/pony_spi.c +++ b/pony_spi.c @@ -272,6 +272,4 @@ /* FIXME */ .devs.note = "Programmers compatible with SI-Prog, serbang or AJAWe\n", .init = pony_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 9ded585..a2e5e84 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -1649,6 +1649,4 @@ .type = USB, .devs.dev = devs_raiden, .init = raiden_debug_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/rayer_spi.c b/rayer_spi.c index cd7a066..8fdcc01 100644 --- a/rayer_spi.c +++ b/rayer_spi.c @@ -329,6 +329,4 @@ /* FIXME */ .devs.note = "RayeR parallel port programmer\n", .init = rayer_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 9637eca..2a5d5ed 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -544,6 +544,4 @@ .type = OTHER, .devs.note = "Device files /dev/i2c-*.\n", .init = realtek_mst_i2c_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/satamv.c b/satamv.c index c3831d1..dcd53c7 100644 --- a/satamv.c +++ b/satamv.c @@ -211,6 +211,4 @@ .type = PCI, .devs.dev = satas_mv, .init = satamv_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/satasii.c b/satasii.c index 0f3311f..011562e 100644 --- a/satasii.c +++ b/satasii.c @@ -153,6 +153,4 @@ .type = PCI, .devs.dev = satas_sii, .init = satasii_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/serprog.c b/serprog.c index 57196a9..12d15d7 100644 --- a/serprog.c +++ b/serprog.c @@ -967,6 +967,5 @@ .devs.note = "All programmer devices speaking the serprog protocol\n", .init = serprog_init, .map_flash_region = serprog_map, - .unmap_flash_region = fallback_unmap, .delay = serprog_delay, }; diff --git a/stlinkv3_spi.c b/stlinkv3_spi.c index dcbed6d..9ae3df5 100644 --- a/stlinkv3_spi.c +++ b/stlinkv3_spi.c @@ -556,6 +556,4 @@ .type = USB, .devs.dev = devs_stlinkv3_spi, .init = stlinkv3_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; diff --git a/usbblaster_spi.c b/usbblaster_spi.c index c5914c4..5e2c020 100644 --- a/usbblaster_spi.c +++ b/usbblaster_spi.c @@ -232,6 +232,4 @@ .type = USB, .devs.dev = devs_usbblasterspi, .init = usbblaster_spi_init, - .map_flash_region = fallback_map, - .unmap_flash_region = fallback_unmap, }; -- To view, visit https://review.coreboot.org/c/flashrom/+/67404 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ea7bd68f7ae2cd4af9902ef07255ab6ce0bfdb3 Gerrit-Change-Number: 67404 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: drivers/: Make 'fallback_{un}map' the default unless defined
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67404 ) Change subject: drivers/: Make 'fallback_{un}map' the default unless defined ...................................................................... Patch Set 5: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67404 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ea7bd68f7ae2cd4af9902ef07255ab6ce0bfdb3 Gerrit-Change-Number: 67404 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 08 Oct 2022 18:35:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: rayer_spi.c: Roll up programmer type search logic into func
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Felix Singer, Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68238 ) Change subject: rayer_spi.c: Roll up programmer type search logic into func ...................................................................... Patch Set 2: (2 comments) File rayer_spi.c: https://review.coreboot.org/c/flashrom/+/68238/comment/f2fef1d6_e10ed986 PS2, Line 264: *prog_type = strdup(arg); > As mentioned in CB:68230, calling `get_progtype()` here and having this function "return" the prog t […] CB:68239 does it https://review.coreboot.org/c/flashrom/+/68238/comment/a2ef32c8_bbf03790 PS2, Line 271: static const struct rayer_programmer *find_progtype(const char *prog_type) If you want to keep CB:68239 as a separate commit, could you please move this function above `get_params()`? -- To view, visit https://review.coreboot.org/c/flashrom/+/68238 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id226ea61132ecc30fd8696e1d8ea50373e752cac Gerrit-Change-Number: 68238 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 08 Oct 2022 13:25:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: rayer_spi.c: Drop lpt_outbyte intermediate var
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Felix Singer, Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68232 ) Change subject: rayer_spi.c: Drop lpt_outbyte intermediate var ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Patchset: PS2: Other than the confusing code comment, LGTM -- To view, visit https://review.coreboot.org/c/flashrom/+/68232 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idedabb7b1c401d666b3b7e621e75704c7e765fd1 Gerrit-Change-Number: 68232 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 08 Oct 2022 13:24:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: rayer_spi.c: Move param parse logic into own func
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Felix Singer, Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68230 ) Change subject: rayer_spi.c: Move param parse logic into own func ...................................................................... Patch Set 1: Code-Review+2 (1 comment) File rayer_spi.c: https://review.coreboot.org/c/flashrom/+/68230/comment/e58782f3_efe06263 PS1, Line 292: if (prog_type) { : for (; prog->type != NULL; prog++) { : if (strcasecmp(prog_type, prog->type) == 0) { : break; : } : } : free(prog_type); : : if (prog->type == NULL) { : msg_perr("Error: Invalid device type specified.\n"); : return 1; : } : } > No, actually I didn't push CB:68238 before which made it unclear what I was thinking. However the idea here was to deconflate the parameter parse logic from the programmer type lookup logic into their own respective functions. This makes the init entry-point more linear in its control flow without all the resource management getting in the way. Hopefully the thinking is a little more clear with CB:68238 push please do let me know if not. Just took a look at CB:68238 and it makes sense, it's similar to what CB:67906 and CB:67908 try to achieve for ft2232_spi. The only thing is that parameter parsing could be encapsulated a bit more. We commented on CB:68238 with more concrete implementation details. Actually... We just made CB:68239 to show exactly what we're thinking of. Feel free to integrate it into CB:68238 or keep it as a separate commit. See CB:68238 for discussion. -- To view, visit https://review.coreboot.org/c/flashrom/+/68230 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I287aa2e5d94e872553d08c0750f8dc6d60b9caff Gerrit-Change-Number: 68230 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 08 Oct 2022 13:23:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: rayer_spi.c: Get rid of temporary `prog_type` string
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68239 ) Change subject: rayer_spi.c: Get rid of temporary `prog_type` string ...................................................................... rayer_spi.c: Get rid of temporary `prog_type` string Make the `get_params()` function provide a pointer to `struct rayer_programmer` directly, instead of having a `prog_type` string passed around three functions. Change-Id: I83e34382ee9814f224025e21e5099fdab73cee8c Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M rayer_spi.c 1 file changed, 58 insertions(+), 50 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/68239/1 diff --git a/rayer_spi.c b/rayer_spi.c index 6984099..8ebfb2d 100644 --- a/rayer_spi.c +++ b/rayer_spi.c @@ -225,49 +225,6 @@ .half_period = 0, }; -static int get_params(const struct programmer_cfg *cfg, uint16_t *lpt_iobase, char **prog_type) -{ - /* Pick a default value for the I/O base. */ - *lpt_iobase = 0x378; - /* no programmer type specified. */ - *prog_type = NULL; - - /* Non-default port requested? */ - char *arg = extract_programmer_param_str(cfg, "iobase"); - if (arg) { - char *endptr = NULL; - unsigned long tmp = strtoul(arg, &endptr, 0); - /* Port 0, port >0x10000, unaligned ports and garbage strings - * are rejected. - */ - if (!tmp || (tmp >= 0x10000) || (tmp & 0x3) || - (*endptr != '\0')) { - /* Using ports below 0x100 is a really bad idea, and - * should only be done if no port between 0x100 and - * 0xfffc works due to routing issues. - */ - msg_perr("Error: iobase= specified, but the I/O base " - "given was invalid.\nIt must be a multiple of " - "0x4 and lie between 0x100 and 0xfffc.\n"); - free(arg); - return -1; - } else { - *lpt_iobase = (uint16_t)tmp; - msg_pinfo("Non-default I/O base requested. This will " - "not change the hardware settings.\n"); - } - free(arg); - } - - arg = extract_programmer_param_str(cfg, "type"); - if (arg) { - *prog_type = strdup(arg); - free(arg); - } - - return 0; -} - static const struct rayer_programmer *find_progtype(const char *prog_type) { if (!prog_type) @@ -297,21 +254,58 @@ return prog; } +static int get_params(const struct programmer_cfg *cfg, uint16_t *lpt_iobase, + const struct rayer_programmer **prog) +{ + /* Pick a default value for the I/O base. */ + *lpt_iobase = 0x378; + /* no programmer type specified. */ + *prog = NULL; + + /* Non-default port requested? */ + char *arg = extract_programmer_param_str(cfg, "iobase"); + if (arg) { + char *endptr = NULL; + unsigned long tmp = strtoul(arg, &endptr, 0); + /* Port 0, port >0x10000, unaligned ports and garbage strings + * are rejected. + */ + if (!tmp || (tmp >= 0x10000) || (tmp & 0x3) || + (*endptr != '\0')) { + /* Using ports below 0x100 is a really bad idea, and + * should only be done if no port between 0x100 and + * 0xfffc works due to routing issues. + */ + msg_perr("Error: iobase= specified, but the I/O base " + "given was invalid.\nIt must be a multiple of " + "0x4 and lie between 0x100 and 0xfffc.\n"); + free(arg); + return -1; + } else { + *lpt_iobase = (uint16_t)tmp; + msg_pinfo("Non-default I/O base requested. This will " + "not change the hardware settings.\n"); + } + free(arg); + } + + arg = extract_programmer_param_str(cfg, "type"); + *prog = find_progtype(arg); + free(arg); + + return *prog ? 0 : -1; +} + static int rayer_spi_init(const struct programmer_cfg *cfg) { + const struct rayer_programmer *prog; struct rayer_pinout *pinout = NULL; uint16_t lpt_iobase; - char *prog_type; if (rget_io_perms()) return 1; - if (get_params(cfg, &lpt_iobase, &prog_type) < 0) - return 1; - - const struct rayer_programmer *prog = find_progtype(prog_type); - free(prog_type); - if (!prog) + if (get_params(cfg, &lpt_iobase, &prog) < 0) return 1; msg_pdbg("Using address 0x%x as I/O base for parallel port access.\n", -- To view, visit https://review.coreboot.org/c/flashrom/+/68239 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I83e34382ee9814f224025e21e5099fdab73cee8c Gerrit-Change-Number: 68239 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: rayer_spi.c: Move rget_io_perms() check to pre-amble
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68231 ) Change subject: rayer_spi.c: Move rget_io_perms() check to pre-amble ...................................................................... Patch Set 1: (1 comment) File rayer_spi.c: https://review.coreboot.org/c/flashrom/+/68231/comment/86fbe6dd_a1c2db14 PS1, Line 292: if (get_params(cfg, &lpt_iobase, &prog_type) < 0) > That is a very good question, this maybe subjective patch though so I don't have strong feelings yet […] The three steps seem reasonable, but i) and ii) seem to be in inverse order. For some programmers, the "permissions&&resource requirements" may not be fully known until the configuration has been checked. How about: 1) validate and parse programmer configuration, before doing anything, 2) obtain required permissions and resource requirements, before init'ing, 3) do programmer init. With the ordering suggested above, this little commit isn't needed. -- To view, visit https://review.coreboot.org/c/flashrom/+/68231 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb0a494bd49f6986e9ac230b7b123329493a82bc Gerrit-Change-Number: 68231 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 08 Oct 2022 13:09:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: rayer_spi.c: Roll up programmer type search logic into func
by Angel Pons (Code Review) 08 Oct '22

08 Oct '22

Attention is currently required from: Felix Singer, Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68238 ) Change subject: rayer_spi.c: Roll up programmer type search logic into func ...................................................................... Patch Set 2: Code-Review+1 (3 comments) File rayer_spi.c: https://review.coreboot.org/c/flashrom/+/68238/comment/59faec05_6c4f00ac PS2, Line 264: *prog_type = strdup(arg); As mentioned in CB:68230, calling `get_progtype()` here and having this function "return" the prog type instead avoids the need to `strdup()` the string. You'd need a "pointer to (pointer to (struct rayer_programmer))". https://review.coreboot.org/c/flashrom/+/68238/comment/ee73b1ee_2ccd991f PS2, Line 274: NULL No, this changes behavior. If no programmer type is specified, the original code defaults to the first entry. You'd have to move this after the array and return a pointer to `rayer_spi_types[0]` (the initial value of `prog`). https://review.coreboot.org/c/flashrom/+/68238/comment/2c55c63a_f405019a PS2, Line 276: const struct rayer_programmer rayer_spi_types[] = { Shouldn't this be marked `static`? Not sure if it matters for constants, but if this were to be placed in the stack (it would definitely be if it were not const), things would break. -- To view, visit https://review.coreboot.org/c/flashrom/+/68238 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id226ea61132ecc30fd8696e1d8ea50373e752cac Gerrit-Change-Number: 68238 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 08 Oct 2022 12:46:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2022