From f26740c61a57f1a1556f79a49c6863479fe5aa6b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:48:13 +0200 Subject: smbus: Fix spd_data_generate() error API violation The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. spd_data_generate() can pass @errp to error_setg() more than once when it adjusts both memory size and type. Harmless, because no caller passes anything that needs adjusting. Until the previous commit, sam460ex passed types that needed adjusting, but not sizes. spd_data_generate()'s contract is rather awkward: If everything's fine, return non-null and don't set an error. Else, if memory size or type need adjusting, return non-null and set an error describing the adjustment. Else, return null and set an error reporting why no data can be generated. Its callers treat the error as a warning even when null is returned. They don't create the "smbus-eeprom" device then. Suspicious. Since the previous commit, only "everything's fine" can actually happen. Drop the unused code and simplify the callers. This gets rid of the error API violation. Signed-off-by: Markus Armbruster Message-Id: <20200422134815.1584-3-armbru@redhat.com> --- hw/i2c/smbus_eeprom.c | 30 ++++-------------------------- 1 file changed, 4 insertions(+), 26 deletions(-) (limited to 'hw/i2c') diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index 5adf3b15b5..07fbbf87f1 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom, } /* Generate SDRAM SPD EEPROM data describing a module of type and size */ -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, - Error **errp) +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size) { uint8_t *spd; uint8_t nbanks; @@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, g_assert_not_reached(); } size = ram_size >> 20; /* work in terms of megabytes */ - if (size < 4) { - error_setg(errp, "SDRAM size is too small"); - return NULL; - } sz_log2 = 31 - clz32(size); size = 1U << sz_log2; - if (ram_size > size * MiB) { - error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, " - "truncating to %u MB", ram_size, size); - } - if (sz_log2 < min_log2) { - error_setg(errp, - "Memory size is too small for SDRAM type, adjusting type"); - if (size >= 32) { - type = DDR; - min_log2 = 5; - max_log2 = 12; - } else { - type = SDR; - min_log2 = 2; - max_log2 = 9; - } - } + assert(ram_size == size * MiB); + assert(sz_log2 >= min_log2); nbanks = 1; while (sz_log2 > max_log2 && nbanks < 8) { @@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size, nbanks++; } - if (size > (1ULL << sz_log2) * nbanks) { - error_setg(errp, "Memory size is too big for SDRAM, truncating"); - } + assert(size == (1ULL << sz_log2) * nbanks); /* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */ if (nbanks == 1 && sz_log2 > min_log2) { -- cgit v1.2.3 From 32c82f0eaf9919cb0268d18d86a94bfd7ff5d1b2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 22 Apr 2020 15:48:15 +0200 Subject: smbus: Fix spd_data_generate() for number of banks > 2 spd_data_generate() splits @ram_size bytes into @nbanks RAM banks of 1 << sz_log2 MiB each, like this: size = ram_size >> 20; /* work in terms of megabytes */ [...] nbanks = 1; while (sz_log2 > max_log2 && nbanks < 8) { sz_log2--; nbanks++; } Each iteration halves the size of a bank, and increments the number of banks. Wrong: it should double the number of banks. The bug goes back all the way to commit b296b664ab "smbus: Add a helper to generate SPD EEPROM data". It can't bite because spd_data_generate()'s current users pass only @ram_size that result in *zero* iterations: machine RAM size #banks type bank size fulong2e 256 MiB 1 DDR 256 MiB sam460ex 2048 MiB 1 DDR2 2048 MiB 1024 MiB 1 DDR2 1024 MiB 512 MiB 1 DDR2 512 MiB 256 MiB 1 DDR2 256 MiB 128 MiB 1 SDR 128 MiB 64 MiB 1 SDR 64 MiB 32 MiB 1 SDR 32 MiB Apply the obvious, minimal fix. I admit I'm tempted to rip out the unused (and obviously untested) feature instead, because YAGNI. Note that this is not the final result, as spd_data_generate() next increases #banks from 1 to 2 if possible. This is done "to avoid a bug in MIPS Malta firmware". We don't even use this function with machine type malta. *Shrug* Signed-off-by: Markus Armbruster Message-Id: <20200422134815.1584-5-armbru@redhat.com> --- hw/i2c/smbus_eeprom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw/i2c') diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index 07fbbf87f1..e199fc8678 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -229,7 +229,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size) nbanks = 1; while (sz_log2 > max_log2 && nbanks < 8) { sz_log2--; - nbanks++; + nbanks *= 2; } assert(size == (1ULL << sz_log2) * nbanks); -- cgit v1.2.3