Repository navigation
Conversation
BugLink: https://bugs.launchpad.net/bugs/ lan743x_tx_close() and lan743x_rx_close() ask the DMA controller to stop the channel and wait for it, but ignore the result. If the channel is still stop-pending when the wait gives up, the descriptor ring, the head write-back buffer and the packet buffers are unmapped and freed while the channel may still be fetching from or writing to them. A TX channel has been seen to stop making progress, with its head index no longer advancing while DMAC_CMD still reports it as started. A channel in that state may not honour the stop request either, and closing the interface would then free memory the engine can still access. If the channel does not stop, issue a channel soft reset and wait for it before anything is freed, and log when that happens. Fixes: 23f0703 ("lan743x: Add main source files for new lan743x driver") Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
BugLink: https://bugs.launchpad.net/bugs/ lan743x_netdev_open() starts phylink before the RX and TX rings exist, and lan743x_netdev_close() only stops it after the rings are freed. The mac_link_up() callback wakes all TX queues, so it can run while there is no TX ring to transmit on. For ndo_open and ndo_stop the qdisc is not active at that point, but the suspend and resume paths call lan743x_netdev_close() and lan743x_netdev_open() directly with the qdisc still attached. A link up event in either window lets the stack call ndo_start_xmit() on a channel whose ring is not allocated yet or has just been freed. The error path of lan743x_netdev_open() is also wrong: a lan743x_ptp_open() failure jumps past lan743x_phylink_disconnect(), leaving phylink started and the PHY attached after open has failed. Every later open then fails because the PHY is already attached. Connect and start phylink as the last step of open and stop it as the first step of close, as other phylink users do. If phylink fails to connect, nothing is attached, so the error path only needs to unwind the rings, PTP, MAC and interrupts. Fixes: a5f199a ("net: lan743x: Migrate phylib to phylink") Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
BugLink: https://bugs.launchpad.net/bugs/ lan743x_pcidev_shutdown(), which suspend also uses, calls netif_device_detach() and then lan743x_netdev_close() with the qdisc still attached. Detaching only sets the queue stop bits. It does not wait for an ndo_start_xmit() that is already running, and the TX NAPI poll still wakes a stopped queue as soon as it has reclaimed descriptors. The TX NAPI of a channel that has not been closed yet can therefore restart its queue, and the stack then transmits on a ring that lan743x_tx_close() is freeing. Only wake a queue from TX NAPI while the device is present. In lan743x_netdev_close(), wait for any NAPI poll that may have missed the detach with synchronize_net(), then stop all queues with netif_tx_disable(), which also waits for transmits in progress, before closing the channels. For ndo_stop the qdisc is already deactivated, so this only adds a grace period there. Fixes: 23f0703 ("lan743x: Add main source files for new lan743x driver") Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
BugLink: https://bugs.launchpad.net/bugs/ lan743x_pm_resume() ignores the return value of lan743x_netdev_open(). If the reopen fails, the rings and interrupts have already been released by its error path, but netif_device_attach() still wakes every TX queue and the stack transmits on rings that do not exist. A later ndo_stop then runs lan743x_netdev_close() a second time, and napi_disable() on the NAPI contexts that the failed open already disabled never returns, with rtnl held. On failure, take the interface down with dev_close() before attaching the device again, so it can be brought back up by the user. Route ndo_stop through a wrapper that skips the hardware close while the device is detached. That covers the dev_close() above, and also a resume that failed earlier in lan743x_hardware_init() and left the device detached with the hardware already closed by suspend. Fixes: 4d94282 ("lan743x: Add power management support") Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
BugLink: https://bugs.launchpad.net/bugs/ The driver does not implement ndo_tx_timeout, so the core TX watchdog is never started. If a TX DMA channel stops completing descriptors, its queue stays stopped and the interface cannot transmit again until it is brought down and back up. Add an ndo_tx_timeout handler that schedules a work item to reset the device. Under rtnl_lock(), the work detaches the netdev, closes it, soft-resets the DMA controller, opens it again and reattaches it. This is the sequence suspend and resume already use, with the DMA controller reset taking the place of the full hardware init. Because everything is reopened, the RX path, PTP and the PHY link are restarted along with TX. As on suspend and resume, the PTP clock is unregistered and registered again and its time is reset to the system time. lan743x_ptp_open() also clears the hardware timestamping configuration, so save it before the reset and restore it afterwards. If reopening fails, the interface is taken down with dev_close() so it can be brought back up later. ndo_stop skips the hardware close while the device is detached, so the hardware is not closed twice. ndo_stop is called with rtnl held and cannot wait for the work, so it only cancels a pending one. A work that is already waiting for rtnl returns if the device is still down when it gets the lock. If the device was opened again in the meantime, it is reset once more, which only costs an extra link flap. On remove, the work is cancelled synchronously after the netdev is unregistered. Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
BugLink: https://bugs.launchpad.net/bugs/ adj_limit caches limit + num_completed, and dql_avail() returns its difference to num_queued. dql_reset() sets limit back to min_limit and clears num_queued and num_completed, but leaves adj_limit at its old value. It is only recomputed by the next dql_completed(). The counters are free running and wrap, so until that first completion the budget reported by dql_avail() is arbitrary. Depending on how many bytes were completed before the reset, netdev_tx_sent_queue() either lets far more than the limit be queued or stops the queue after the first packet. In the first case, if the device stops completing descriptors right after netdev_tx_reset_queue(), nothing limits the bytes in flight and the queue only stops once the driver's ring is full, which also delays the TX watchdog. Set adj_limit to limit, which is limit + num_completed after the reset. This leaves the queue in the same state as dql_init() does. Fixes: 75957ba ("dql: Dynamic queue limits") Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
BugLink: https://bugs.launchpad.net/bugs/ Report the bytes queued to and completed by each TX ring to BQL. Besides limiting how much data sits in the TX ring, this lets the stack stop a queue once the bytes in flight exceed the BQL limit. Without it, a queue is only stopped when its 128 entry ring is full, and the TX watchdog only looks at stopped queues. With light traffic it can take tens of seconds after a TX hang to fill the ring, before the watchdog timeout even starts counting. lan743x_tx_close() frees pending descriptors without completing them, so reset the BQL state there. Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
BaseOS Kernel ReviewWarning
|
PR Validation ReportPatchscan ✅ No Missing FixesAll cherry-picked commits checked — no missing upstream fixes found. PR Lint ❌ Errors foundDetailsChecking 7 commits... Cherry-pick digest: ┌──────────────┬──────────────────────────────────────────────────────────────────┬────────────┬─────────┬───────────────────────────┐ │ Local │ Referenced upstream / Patch subject │ Patch-ID │ Subject │ SoB chain │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ 84678a7c18bd │ [SAUCE] net: lan743x: add byte queue limits support │ N/A │ N/A │ cbabrosk │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ e7677e047375 │ [SAUCE] dql: reset adj_limit in dql_reset() │ N/A │ N/A │ cbabrosk │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ c8ac2c4f659e │ [SAUCE] net: lan743x: reset the device on tx timeout │ N/A │ N/A │ cbabrosk │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ a3a37f5e325c │ [SAUCE] net: lan743x: handle a failed reopen on resume │ N/A │ N/A │ cbabrosk │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ 752db4239951 │ [SAUCE] net: lan743x: quiesce tx before freeing the rings │ N/A │ N/A │ cbabrosk │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ 4b7103ace3bc │ [SAUCE] net: lan743x: start phylink last and stop it first │ N/A │ N/A │ cbabrosk │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ b0dddf3701e2 │ [SAUCE] net: lan743x: reset dma channels that fail to stop │ N/A │ N/A │ cbabrosk │ └──────────────┴──────────────────────────────────────────────────────────────────┴────────────┴─────────┴───────────────────────────┘ Lint: all checks passed. PR metadata: E: PR targets 26.04_linux-nvidia but body has no https://bugs.launchpad.net/... link |
|
BugLink: please drop the empty Tracking bug: is there an internal NVBug or JIRA for this? A link would give reviewers some context, such as which platform hits the TX hang and how it reproduces. Upstream plan: what is the plan for getting these upstream? None of them has been posted to netdev yet. Posting them first and getting maintainer feedback before they are taken here would be preferred. In particular, the dql fix changes core code used by every BQL driver. |
|
Hi @jamieNguyenNVIDIA , sorry for the confusion - I opened this PR as a "draft" so we could start reviewing internally on our team first. The relevant internal bug link is https://redmine.nvidia.com/issues/5165227. Our release code freeze date is coming up soon so I wanted to make sure that we have changes ready to go if needed. I will clean this up before marking the PR ready for review. We are also reviewing the changes internally with Microchip in parallel and would like approval from them before sending any of these patches upstream. Microchip are the primary maintainers of this driver, but the patch series adds recovery for a TX hang issue that can only be reproduced on BlueField 4 so we have been making the driver changes. |
When you are ready to move from draft we probably will need another PR for [26.04_linux-nvidia-bos] with these changes. |
Ah -- got it. Thank you for the heads up. |
4 participants
WIP: no Bug opened with Canonical yet.
This set of changes will also need to be included in 24.04 7.0-HWE.