Hi Stephen,
Thanks for your review. I have addressed these issues and just sent
out v12 series.
> > Patch 13/17 (net/txgbe: fix Tx laser left on at AML40 link down)
>
> enable_tx_laser() still takes the SWFW semaphore and does an I2C write on
> AML40:
>
> hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY) == 0) {
> hw->phy.write_i2c_eeprom(hw,
> TXGBE_SFF_8636_TX_DISABLE, 0x0);
>
> and txgbe_set_link_to_amlite() still calls enable_tx_laser() on the
> TXGBE_ERR_PHY_INIT_NOT_DONE path, under the phy_lock spinlock
taken in
> txgbe_setup_phy_link_aml40().
>
> It is worse than a long hold. txgbe_dev_setup_link_alarm_handler_aml()
> acquires hw->phy.phy_semaphore_mask before calling setup_link(), and
> txgbe_read_phy_if() sets that mask to TXGBE_MNGSEM_SWPHY. The nested acquire
> therefore cannot succeed. It spins 200 x 5 ms holding
> phy_lock, then the timeout path in txgbe_acquire_swfw_sync() clears the SW
> bit the outer caller still holds, so the rest of that
handler runs
> without the semaphore and its final release drops a lock it no longer owns.
You are right about the nested acquire. The call that triggers it is the
one in the PHY init failure path of txgbe_set_link_to_amlite(), which
runs with the PHY lock held and after the alarm handler already took the
semaphore.
The laser is switched there because of what the function does on the peer:
before the local side configures the PHY, the transmitter is switched off and
then switched back on, so that the peer sees an interruption and notices that
it has to re-establish its side against the new configuration.
On the 40G MAC that is not done. The disable before the PHY reset and the
enable on the success path are both limited to the 25G MAC. This was an
oversight: a configuration intended only for the 25G MAC was applied to the
40G MAC. Therefore, I simply added the restriction that limits it to the 25G
MAC.
> Patch 14/17: still busy-waits up to 200 ms (page exchange) and 400 ms
> (CL72) with usec_delay() on the EAL alarm thread.
The issue is confirmed, but changing this here is complicated. For the current
version, I prefer not to make further changes for now.
A later change will decouple negotiation from the alarm callback and run
per-port negotiations in parallel. That change is still being tested.
> Patch 17/17: a field rename is not a fix, and 234ce0d1fa9d is not in 25.11,
> so there is nothing to backport. Drop Fixes: and Cc:
stable.
234ce0d1fa was picked into 25.11.3, and the field is present in 25.11.3 and
25.11.4. So the rename does have a backport target.
This is not just a rename. It fixes an obvious spelling error. I checked the
commit history, and fixes for spelling errors are indeed tagged with 'Fixes:',
for example aecba37874 ("net/idpf: fix typo in CQ scan threshold macro
name") so I kept these tags.
Thanks,
Zaiyu Wang
