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

[Vladimir Oltean]

Hi Alvin,

On Mon, Feb 21, 2022 at 02:50:24PM +0000, Alvin Šipraga wrote:
> 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.

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.