* Re: [PATCH] net: ena: fix PHC cleanup on probe failure
2026-09-16 2:29 [PATCH] net: ena: fix PHC cleanup on probe failure Guangshuo Li
@ 2026-09-16 5:44 ` Arthur Kiyanovski
2026-09-21 15:23 ` Guangshuo Li
2026-09-17 17:31 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Arthur Kiyanovski @ 2026-09-16 5:44 UTC (permalink / raw)
To: Guangshuo Li
Cc: Arthur Kiyanovski, David Arinzon, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Sang-Heon Jeon,
Niklas Söderlund, Dawei Feng, Amit Bernstein, netdev,
linux-kernel, stable
On Wed, 16 Sep 2026 10:29:54 +0800, Guangshuo Li <lgs201920130244@gmail.com> wrote:
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index ea89619039d8..5f0864d16dd3 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -4122,6 +4122,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> err_device_destroy:
> ena_com_delete_host_info(ena_dev);
> ena_com_admin_destroy(ena_dev);
> + ena_phc_destroy(adapter);
> ena_devlink_destroy:
> ena_devlink_free(devlink);
> err_metrics_destroy:
Thank you for the patch.
It looks correct.
Please resend this together with "net: ena: fix MMIO read buffer leak on
probe failure" as one two-patch series - both insert a line into the same
hunk, so they conflict with each other.
--
Arthur Kiyanovski <akiyano@amazon.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net: ena: fix PHC cleanup on probe failure
2026-09-16 2:29 [PATCH] net: ena: fix PHC cleanup on probe failure Guangshuo Li
2026-09-16 5:44 ` Arthur Kiyanovski
@ 2026-09-17 17:31 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-17 17:31 UTC (permalink / raw)
To: lgs201920130244
Cc: akiyano, darinzon, andrew+netdev, davem, edumazet, kuba, pabeni,
ekffu200098, niklas.soderlund+renesas, dawei.feng, amitbern,
netdev, linux-kernel, stable
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 0 (1)
- [Medium] ena_probe() error unwind still omits
ena_com_mmio_reg_read_request_destroy(), leaking the DMA-coherent…
- [Medium] The commit message's premise is not reachable: during an
initial ena_probe(), PHC is always disabled, so no PTP clock and no…
Pre-existing issues:
- [Low] ena_probe()'s err_worker_destroy label calls
timer_delete(&adapter->timer_service)…
--
net: ena: fix PHC cleanup on probe failure
This adds a call to ena_phc_destroy() to the err_device_destroy label in
ena_probe(), so the probe error path mirrors the ena_destroy_device()
teardown before ena_phc_free() runs.
A couple of questions about the changelog and the error path below.
> The normal removal path calls ena_phc_destroy() through
> ena_destroy_device() before ena_phc_free(). However, if probe fails
> after ena_device_init() succeeds, the error path reaches ena_phc_free()
> without unregistering the PTP clock or destroying the device PHC
> resources.
[Severity: Medium]
Is there actually a registered PTP clock or allocated device PHC resource
at that point during an initial probe?
Following the state that PHC has when ena_device_init() returns 0:
ena_phc_alloc() vzalloc()s phc_info, so phc_info->enabled is false, and
ena_devlink_configure_params() seeds the driverinit ENABLE_PHC value from
that state:
value.vbool = ena_phc_is_enabled(adapter);
devl_param_driverinit_value_set(devlink,
DEVLINK_PARAM_GENERIC_ID_ENABLE_PHC,
&value);
ena_device_init() -> ena_devlink_params_get() then reads that value back
and calls ena_phc_enable(adapter, false), so ena_phc_init() takes:
if (!ena_phc_is_enabled(adapter)) {
netdev_dbg(netdev, "PHC feature is not enabled by the kernel\n");
goto err_ena_com_phc_init;
}
which skips ena_com_phc_init() (phc->virt_addr stays NULL) and
ena_phc_register() (phc_info->clock stays NULL).
Userspace cannot flip the param earlier either, since devlink netlink
lookup rejects instances that are not registered yet
(devlink_get_from_attrs_lock() requires devl_is_registered()), and ENA
only calls ena_devlink_register() at the successful end of ena_probe().
So on every path that can reach the new call, ena_phc_unregister()
returns immediately via ena_phc_is_active() and ena_com_phc_destroy()
returns immediately on !phc->virt_addr. The only path where PHC can
really be active on failure is ena_restore_device(), which already calls
ena_phc_destroy().
Given that, are the Fixes: tag and Cc: stable warranted here, or would
describing this as hardening for symmetry be more accurate?
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index ea89619039d89..5f0864d16dd3c 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -4122,6 +4122,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> err_device_destroy:
> ena_com_delete_host_info(ena_dev);
> ena_com_admin_destroy(ena_dev);
> + ena_phc_destroy(adapter);
[Severity: Medium]
Does this label still leak the mmio read-less response buffer allocated
by ena_com_mmio_reg_read_request_init()?
ena_device_init() calls ena_com_mmio_reg_read_request_init()
unconditionally, which does:
mmio_read->read_resp = dma_alloc_coherent(ena_dev->dmadev, sizeof(*mmio_read->read_resp),
&mmio_read->read_resp_dma_addr, GFP_KERNEL);
and programs that address into the device via
ena_com_mmio_reg_read_request_write_dev_addr(). It is only released
inside ena_device_init() itself, at its err_mmio_read_less label.
The teardown sequence this patch is modelled on does three steps:
drivers/net/ethernet/amazon/ena/ena_netdev.c:ena_destroy_device() {
...
ena_com_admin_destroy(ena_dev);
ena_phc_destroy(adapter);
ena_com_mmio_reg_read_request_destroy(ena_dev);
...
}
and ena_restore_device()'s error path pairs them the same way:
ena_phc_destroy(adapter);
ena_com_mmio_reg_read_request_destroy(ena_dev);
The new code adds the middle step but not the last one, so once
ena_device_init() has returned 0, any later probe failure walks
err_device_destroy -> ena_devlink_free() ->
ena_com_delete_customer_metrics_buffer() -> ena_phc_free() ->
free_netdev() -> vfree(ena_dev) with mmio_read.read_resp still
allocated. After vfree(ena_dev) the pointer and DMA handle are gone, and
the device's ENA_REGS_MMIO_RESP_LO/HI registers keep the stale address,
since only ena_com_mmio_reg_read_request_destroy() zeroes them:
writel(0x0, ena_dev->reg_bar + ENA_REGS_MMIO_RESP_LO_OFF);
writel(0x0, ena_dev->reg_bar + ENA_REGS_MMIO_RESP_HI_OFF);
dma_free_coherent(ena_dev->dmadev, sizeof(*mmio_read->read_resp), mmio_read->read_resp,
mmio_read->read_resp_dma_addr);
Reachable failures include max_num_io_queues == 0 (which also does no
ena_com_dev_reset()), ena_com_init_interrupt_moderation() failure,
ena_enable_msix_and_set_admin_interrupts() failure,
ena_rss_init_default() failure and register_netdev() failure.
Should ena_com_mmio_reg_read_request_destroy(ena_dev) be added right
after the new ena_phc_destroy(adapter) here?
> ena_devlink_destroy:
> ena_devlink_free(devlink);
> err_metrics_destroy:
> ena_com_delete_customer_metrics_buffer(ena_dev);
[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
while looking at the labels just above the changed one: does
err_worker_destroy call timer_delete() on an uninitialized timer?
err_worker_destroy:
timer_delete(&adapter->timer_service);
timer_service is only ever set up after register_netdev() succeeds:
timer_setup(&adapter->timer_service, ena_timer_service, 0);
mod_timer(&adapter->timer_service, round_jiffies(jiffies + HZ));
and there is no failure goto after that point, so every path that lands
on err_worker_destroy (MSI-X setup failure, ena_rss_init_default()
failure falling through err_free_msix, register_netdev() failure falling
through err_rss) runs before timer_setup(). The memory is the zeroed
netdev private area from alloc_etherdev_mq().
With CONFIG_DEBUG_OBJECTS_TIMERS, __timer_delete() starts with
debug_assert_init(timer), and timer_is_static_object() requires
entry.next == TIMER_ENTRY_STATIC while the zeroed timer has
entry.next == NULL, so debug_print_object() issues an "assert_init"
warning before timer_fixup_assert_init() installs a stub. Without that
option, timer_pending() is false and the call is a no-op, so this only
shows up as a splat on debug kernels during an already-failing probe.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916022954.2690322-1-lgs201920130244%40gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread