Re: [PATCH] hw/nvram: at24 return 0xff if 1 byte address

[Philippe Mathieu-Daudé]

On 12/20/21 16:32, Patrick Venture wrote:
> On Mon, Dec 20, 2021 at 1:12 AM Philippe Mathieu-Daudé
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>     Hi Patrick,
> 
>     On 12/20/21 01:32, Patrick Venture wrote:
>     > The at24 eeproms are 2 byte devices that return 0xff when they are
>     read
>     > from with a partial (1-byte) address written.  This distinction was
>     > found comparing model behavior to real hardware testing.
>     >
>     > Tested: `i2ctransfer -f -y 45 w1@85 0 r1` returns 0xff instead of next
>     > byte
>     >
>     > Signed-off-by: Patrick Venture <venture@google.com
>     <mailto:venture@google.com>>
>     > ---
>     >  hw/nvram/eeprom_at24c.c | 8 +++++++-
>     >  1 file changed, 7 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
>     > index a9e3702b00..184fac9702 100644
>     > --- a/hw/nvram/eeprom_at24c.c
>     > +++ b/hw/nvram/eeprom_at24c.c
>     > @@ -62,7 +62,9 @@ int at24c_eeprom_event(I2CSlave *s, enum
>     i2c_event event)
>     >      case I2C_START_SEND:
>     >      case I2C_START_RECV:
>     >      case I2C_FINISH:
>     > -        ee->haveaddr = 0;
>     > +        if (event != I2C_START_RECV) {
>     > +            ee->haveaddr = 0;
>     > +        }
> 
>     Alternatively (matter of taste, I find it easier to read):
> 
>            case I2C_START_SEND:
>            case I2C_FINISH:
>                ee->haveaddr = 0;
>                /* fallthrough */
>            case I2C_START_RECV:
> 
> 
> That may be easier to read :) I'm not sure, but I'm willing to bend and
> change my patch to behave this way.  Sometimes the fallthrough things
> leads to compiler annoyances in my experience.  We might  need
> __attribute__(fallthrough) or the like to convince the system that's
> what we really want. 

OK then.

> 
> 
>     >          DPRINTK("clear\n");
>     >          if (ee->blk && ee->changed) {
>     >              int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
>     > @@ -86,6 +88,10 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
>     >      EEPROMState *ee = AT24C_EE(s);
>     >      uint8_t ret;
>     > 
>     > +    if (ee->haveaddr == 1) {
>     > +        return 0xff;
> 
>     Don't we need to increase ee->haveaddr?
> 
> 
> We don't because the call to recv doesn't set any addr bytes.  This
> patch is primarily a behavioral fix to handle the device being treated
> as 8-bit addressable.  This is typically tested by writing a 1 byte
> address and then trying to read.  The chip itself will not have enough
> address bytes and reject this read by returning 0xff.  The
> haveaddr variable is strictly updated when they've written another byte
> to the address, or they've changed states in such a way that should
> clear any previously written address.  You can read from an eeprom by
> just reading or by setting an address and then reading.

Yes. And your approach is simple enough.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks,

Phil.