Edward O'Callaghan submitted this change.

View Change

Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Edward O'Callaghan: Looks good to me, approved
spi_master: Use new API to register shutdown function

This allows spi masters to register shutdown function in spi_master
struct, which means there is no need to call register_shutdown in init
function, since this call is now a part of register_spi_master.

As a consequence of using new API, two things are happening here:
1) No resource leakage anymore in case register_shutdown() would fail,
2) Fixed propagation of register_spi_master() return values.

Basic testing: when I comment out free(data) in linux_spi_shutdown, test
fails with error
../linux_spi.c:235: note: block 0x55a4db276510 allocated here
ERROR: linux_spi_init_and_shutdown_test_success leaked 1 block(s)
Means, shutdown function is invoked.

BUG=b:185191942
TEST= 1) builds and ninja test including CB:56911
2) On ARMv7 device
flashrom -p linux_spi -V
-> using linux_spi, chip found
3) On x86_64 AMD device
flashrom -p internal -V
-> this is actually using sb600spi, chip found

Change-Id: Ib60300f9ddb295a255d5ef3f8da0e07064207140
Signed-off-by: Anastasia Klimchuk <aklm@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/56103
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Nico Huber <nico.h@gmx.de>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
---
M bitbang_spi.c
M buspirate_spi.c
M ch341a_spi.c
M dediprog.c
M digilent_spi.c
M ene_lpc.c
M ft2232_spi.c
M it85spi.c
M it87spi.c
M jlink_spi.c
M linux_spi.c
M lspcon_i2c_spi.c
M mec1308.c
M mstarddc_spi.c
M ni845x_spi.c
M pickit2_spi.c
M realtek_mst_i2c_spi.c
M sb600spi.c
M stlinkv3_spi.c
M usbblaster_spi.c
M wbsio_spi.c
21 files changed, 40 insertions(+), 112 deletions(-)

diff --git a/bitbang_spi.c b/bitbang_spi.c
index 83c3501..7c9a04d 100644
--- a/bitbang_spi.c
+++ b/bitbang_spi.c
@@ -147,6 +147,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = bitbang_spi_shutdown,
};

int register_spi_bitbang_master(const struct bitbang_spi_master *master, void *spi_data)
@@ -168,7 +169,6 @@
data->spi_data = spi_data;

register_spi_master(&mst, data);
- register_shutdown(bitbang_spi_shutdown, data);

/* Only mess with the bus if we're sure nobody else uses it. */
bitbang_spi_request_bus(master, spi_data);
diff --git a/buspirate_spi.c b/buspirate_spi.c
index 3c0a6ff..6d7fba7 100644
--- a/buspirate_spi.c
+++ b/buspirate_spi.c
@@ -181,6 +181,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = buspirate_spi_shutdown,
};

static const struct buspirate_speeds spispeeds[] = {
@@ -698,13 +699,7 @@
goto init_err_cleanup_exit;
}

- if (register_shutdown(buspirate_spi_shutdown, bp_data) != 0) {
- ret = 1;
- goto init_err_cleanup_exit;
- }
- register_spi_master(&spi_master_buspirate, bp_data);
-
- return 0;
+ return register_spi_master(&spi_master_buspirate, bp_data);

init_err_cleanup_exit:
buspirate_spi_shutdown(bp_data);
diff --git a/ch341a_spi.c b/ch341a_spi.c
index 22c9781..4a76286 100644
--- a/ch341a_spi.c
+++ b/ch341a_spi.c
@@ -417,6 +417,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = ch341a_spi_shutdown,
};

static int ch341a_spi_init(void)
@@ -505,10 +506,7 @@
if ((config_stream(CH341A_STM_I2C_100K) < 0) || (enable_pins(true) < 0))
goto dealloc_transfers;

- register_shutdown(ch341a_spi_shutdown, NULL);
- register_spi_master(&spi_master_ch341a_spi, NULL);
-
- return 0;
+ return register_spi_master(&spi_master_ch341a_spi, NULL);

dealloc_transfers:
for (i = 0; i < USB_IN_TRANSFERS; i++) {
diff --git a/dediprog.c b/dediprog.c
index d98af49..323dcd0 100644
--- a/dediprog.c
+++ b/dediprog.c
@@ -1019,6 +1019,7 @@
.read = dediprog_spi_read,
.write_256 = dediprog_spi_write_256,
.write_aai = dediprog_spi_write_aai,
+ .shutdown = dediprog_shutdown,
};

/*
@@ -1271,9 +1272,6 @@
if (protocol() >= PROTOCOL_V2)
spi_master_dediprog.features |= SPI_MASTER_4BA;

- if (register_shutdown(dediprog_shutdown, NULL))
- goto init_err_cleanup_exit;
-
if (register_spi_master(&spi_master_dediprog, NULL) || dediprog_set_leds(LED_NONE))
return 1; /* shutdown function does cleanup */

