From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: BALATON Zoltan <balaton@eik.bme.hu>,
Markus Armbruster <armbru@redhat.com>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation
Date: Wed, 22 Apr 2020 16:50:18 +0200 [thread overview]
Message-ID: <0af0e0f0-8127-da83-d9d2-89a3fe28f778@redhat.com> (raw)
In-Reply-To: <alpine.BSF.2.22.395.2004221622140.19234@zero.eik.bme.hu>
On 4/22/20 4:27 PM, BALATON Zoltan wrote:
> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>> 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.
>
> This leaves board code no chance to recover from values given by user
> that won't fit without duplicating checks that this function does. Also
> this will abort without giving meaningful errors if an invalid value
> does get through and result in a crash which is not used friendly. So I
> don't like this but if others think this is acceptable maybe at least
> unit test should be adjusted to make sure aborts cannot be triggered by
> user for values that are not usually tested during development.
Agreed. Do you have an example (or more) to better show Markus this code
use? So we can add tests.
Personally I'd use a script to generate a dumb static array of all
possible sizes...
>
> Regards,
> BALATON Zoltan
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/hw/i2c/smbus_eeprom.h | 2 +-
>> hw/i2c/smbus_eeprom.c | 30 ++++--------------------------
>> hw/mips/mips_fulong2e.c | 10 ++--------
>> hw/ppc/sam460ex.c | 12 +++---------
>> 4 files changed, 10 insertions(+), 44 deletions(-)
>>
>> diff --git a/include/hw/i2c/smbus_eeprom.h
>> b/include/hw/i2c/smbus_eeprom.h
>> index 15e2151b50..68b0063ab6 100644
>> --- a/include/hw/i2c/smbus_eeprom.h
>> +++ b/include/hw/i2c/smbus_eeprom.h
>> @@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
>> const uint8_t *eeprom_spd, int size);
>>
>> enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
>> -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size,
>> Error **errp);
>> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);
>>
>> #endif
>> 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) {
>> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
>> index 5040afd581..ef02d54b33 100644
>> --- a/hw/mips/mips_fulong2e.c
>> +++ b/hw/mips/mips_fulong2e.c
>> @@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine)
>> MemoryRegion *bios = g_new(MemoryRegion, 1);
>> long bios_size;
>> uint8_t *spd_data;
>> - Error *err = NULL;
>> int64_t kernel_entry;
>> PCIBus *pci_bus;
>> ISABus *isa_bus;
>> @@ -377,13 +376,8 @@ static void mips_fulong2e_init(MachineState
>> *machine)
>> }
>>
>> /* Populate SPD eeprom data */
>> - spd_data = spd_data_generate(DDR, machine->ram_size, &err);
>> - if (err) {
>> - warn_report_err(err);
>> - }
>> - if (spd_data) {
>> - smbus_eeprom_init_one(smbus, 0x50, spd_data);
>> - }
>> + spd_data = spd_data_generate(DDR, machine->ram_size);
>> + smbus_eeprom_init_one(smbus, 0x50, spd_data);
>>
>> mc146818_rtc_init(isa_bus, 2000, NULL);
>>
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 1e3eaac0db..42a8c9fb7f 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -292,7 +292,6 @@ static void sam460ex_init(MachineState *machine)
>> SysBusDevice *sbdev;
>> struct boot_info *boot_info;
>> uint8_t *spd_data;
>> - Error *err = NULL;
>> int success;
>>
>> cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>> @@ -336,14 +335,9 @@ static void sam460ex_init(MachineState *machine)
>> i2c = PPC4xx_I2C(dev)->bus;
>> /* SPD EEPROM on RAM module */
>> spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
>> - ram_sizes[0], &err);
>> - if (err) {
>> - warn_report_err(err);
>> - }
>> - if (spd_data) {
>> - spd_data[20] = 4; /* SO-DIMM module */
>> - smbus_eeprom_init_one(i2c, 0x50, spd_data);
>> - }
>> + ram_sizes[0]);
>> + spd_data[20] = 4; /* SO-DIMM module */
>> + smbus_eeprom_init_one(i2c, 0x50, spd_data);
>> /* RTC */
>> i2c_create_slave(i2c, "m41t80", 0x68);
>>
>>
>
next prev parent reply other threads:[~2020-04-22 14:51 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-22 13:48 [PATCH v2 0/4] smbus: SPD fixes Markus Armbruster
2020-04-22 13:48 ` [PATCH v2 1/4] sam460ex: Suppress useless warning on -m 32 and -m 64 Markus Armbruster
2020-04-22 14:44 ` Philippe Mathieu-Daudé
2020-04-22 13:48 ` [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation Markus Armbruster
2020-04-22 14:27 ` BALATON Zoltan
2020-04-22 14:50 ` Philippe Mathieu-Daudé [this message]
2020-04-22 19:32 ` BALATON Zoltan
2020-06-26 11:30 ` BALATON Zoltan
2020-06-27 7:17 ` Markus Armbruster
2020-06-27 11:30 ` BALATON Zoltan
2020-06-29 15:56 ` Markus Armbruster
2020-06-30 1:30 ` BALATON Zoltan
2020-06-29 19:00 ` Philippe Mathieu-Daudé
2020-06-29 21:31 ` BALATON Zoltan
2020-06-30 6:09 ` Philippe Mathieu-Daudé
2020-06-30 9:27 ` BALATON Zoltan
2020-04-22 13:48 ` [PATCH v2 3/4] bamboo, sam460ex: Tidy up error message for unsupported RAM size Markus Armbruster
2020-04-22 14:20 ` BALATON Zoltan
2020-04-22 14:22 ` Philippe Mathieu-Daudé
2020-04-22 13:48 ` [PATCH v2 4/4] smbus: Fix spd_data_generate() for number of banks > 2 Markus Armbruster
2020-04-22 18:26 ` [PATCH v2 0/4] smbus: SPD fixes no-reply
2020-04-29 7:14 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0af0e0f0-8127-da83-d9d2-89a3fe28f778@redhat.com \
--to=philmd@redhat.com \
--cc=armbru@redhat.com \
--cc=balaton@eik.bme.hu \
--cc=david@gibson.dropbear.id.au \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).