| Message ID | fd8e0478febd60d5f48c58bc77c60e043d1c3cdc.1740839457.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Misc eeprom_at24c clean ups | expand |
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)); > } > > /*
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)); >> } >> /* > >
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)); >>> } >>> /* >> >>
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)); >>>> } >>>> /* >>> >>> >
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)); } /*
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(-)