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

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

Vladimir Oltean <olteanv@gmail.com> writes:

> 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.

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