Messages in this thread | | | From | Alvin Šipraga <> | Subject | Re: [PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access | Date | Mon, 21 Feb 2022 18:10:01 +0000 |
| |
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 | |