Re: [Qemu-devel] [Qemu-trivial] [PATCH] hw/net/eepro100: Implement read-only bits in MDI registers

[Michael Tokarev <mjt@tls.msk.ru>]

07.06.2014 20:52, Peter Maydell wrote:
> Although we defined an eepro100_mdi_mask[] array indicating which bits
> in the registers are read-only, we weren't actually doing anything with
> it. Make the MDI register-read code use it rather than manually making
> registers 2 and 3 totally read-only and the rest totally read-write.

s/register-read/register-write/ -- can be fixed when applying.

I'm not sure this is "trivial enough", because the side effect is
not obvious, at least not to someone not familiar with eepro100
registers and their usage.

Besides, the description does not seem to be very accurate too.
>From the code I see that the original code makes register 0
"semi-writable", register 1 is unwritable and the rest fully
writable.

In this context, apparently we're losing the ability to write to
register 0 completely, since its mask is 0 but the original code
allows writing something to it.

Also, maybe updating the "missing()" calls according to the
bitmask is a good idea...

Thanks,

/mjt

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This seemed a better fix for the unused variable than just deleting it...
> 
>  hw/net/eepro100.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 3b891ca..9c70cce 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -1217,7 +1217,6 @@ static void eepro100_write_mdi(EEPRO100State *s)
>                  break;
>              case 1:            /* Status Register */
>                  missing("not writable");
> -                data = s->mdimem[reg];
>                  break;
>              case 2:            /* PHY Identification Register (Word 1) */
>              case 3:            /* PHY Identification Register (Word 2) */
> @@ -1230,7 +1229,8 @@ static void eepro100_write_mdi(EEPRO100State *s)
>              default:
>                  missing("not implemented");
>              }
> -            s->mdimem[reg] = data;
> +            s->mdimem[reg] &= eepro100_mdi_mask[reg];
> +            s->mdimem[reg] |= data & ~eepro100_mdi_mask[reg];
>          } else if (opcode == 2) {
>              /* MDI read */
>              switch (reg) {
>