netdev - Understanding the et1011c PHY driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 23 Mar 2011 16:14:11 -0500
From:	"Moffett, Kyle D" <Kyle.D.Moffett@...ing.com>
To:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:	Chaithrika U S <chaithrika@...com>
Subject: Understanding the et1011c PHY driver

Hello,

I'm currently working on some PHY layer refactoring I'd like to propose to
support some of our custom hardware, and I'm trying to understand what the
heck the ET1011C PHY driver is doing so I can fix it up to match.

Specifically, the et1011c_read_status() code does this:

  int ret;
  u32 val;
  static int speed;
  ret = genphy_read_status(phydev);

  if (speed != phydev->speed) {
          speed = phydev->speed;
          val = phy_read(phydev, ET1011C_STATUS_REG);
          if ((val & ET1011C_SPEED_MASK) == ET1011C_GIGABIT_SPEED) {
                  val = phy_read(phydev, ET1011C_CONFIG_REG);
                  val &= ~ET1011C_TX_FIFO_MASK;
                  phy_write(phydev, ET1011C_CONFIG_REG, val\
                                  | ET1011C_GMII_INTERFACE\
                                  | ET1011C_SYS_CLK_EN\
                                  | ET1011C_TX_FIFO_DEPTH_16);
          }
  }
  return ret;

There are so many things wrong with this code that it's not even funny.
There's a static variable promising terrible terrible things should you
have more than 1 such PHY in a system.  The "GMII_INTERFACE" and
"SYS_CLK_EN" bits should only ever be modified as a board-specific PHY
fixup, because the PHY driver has no hope of getting it right in general.
The FIFO depth bits are "changed" to the default value after a reset,
and they are not touched anywhere else in the code.

On top of that, I don't see anywhere that the code would ever undo any
of these changes if the interface dropped from 1000BaseT down to 100BaseT
without a reset.  Looking at the datasheet, it does not even appear that
any of those changes *should* be different between gigabit/non-gigabit.

Then the et1011c_config_aneg() function is customized to do an extra PHY
reset before calling genphy_config_aneg(), which I suspect is only
necessary because of the other abominations above.

As far as I can tell, this PHY *SHOULD* be handled entirely by the genphy
driver with just a small board-specific phy_register_fixup() that gets
run after every reset of the PHY.

I'm looking forward to hearing your opinions on this driver.

Cheers,
Kyle Moffett
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html