diff --git a/digilent_spi.c b/digilent_spi.c
index 0ec402e..e2cfcce 100644
--- a/digilent_spi.c
+++ b/digilent_spi.c
@@ -337,6 +337,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = digilent_spi_shutdown,
};

static bool default_reset(struct libusb_device_handle *handle)
@@ -454,10 +455,7 @@
digilent_data->reset_board = reset_board;
digilent_data->handle = handle;

- register_shutdown(digilent_spi_shutdown, digilent_data);
- register_spi_master(&spi_master_digilent_spi, digilent_data);
-
- return 0;
+ return register_spi_master(&spi_master_digilent_spi, digilent_data);

close_handle:
libusb_close(handle);
diff --git a/ene_lpc.c b/ene_lpc.c
index 23ba12b..374d1fd 100644
--- a/ene_lpc.c
+++ b/ene_lpc.c
@@ -512,6 +512,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = ene_leave_flash_mode,
};

static int check_params(void)
@@ -570,16 +571,7 @@

internal_buses_supported |= BUS_LPC;

- if (register_shutdown(ene_leave_flash_mode, ctx_data))
- goto init_err_cleanup_exit;
- register_spi_master(&spi_master_ene, ctx_data);
- msg_pdbg("%s: successfully initialized ene\n", __func__);
-
- return 0;
-
-init_err_cleanup_exit:
- ene_leave_flash_mode(ctx_data);
- return 1;
+ return register_spi_master(&spi_master_ene, ctx_data);

init_err_exit:
free(ctx_data);
diff --git a/ft2232_spi.c b/ft2232_spi.c
index e32d7f8..15837d9 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -291,6 +291,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = ft2232_shutdown,
};

/* Returns 0 upon success, a negative number upon errors. */
@@ -579,13 +580,7 @@
spi_data->pindir = pindir;
spi_data->ftdic_context = ftdic;

- if (register_shutdown(ft2232_shutdown, spi_data)) {
- free(spi_data);
- goto ftdi_err;
- }
- register_spi_master(&spi_master_ft2232, spi_data);
-
- return 0;
+ return register_spi_master(&spi_master_ft2232, spi_data);

ftdi_err:
if ((f = ftdi_usb_close(&ftdic)) < 0) {
diff --git a/it85spi.c b/it85spi.c
index 9812fc7..dd0cb6c 100644
--- a/it85spi.c
+++ b/it85spi.c
@@ -289,6 +289,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = it85xx_shutdown,
};

int it85xx_spi_init(struct superio s)
@@ -353,20 +354,13 @@
data->ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */
data->ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */

- if (register_shutdown(it85xx_shutdown, data)) {
- free(data);
- return 1;
- }
-
/* FIXME: Really leave FWH enabled? We can't use this region
* anymore since accessing it would mess up IT85 communication.
* If we decide to disable FWH for this region, we should print
* a debug message about it.
*/
/* Set this as SPI controller. */
- register_spi_master(&spi_master_it85xx, data);
-
- return 0;
+ return register_spi_master(&spi_master_it85xx, data);
}

#endif
diff --git a/it87spi.c b/it87spi.c
index e552328..3e422d7 100644
--- a/it87spi.c
+++ b/it87spi.c
@@ -304,6 +304,7 @@
.read = it8716f_spi_chip_read,
.write_256 = it8716f_spi_chip_write_256,
.write_aai = spi_chip_write_1,
+ .shutdown = it8716f_shutdown,
};

static uint16_t it87spi_probe(uint16_t port)
@@ -419,13 +420,10 @@
data->flashport = flashport;
data->fast_spi = 1;

- register_shutdown(it8716f_shutdown, data);
-
if (internal_buses_supported & BUS_SPI)
msg_pdbg("Overriding chipset SPI with IT87 SPI.\n");
/* FIXME: Add the SPI bus or replace the other buses with it? */
- register_spi_master(&spi_master_it87xx, data);
- return 0;
+ return register_spi_master(&spi_master_it87xx, data);
}

int init_superio_ite(void)
diff --git a/jlink_spi.c b/jlink_spi.c
index daa8eb6..4b34bfd 100644
--- a/jlink_spi.c
+++ b/jlink_spi.c
@@ -179,6 +179,7 @@
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
.features = SPI_MASTER_4BA,
+ .shutdown = jlink_spi_shutdown,
};

static int jlink_spi_init(void)
@@ -468,11 +469,7 @@
if (!deassert_cs(jlink_data))
goto init_err;

