[PATCH 0/4] Misc eeprom_at24c clean ups

[PATCH 0/4] Misc eeprom_at24c clean ups

[BALATON Zoltan]

These are some small misc clean ups in hw/nvram/eeprom_at24c.

Regards,
BALATON Zoltan

BALATON Zoltan (4):
  hw/nvram/eeprom_at24c: Use OBJECT_DECLARE_SIMPLE_TYPE
  hw/nvram/eeprom_at24c: Remove ERR macro that calls fprintf to stderr
  hw/nvram/eeprom_at24c: Remove memset after g_malloc0
  hw/nvram/eeprom_at24c: Reorganise init to avoid overwriting values

 hw/nvram/eeprom_at24c.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

-- 
2.30.9

[PATCH 1/4] hw/nvram/eeprom_at24c: Use OBJECT_DECLARE_SIMPLE_TYPE

[BALATON Zoltan]

No need to open code it so use the simple object type declaration.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/nvram/eeprom_at24c.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index a40cc5dd15..2ae03935d4 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -30,9 +30,7 @@
                             ## __VA_ARGS__)
 
 #define TYPE_AT24C_EE "at24c-eeprom"
-typedef struct EEPROMState EEPROMState;
-DECLARE_INSTANCE_CHECKER(EEPROMState, AT24C_EE,
-                         TYPE_AT24C_EE)
+OBJECT_DECLARE_SIMPLE_TYPE(EEPROMState, AT24C_EE)
 
 struct EEPROMState {
     I2CSlave parent_obj;
-- 
2.30.9

Re: [PATCH 1/4] hw/nvram/eeprom_at24c: Use OBJECT_DECLARE_SIMPLE_TYPE

[Philippe Mathieu-Daudé]

On 1/3/25 15:35, BALATON Zoltan wrote:
> No need to open code it so use the simple object type declaration.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/nvram/eeprom_at24c.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

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

[PATCH 2/4] hw/nvram/eeprom_at24c: Remove ERR macro that calls fprintf to stderr

[BALATON Zoltan]

In the realize method error_setg can be used like other places there
already do. The other usage can be replaced with error_report which is
the preferred way instead of directly printing to stderr.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/nvram/eeprom_at24c.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 2ae03935d4..9f606842eb 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "hw/i2c/i2c.h"
 #include "hw/nvram/eeprom_at24c.h"
@@ -26,9 +27,6 @@
 #define DPRINTK(FMT, ...) do {} while (0)
 #endif
 
-#define ERR(FMT, ...) fprintf(stderr, TYPE_AT24C_EE " : " FMT, \
-                            ## __VA_ARGS__)
-
 #define TYPE_AT24C_EE "at24c-eeprom"
 OBJECT_DECLARE_SIMPLE_TYPE(EEPROMState, AT24C_EE)
 
@@ -75,8 +73,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
         if (ee->blk && ee->changed) {
             int ret = blk_pwrite(ee->blk, 0, ee->rsize, ee->mem, 0);
             if (ret < 0) {
-                ERR(TYPE_AT24C_EE
-                        " : failed to write backing file\n");
+                error_report("%s: failed to write backing file", __func__);
             }
             DPRINTK("Wrote to backing file\n");
         }
@@ -203,8 +200,9 @@ static void at24c_eeprom_realize(DeviceState *dev, Error **errp)
         int ret = blk_pread(ee->blk, 0, ee->rsize, ee->mem, 0);
 
         if (ret < 0) {
-            ERR(TYPE_AT24C_EE
-                    " : Failed initial sync with backing file\n");
+            error_setg(errp, "%s: Failed initial sync with backing file",
+                       TYPE_AT24C_EE);
+            return;
         }
         DPRINTK("Reset read backing file\n");
     }
-- 
2.30.9

Re: [PATCH 2/4] hw/nvram/eeprom_at24c: Remove ERR macro that calls fprintf to stderr

[Philippe Mathieu-Daudé]

On 1/3/25 15:35, BALATON Zoltan wrote:
> In the realize method error_setg can be used like other places there
> already do. The other usage can be replaced with error_report which is
> the preferred way instead of directly printing to stderr.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/nvram/eeprom_at24c.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)

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

[PATCH 3/4] hw/nvram/eeprom_at24c: Remove memset after g_malloc0

[BALATON Zoltan]

Calling memset to zero memory is not needed after g_malloc0 which
already clears memory. These used to be in separate functions but
after some patches the memset ended up after g_malloc0 and thus can be
dropped.

Fixes: 4f2c6448c3 (hw/nvram/eeprom_at24c: Make reset behavior more like hardware)
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/nvram/eeprom_at24c.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 9f606842eb..78c81bea77 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -190,7 +190,6 @@ static void at24c_eeprom_realize(DeviceState *dev, Error **errp)
     }
 
     ee->mem = g_malloc0(ee->rsize);
-    memset(ee->mem, 0, ee->rsize);
 
     if (ee->init_rom) {
         memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee->rsize));
-- 
2.30.9

Re: [PATCH 3/4] hw/nvram/eeprom_at24c: Remove memset after g_malloc0

[Philippe Mathieu-Daudé]

On 1/3/25 15:35, BALATON Zoltan wrote:
> Calling memset to zero memory is not needed after g_malloc0 which
> already clears memory. These used to be in separate functions but
> after some patches the memset ended up after g_malloc0 and thus can be
> dropped.
> 
> Fixes: 4f2c6448c3 (hw/nvram/eeprom_at24c: Make reset behavior more like hardware)
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/nvram/eeprom_at24c.c | 1 -
>   1 file changed, 1 deletion(-)

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

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

[BALATON Zoltan]

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

[Philippe Mathieu-Daudé]

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]

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

[Philippe Mathieu-Daudé]

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

[Philippe Mathieu-Daudé]

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

Re: [PATCH 0/4] Misc eeprom_at24c clean ups

[Philippe Mathieu-Daudé]

On 1/3/25 15:35, BALATON Zoltan wrote:
> These are some small misc clean ups in hw/nvram/eeprom_at24c.

> BALATON Zoltan (4):
>    hw/nvram/eeprom_at24c: Use OBJECT_DECLARE_SIMPLE_TYPE
>    hw/nvram/eeprom_at24c: Remove ERR macro that calls fprintf to stderr
>    hw/nvram/eeprom_at24c: Remove memset after g_malloc0
>    hw/nvram/eeprom_at24c: Reorganise init to avoid overwriting values

Thanks, series queued.