Re: [PATCH net v2 2/2] ionic: free port_info when probe fails after the port is set up

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new ionic_port_reset() call in ionic_probe()'s err_out_pci
  frees the port_info buffer with dma_free_coherent() even when the…

--

ionic: free port_info when probe fails after the port is set up

This patch adds an ionic_port_reset() call at the err_out_pci label in
ionic_probe(). The goal is to free the port_info DMA buffer that
ionic_setup_one()->ionic_port_init() allocated when probe fails later.
It follows the cleanup order already used in ionic_remove().

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 05f19489ec5c8..68b3bceedd999 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -397,6 +397,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  err_out_free_irqs:
>  	ionic_bus_free_irq_vectors(ionic);
>  err_out_pci:
> +	ionic_port_reset(ionic);

[Severity: Medium]
Is it safe to free port_info here if the PORT_RESET command never
completes?

ionic_port_reset() sends the command, but the wait result does not
decide whether the free happens:

drivers/net/ethernet/pensando/ionic/ionic_main.c:ionic_port_reset() {
    ...
	if (ionic_is_fw_running(idev)) {
		mutex_lock(&ionic->dev_cmd_lock);
		ionic_dev_cmd_port_reset(idev);
		err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
		mutex_unlock(&ionic->dev_cmd_lock);
	}

	dma_free_coherent(ionic->dev, idev->port_info_sz,
			  idev->port_info, idev->port_info_pa);
    ...
}

__ionic_dev_cmd_wait() can return -ETIMEDOUT while the firmware still
reports that it is running:

	if (!done && !time_before(jiffies, max_wait)) {
		ionic_dev_cmd_clean(ionic);
		dev_warn(ionic->dev, "DEVCMD %s (%d) timeout after %ld secs\n",
			 ionic_opcode_to_str(opcode), opcode, max_seconds);
		return -ETIMEDOUT;
	}

ionic_dev_cmd_port_init() passed port_info_pa to the firmware through
port_init.info_pa. The comment in ionic_get_link_ksettings() says the NIC
updates this buffer by DMA on its own.

Bus mastering, enabled by pci_set_master() in ionic_setup_one(), stays on
until pci_disable_device() in ionic_clear_pci(). That runs only after
ionic_dev_teardown().

Take a probe failure like an adminq timeout in ionic_lif_init(). The
firmware could be running but not answering dev-cmds. If it keeps writing
port status into port_info during that window, can it DMA into pages that
have already been freed and possibly reused?

Before this patch, the buffer was leaked on this path instead of freed.

The commit message says:

    the firmware gets a PORT_RESET before the buffer is freed, the same
    order as in ionic_remove().

However, the command is only sent here, and its completion is never
checked. ionic_remove() uses the same pattern, so this patch carries that
convention over to the probe failure path.

Should the free be skipped when the PORT_RESET wait fails? Alternatively,
could it be deferred until bus mastering has been disabled?

>  	ionic_dev_teardown(ionic);
>  	ionic_clear_pci(ionic);
>  	ionic_debugfs_del_dev(ionic);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-ionic-port-info-lifetime-v2-0-57232013fd79%40amd.com