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

[Eric Joyner <eric.joyner@amd.com>]

On 10/19/2026 4:20 PM, netdev-bot+sashiko@kernel.org wrote:
> [Some people who received this message don't often get email from netdev-bot+sashiko@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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

Hi netdev's Sashiko Bot,

This finding is that this invites a change to cleanup handling in ionic that is
outside the scope of the patch; this patch does attempt to slightly clean up
things by changing a leak to a proper free while tearing down the driver after a
failure in probe, but it is valid that the device could still write into the
freed memory if that cleanup also has a failure.

I think a proper fix could be patterned after Nikhil's patch in 6443f4f20bda
("pds_core: quiesce DMA before freeing resources"), but that was applied to
net-next:
https://lore.kernel.org/all/178096501514.1726280.11716563786761485251.git-patchwork-notify@kernel.org/

- Eric