Misc eeprom_at24c clean ups

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

Posted by BALATON Zoltan 3 days, 11 hours ago

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) {
+        memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee->rsize));
     }
 
     /*
-- 
2.30.9

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

Posted by Philippe Mathieu-Daudé 1 day, 14 hours ago

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//?

> +        memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee->rsize));
>       }
>   
>       /*

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

Posted by BALATON Zoltan 1 day, 13 hours ago

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 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). 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));
>>       }
>>         /*
>
>

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

Posted by Philippe Mathieu-Daudé 1 day, 13 hours ago

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));
>>>       }
>>>         /*
>>
>>

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

Posted by Philippe Mathieu-Daudé 1 day, 13 hours ago

On 3/3/25 13:40, Philippe Mathieu-Daudé wrote:
> 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).
> ---

Reworded as:

---
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.

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).
Do the init_rom[] handling only if it would not be overwritten.
---

> 
> 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));
>>>>       }
>>>>         /*
>>>
>>>
>