- if (register_shutdown(jlink_spi_shutdown, jlink_data))
- goto init_err;
- register_spi_master(&spi_master_jlink_spi, jlink_data);
-
- return 0;
+ return register_spi_master(&spi_master_jlink_spi, jlink_data);

init_err:
if (jaylink_devh)
diff --git a/linux_spi.c b/linux_spi.c
index 46779a0..bd9ffb3 100644
--- a/linux_spi.c
+++ b/linux_spi.c
@@ -122,6 +122,7 @@
.read = linux_spi_read,
.write_256 = linux_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = linux_spi_shutdown,
};

/* Read max buffer size from sysfs, or use page size as fallback. */
@@ -239,12 +240,7 @@
spi_data->fd = fd;
spi_data->max_kernel_buf_size = max_kernel_buf_size;

- if (register_shutdown(linux_spi_shutdown, spi_data)) {
- free(spi_data);
- goto init_err;
- }
- register_spi_master(&spi_master_linux, spi_data);
- return 0;
+ return register_spi_master(&spi_master_linux, spi_data);

init_err:
close(fd);
diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c
index e9ff2dd..bed7efc 100644
--- a/lspcon_i2c_spi.c
+++ b/lspcon_i2c_spi.c
@@ -434,6 +434,7 @@
.read = lspcon_i2c_spi_read,
.write_256 = lspcon_i2c_spi_write_256,
.write_aai = lspcon_i2c_spi_write_aai,
+ .shutdown = lspcon_i2c_spi_shutdown,
};

static int lspcon_i2c_spi_init(void)
@@ -458,10 +459,7 @@

data->fd = fd;

- ret |= register_shutdown(lspcon_i2c_spi_shutdown, data);
- ret |= register_spi_master(&spi_master_i2c_lspcon, data);
-
- return ret;
+ return register_spi_master(&spi_master_i2c_lspcon, data);
}

const struct programmer_entry programmer_lspcon_i2c_spi = {
diff --git a/mec1308.c b/mec1308.c
index c085ff2..e9c6214 100644
--- a/mec1308.c
+++ b/mec1308.c
@@ -405,6 +405,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = mec1308_shutdown,
};

static int check_params(void)
@@ -507,12 +508,7 @@

internal_buses_supported |= BUS_LPC; /* for LPC <--> SPI bridging */

- if (register_shutdown(mec1308_shutdown, ctx_data))
- goto init_err_cleanup_exit;
- register_spi_master(&spi_master_mec1308, ctx_data);
- msg_pdbg("%s(): successfully initialized mec1308\n", __func__);
-
- return 0;
+ return register_spi_master(&spi_master_mec1308, ctx_data);

init_err_cleanup_exit:
mec1308_shutdown(ctx_data);
diff --git a/mstarddc_spi.c b/mstarddc_spi.c
index a7816a4..8e2d5a8 100644
--- a/mstarddc_spi.c
+++ b/mstarddc_spi.c
@@ -147,6 +147,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = mstarddc_spi_shutdown,
};

/* Returns 0 upon success, a negative number upon errors. */
@@ -240,9 +241,6 @@
mstarddc_data->addr = mstarddc_addr;
mstarddc_data->doreset = mstarddc_doreset;

- // Register shutdown function
- register_shutdown(mstarddc_spi_shutdown, mstarddc_data);
-
// Register programmer
register_spi_master(&spi_master_mstarddc, mstarddc_data);
out:
diff --git a/ni845x_spi.c b/ni845x_spi.c
index 2153510..87a1dd8 100644
--- a/ni845x_spi.c
+++ b/ni845x_spi.c
@@ -536,6 +536,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = ni845x_spi_shutdown,
};

static int ni845x_spi_init(void)
@@ -625,14 +626,7 @@
return 1;
}

- if (register_shutdown(ni845x_spi_shutdown, NULL)) {
- ni845x_spi_shutdown(NULL);
- return 1;
- }
-
- register_spi_master(&spi_programmer_ni845x, NULL);
-
- return 0;
+ return register_spi_master(&spi_programmer_ni845x, NULL);
}

const struct programmer_entry programmer_ni845x_spi = {
diff --git a/pickit2_spi.c b/pickit2_spi.c
index 73ace4b..17da1d9 100644
--- a/pickit2_spi.c
+++ b/pickit2_spi.c
@@ -386,6 +386,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = pickit2_shutdown,
};

static int pickit2_spi_init(void)
@@ -498,11 +499,7 @@
goto init_err_cleanup_exit;
}

- if (register_shutdown(pickit2_shutdown, pickit2_data))
- goto init_err_cleanup_exit;
- register_spi_master(&spi_master_pickit2, pickit2_data);
-
- return 0;
+ return register_spi_master(&spi_master_pickit2, pickit2_data);

