Re: [PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access

Vladimir Oltean [off-list ref] writes:

Hi Alvin,

On Mon, Feb 21, 2022 at 02:50:24PM +0000, Alvin Šipraga wrote:

quoted

So I made a test module which, in summary, checks the following:

1. for PHY reads, at what point does inserting a stray register access
   (either read or write) cause the PHY read to fail?
2. for PHY writes, can stray register access also cause failure?
2. for MIB reads, can stray register access also cause failure?

For (1) I instrumented the PHY indirect access functions in the 6
possible places where spurious register access could occur. Of those 6
locations for spurious register access, 4 have no effect: you can put a
read or write to an unrelated register there and the PHY read will
always succeed. I tested this with spurious access to nearly every
available register on the switch.

However, for two locations of spurious register access, the PHY read
_always_ fails. The locations are marked /* XXX */ below:

/* Simplified for brevity */
static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
				  u32 ocp_addr, u16 *data)
{
	rtl8365mb_phy_poll_busy(priv);

	rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr);

	/* Execute read operation */
	regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_CTRL_REG, val);

	/* XXX */

	rtl8365mb_phy_poll_busy(priv);

	/* XXX */

	/* Get PHY register data */
	regmap_read(priv->map, RTL8365MB_INDIRECT_ACCESS_READ_DATA_REG,
		    &val);

	*data = val & 0xFFFF;

	return 0;
}

In the case of a spurious read, the result of that read then poisons the
ongoing PHY read, as suggested before. Again I verified that this is
always the case, for each available register on the switch. Spurious
writes also cause failure, and in the same locations too. I did not
investigate whether the value written is then read back as part of the
PHY read.

For (2) I did something similar to (1), but the difference here is that
I could never get PHY writes to fail. Admittedly not all bits of the PHY
registers tend to be writable, but for those bits that were writable, I
would always then read back what I had written.

For (3) I did something similar to (1), and as claimed previously, this
never resulted in a read failure. Here I had to use the MIB counters of
a disconnected port so that I could assume the values were always 0.

I have attached the test module (and header file generated from an
enormous header file from the Realtek driver sources, so that I could
iterate over every possible register). It is pretty gruesome reading but
gives me confidence in my earlier claims. The only refinements to those
claims are:

- where _exactly_ a spurious register access will cause failure: see the
  /* XXX */ in the code snippet upstairs;
- PHY writes seem not to be affected at all.

Finally, I reached out to Realtek, and they confirmed pretty much the
same as above. However, they claim it is not a hardware bug, but merely
a property of the hardware design. Here I paraphrase what was said:

1. Yes, spurious register access during PHY indirect access will cause
the indirect access to fail. This is a result of the hardware design. In
general, _if a read fails, the value read back will be the result of the
last successful read_. This confirms the "register poisoning" described
earlier.

2. MIB access is a different story - this is table lookup, not indirect
access. Table lookup is not affected by spurious register access.

3. Other possible accesses - not currently present in this driver, but
for which I have some WIP changes - include ACL (Access Control List),
L2 (FDB), and MC (MDB) access. But all of these are table access similar
to MIB access, and hence not troubled by spurious register access.

4. HOWEVER, only one table can be accessed at a time. So a lock is
needed here. Currently the only table lookup is MIB access, which is
protected by mib_lock, so we are OK for now.

5. It should be sufficient to lock during indirect PHY register access
as prescribed in my patch.

I hope that clears things up. I will be sending a v2 with a revised
description, including the statements from Realtek and the results of
the tests I ran.

Kind regards,
Alvin

Nice work!

This looks more comprehensive, although regarding check_phy_write(),
my understanding is that you checked cross-reads and cross-writes with
only one register: priv->read_reg is implicitly 0 during the
do_reg_work() -> check_phy_write() call sequence, so that register is
probably PORT0_CGST_HALF_CFG.

Woops, my bad - thanks for checking. I added an extra loop in
check_phy_write() now to do the checking with a cross-read of every
(good) register (and moved check_good_regs() before
check_phy_write()). The results are broadly the same, although I start
to get some changes when reaching registers like PORT0_STATUS (for
obvious reasons, since I'm poking the PHY control register). There the
logic of my test starts to break down a bit because a lot of the sanity
checks assume that the registers are non-volatile.

Still, PORT0_STATUS is the ~3000th register in the list, so if all other
cross-reads prior to that don't affect the PHY write, I am convinced
that this is not a problem for PHY writes.

For cross-writes I am always writing the same register anyway.

Anyway, if Realtek's description is that "if a read fails, the value
read back will be the result of the last successful read", then it's
probably not suprising that cross-reads and cross-writes don't make the
indirect PHY write fail (since there's no register read). I don't have
the background of what is the OCP, but the implication of the above
paragraph seems to be that an indirect PHY read is in essence the read
of a single register, which gets aborted when a read of any other
register except RTL8365MB_INDIRECT_ACCESS_STATUS_REG or
RTL8365MB_INDIRECT_ACCESS_READ_DATA_REG gets initiated.

I agree with what you wrote above, I think it captures the point
succinctly. (I also don't know what OCP stands for.)

Kind regards,
Alvin