Re: [Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Gabriel L. Somlo]

Sorry for the repeated self-replies, I appear to be thinking in very
small increments today :)

On Mon, Jun 30, 2014 at 03:29:57PM -0400, Gabriel L. Somlo wrote:
> > > Weird.
> > > So does this mean have_autoneg is always false then?
> > > How does it work?
> 
> Oh, I think I know what's going on now:
> 
> In set_mdic() we have:
> 
> ...
>         } else {
>             if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
>                 phyreg_writeops[addr](s, index, data);
>             }
>             s->phy_reg[addr] = data;
>         }
> ...

Of course, we could change this to

...
        } else {
            if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
                phyreg_writeops[addr](s, index, data);
            } else {
                s->phy_reg[addr] = data;
            }
        }

and have phy_set_ctrl() be a proper write_op, expected to actually
write the data as well as do other things. That would at least be a
step toward the principle of least WTF, IMHO :)

I'll send out an updated patch shortly...

Thanks,
--Gabriel

> ...
> 
> Basically, phyreg_writeops[PHY_CTRL] == set_phy_ctrl() is the only
> one that applies, but it's like a "pre-write" writeop, the write
> happens *after* the write_op completes :) :) Fun stuff...
> 
> Leaving it as-is is probably OK, although it does indeed not handle
> SC bits properly.
> 
> I could rewrite have_autoneg() to accept the *value* of the phy control
> register, then we can use it from set_phy_ctrl() before the register
> is actually written. Not sure about whether that would be more or less
> ugly than the current form :)
> 
> Thanks,
> --Gabriel