init_err_cleanup_exit:
pickit2_shutdown(pickit2_data);
diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 4145864..954150d 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -440,6 +440,7 @@
.read = realtek_mst_i2c_spi_read,
.write_256 = realtek_mst_i2c_spi_write_256,
.write_aai = realtek_mst_i2c_spi_write_aai,
+ .shutdown = realtek_mst_i2c_spi_shutdown,
};

static int get_params(int *reset, int *enter_isp)
@@ -512,10 +513,7 @@

data->fd = fd;
data->reset = reset;
- ret |= register_shutdown(realtek_mst_i2c_spi_shutdown, data);
- ret |= register_spi_master(&spi_master_i2c_realtek_mst, data);
-
- return ret;
+ return register_spi_master(&spi_master_i2c_realtek_mst, data);
}

const struct programmer_entry programmer_realtek_mst_i2c_spi = {
diff --git a/sb600spi.c b/sb600spi.c
index 5c65475..fe7920f 100644
--- a/sb600spi.c
+++ b/sb600spi.c
@@ -589,6 +589,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = sb600spi_shutdown,
};

static const struct spi_master spi_master_yangtze = {
@@ -599,6 +600,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = sb600spi_shutdown,
};

static const struct spi_master spi_master_promontory = {
@@ -609,6 +611,7 @@
.read = promontory_read_memmapped,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = sb600spi_shutdown,
};

int sb600_probe_spi(struct pci_dev *dev)
@@ -786,8 +789,6 @@
data->flash = NULL;
data->spibar = sb600_spibar;

- register_shutdown(sb600spi_shutdown, data);
-
/* Starting with Yangtze the SPI controller got a different interface with a much bigger buffer. */
if (amd_gen < CHIPSET_YANGTZE)
register_spi_master(&spi_master_sb600, data);
diff --git a/stlinkv3_spi.c b/stlinkv3_spi.c
index 3a5aeb3..58e7a41 100644
--- a/stlinkv3_spi.c
+++ b/stlinkv3_spi.c
@@ -467,6 +467,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = stlinkv3_spi_shutdown,
};

static int stlinkv3_spi_init(void)
@@ -531,17 +532,7 @@
stlinkv3_data->usb_ctx = usb_ctx;
stlinkv3_data->handle = stlinkv3_handle;

- if (register_shutdown(stlinkv3_spi_shutdown, stlinkv3_data))
- goto init_err_cleanup_exit;
-
- if (register_spi_master(&spi_programmer_stlinkv3, stlinkv3_data))
- return 1; /* shutdown function does cleanup */
-
- return 0;
-
-init_err_cleanup_exit:
- stlinkv3_spi_shutdown(stlinkv3_data);
- return 1;
+ return register_spi_master(&spi_programmer_stlinkv3, stlinkv3_data);

init_err_exit:
if (stlinkv3_handle)
diff --git a/usbblaster_spi.c b/usbblaster_spi.c
index dfd98ec..bd4b1c9 100644
--- a/usbblaster_spi.c
+++ b/usbblaster_spi.c
@@ -174,6 +174,7 @@
.read = default_spi_read,
.write_256 = default_spi_write_256,
.write_aai = default_spi_write_aai,
+ .shutdown = usbblaster_shutdown,
};

/* Returns 0 upon success, a negative number upon errors. */
@@ -224,12 +225,7 @@
}
usbblaster_data->ftdic = ftdic;

- if (register_shutdown(usbblaster_shutdown, usbblaster_data)) {
- free(usbblaster_data);
- return -1;
- }
- register_spi_master(&spi_master_usbblaster, usbblaster_data);
- return 0;
+ return register_spi_master(&spi_master_usbblaster, usbblaster_data);
}

const struct programmer_entry programmer_usbblaster_spi = {
diff --git a/wbsio_spi.c b/wbsio_spi.c
index 2336845..a175b21 100644
--- a/wbsio_spi.c
+++ b/wbsio_spi.c
@@ -191,6 +191,7 @@
.read = wbsio_spi_read,
.write_256 = spi_chip_write_1,
.write_aai = spi_chip_write_1,
+ .shutdown = wbsio_spi_shutdown,
};

int wbsio_check_for_spi(void)
@@ -214,10 +215,7 @@
}
data->spibase = wbsio_spibase;

- register_shutdown(wbsio_spi_shutdown, data);
- register_spi_master(&spi_master_wbsio, data);
-
- return 0;
+ return register_spi_master(&spi_master_wbsio, data);
}

#endif /* defined(__i386__) || defined(__x86_64__) */

To view, visit change 56103. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib60300f9ddb295a255d5ef3f8da0e07064207140
Gerrit-Change-Number: 56103
Gerrit-PatchSet: 8
Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev@gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h@gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src@posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter@mailbox.org>
Gerrit-MessageType: merged