Re: [PATCH 4/4] hw/nvram/eeprom_at24c: Reorganise init to avoid overwriting values

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 3/3/25 13:32, BALATON Zoltan wrote:
> On Mon, 3 Mar 2025, Philippe Mathieu-Daudé wrote:
>> On 1/3/25 15:35, BALATON Zoltan wrote:
>>> The init_rom can write values to the beginning of the memory but these
>>> are overwritten by values from a backing file that covers the whole
>>> memory. Do the init_rom handling only if it would not be overwritten.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/nvram/eeprom_at24c.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
>>> index 78c81bea77..ff7a21eee7 100644
>>> --- a/hw/nvram/eeprom_at24c.c
>>> +++ b/hw/nvram/eeprom_at24c.c
>>> @@ -191,10 +191,6 @@ static void at24c_eeprom_realize(DeviceState 
>>> *dev, Error **errp)
>>>         ee->mem = g_malloc0(ee->rsize);
>>>   -    if (ee->init_rom) {
>>> -        memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee- 
>>> >rsize));
>>> -    }
>>> -
>>>       if (ee->blk) {
>>>           int ret = blk_pread(ee->blk, 0, ee->rsize, ee->mem, 0);
>>>   @@ -204,6 +200,8 @@ static void at24c_eeprom_realize(DeviceState 
>>> *dev, Error **errp)
>>>               return;
>>>           }
>>>           DPRINTK("Reset read backing file\n");
>>> +    } else if (ee->init_rom) {
>>
>> Don't you want to keep overwritting the init_rom[] buffer?
>>
>> IOW why not s/else//?
> 
> I've tried to explain that in the commit message. Current behaviour is 
> to use backing file content overwriting init_rom content. Removing else 
> here would change that and init_rom would overwrite data read from 
> backing file. I think normally 

OK, I'll amend in description:

---
> init_rom is used only if there's no 
> backing file (provides default content) but should not overwrite backing 
> file content (especially leaving the file unchanged and only change it 
> in memory).
---

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> So I don't see why would that be useful.
> 
> Regards,
> BALATON Zoltan
> 
>>> +        memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee- 
>>> >rsize));
>>>       }
>>>         /*
>>
>>