[PATCH net-next 0/3] ionic: three little changes

[PATCH net-next 0/3] ionic: three little changes

[Shannon Nelson]

These are three little changes for the code from inspection
and testing.

Shannon Nelson (3):
  ionic: print firmware heartbeat as unsigned
  ionic: clean dbpage in de-init
  ionic: cancel delayed work earlier in remove

 drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c | 1 +
 drivers/net/ethernet/pensando/ionic/ionic_dev.c     | 4 ++--
 drivers/net/ethernet/pensando/ionic/ionic_lif.c     | 7 +++----
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] ionic: print firmware heartbeat as unsigned

[Shannon Nelson]

The firmware heartbeat value is an unsigned number, and seeing
a negative number when it gets big is a little disconcerting.
Example:
    ionic 0000:24:00.0: FW heartbeat stalled at -1342169688

Print using the unsigned flag.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 57edcde9e6f8..532faf6d15ee 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -424,9 +424,9 @@ int ionic_heartbeat_check(struct ionic *ionic)
 	if (fw_hb_ready != idev->fw_hb_ready) {
 		idev->fw_hb_ready = fw_hb_ready;
 		if (!fw_hb_ready)
-			dev_info(ionic->dev, "FW heartbeat stalled at %d\n", fw_hb);
+			dev_info(ionic->dev, "FW heartbeat stalled at %u\n", fw_hb);
 		else
-			dev_info(ionic->dev, "FW heartbeat restored at %d\n", fw_hb);
+			dev_info(ionic->dev, "FW heartbeat restored at %u\n", fw_hb);
 	}
 
 	if (!fw_hb_ready)
-- 
2.17.1

[PATCH net-next 2/3] ionic: clean dbpage in de-init

[Shannon Nelson]

Since the kern_dbpage gets set up in ionic_lif_init() and that
function's error path will clean it if needed, the kern_dbpage
on teardown should be cleaned in ionic_lif_deinit(), not in
ionic_lif_free().  As it is currently we get a double call
to iounmap() on kern_dbpage if the PCI ionic fails setting up
the lif.  One example of this is when firmware isn't responding
to AdminQ requests and ionic's first AdminQ call fails to
setup the NotifyQ.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 7707a9e53c43..48cb5d30b5f6 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -3526,10 +3526,6 @@ void ionic_lif_free(struct ionic_lif *lif)
 	lif->info = NULL;
 	lif->info_pa = 0;
 
-	/* unmap doorbell page */
-	ionic_bus_unmap_dbpage(lif->ionic, lif->kern_dbpage);
-	lif->kern_dbpage = NULL;
-
 	mutex_destroy(&lif->config_lock);
 	mutex_destroy(&lif->queue_lock);
 
@@ -3555,6 +3551,9 @@ void ionic_lif_deinit(struct ionic_lif *lif)
 	ionic_lif_qcq_deinit(lif, lif->notifyqcq);
 	ionic_lif_qcq_deinit(lif, lif->adminqcq);
 
+	ionic_bus_unmap_dbpage(lif->ionic, lif->kern_dbpage);
+	lif->kern_dbpage = NULL;
+
 	ionic_lif_reset(lif);
 }
 
-- 
2.17.1

[PATCH net-next 3/3] ionic: cancel delayed work earlier in remove

[Shannon Nelson]

Cancel any entries on the delayed work queue before starting
to tear down the lif to be sure there is no race with any
other events.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index 4c377bdc62c8..136bfa3516d0 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -409,6 +409,7 @@ static void ionic_remove(struct pci_dev *pdev)
 	timer_shutdown_sync(&ionic->watchdog_timer);
 
 	if (ionic->lif) {
+		cancel_work_sync(&ionic->lif->deferred.work);
 		/* prevent adminq cmds if already known as down */
 		if (test_and_clear_bit(IONIC_LIF_F_FW_RESET, ionic->lif->state))
 			set_bit(IONIC_LIF_F_FW_STOPPING, ionic->lif->state);
-- 
2.17.1

Re: [PATCH net-next 1/3] ionic: print firmware heartbeat as unsigned

[Simon Horman]

On Mon, Jun 09, 2025 at 02:46:42PM -0700, Shannon Nelson wrote:
> The firmware heartbeat value is an unsigned number, and seeing
> a negative number when it gets big is a little disconcerting.
> Example:
>     ionic 0000:24:00.0: FW heartbeat stalled at -1342169688
> 
> Print using the unsigned flag.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 2/3] ionic: clean dbpage in de-init

[Simon Horman]

On Mon, Jun 09, 2025 at 02:46:43PM -0700, Shannon Nelson wrote:
> Since the kern_dbpage gets set up in ionic_lif_init() and that
> function's error path will clean it if needed, the kern_dbpage
> on teardown should be cleaned in ionic_lif_deinit(), not in
> ionic_lif_free().  As it is currently we get a double call
> to iounmap() on kern_dbpage if the PCI ionic fails setting up
> the lif.  One example of this is when firmware isn't responding
> to AdminQ requests and ionic's first AdminQ call fails to
> setup the NotifyQ.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 3/3] ionic: cancel delayed work earlier in remove

[Simon Horman]

On Mon, Jun 09, 2025 at 02:46:44PM -0700, Shannon Nelson wrote:
> Cancel any entries on the delayed work queue before starting
> to tear down the lif to be sure there is no race with any
> other events.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 1/3] ionic: print firmware heartbeat as unsigned

[Joe Damato]

On Mon, Jun 09, 2025 at 02:46:42PM -0700, Shannon Nelson wrote:
> The firmware heartbeat value is an unsigned number, and seeing
> a negative number when it gets big is a little disconcerting.
> Example:
>     ionic 0000:24:00.0: FW heartbeat stalled at -1342169688
> 
> Print using the unsigned flag.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_dev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net-next 2/3] ionic: clean dbpage in de-init

[Joe Damato]

On Mon, Jun 09, 2025 at 02:46:43PM -0700, Shannon Nelson wrote:
> Since the kern_dbpage gets set up in ionic_lif_init() and that
> function's error path will clean it if needed, the kern_dbpage
> on teardown should be cleaned in ionic_lif_deinit(), not in
> ionic_lif_free().  As it is currently we get a double call
> to iounmap() on kern_dbpage if the PCI ionic fails setting up
> the lif.  One example of this is when firmware isn't responding
> to AdminQ requests and ionic's first AdminQ call fails to
> setup the NotifyQ.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_lif.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 7707a9e53c43..48cb5d30b5f6 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -3526,10 +3526,6 @@ void ionic_lif_free(struct ionic_lif *lif)
>  	lif->info = NULL;
>  	lif->info_pa = 0;
>  
> -	/* unmap doorbell page */
> -	ionic_bus_unmap_dbpage(lif->ionic, lif->kern_dbpage);
> -	lif->kern_dbpage = NULL;
> -
>  	mutex_destroy(&lif->config_lock);
>  	mutex_destroy(&lif->queue_lock);
>  
> @@ -3555,6 +3551,9 @@ void ionic_lif_deinit(struct ionic_lif *lif)
>  	ionic_lif_qcq_deinit(lif, lif->notifyqcq);
>  	ionic_lif_qcq_deinit(lif, lif->adminqcq);
>  
> +	ionic_bus_unmap_dbpage(lif->ionic, lif->kern_dbpage);
> +	lif->kern_dbpage = NULL;
> +

Seems fine to me; my only minor nit that you can probably ignore is that the
code to unmap the page and set kern_dbpage to NULL is repeated, as you
mentioned, in ionic_lif_init's error path. Not sure if it's worth adding a
wrapper to avoid the repetition.

In either case:

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net-next 3/3] ionic: cancel delayed work earlier in remove

[Joe Damato]

On Mon, Jun 09, 2025 at 02:46:44PM -0700, Shannon Nelson wrote:
> Cancel any entries on the delayed work queue before starting
> to tear down the lif to be sure there is no race with any
> other events.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 4c377bdc62c8..136bfa3516d0 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -409,6 +409,7 @@ static void ionic_remove(struct pci_dev *pdev)
>  	timer_shutdown_sync(&ionic->watchdog_timer);
>  
>  	if (ionic->lif) {
> +		cancel_work_sync(&ionic->lif->deferred.work);
>  		/* prevent adminq cmds if already known as down */
>  		if (test_and_clear_bit(IONIC_LIF_F_FW_RESET, ionic->lif->state))
>  			set_bit(IONIC_LIF_F_FW_STOPPING, ionic->lif->state);

Wanted to note that it seems this could be called twice in this path?

Once above and then a second time in ionic_lif_deinit, although
ionic_lif_deinit seems to be called from other paths (like fw down?), so
that's probably OK.

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net-next 0/3] ionic: three little changes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon, 9 Jun 2025 14:46:41 -0700 you wrote:
> These are three little changes for the code from inspection
> and testing.
> 
> Shannon Nelson (3):
>   ionic: print firmware heartbeat as unsigned
>   ionic: clean dbpage in de-init
>   ionic: cancel delayed work earlier in remove
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] ionic: print firmware heartbeat as unsigned
    https://git.kernel.org/netdev/net-next/c/696158ff4dcd
  - [net-next,2/3] ionic: clean dbpage in de-init
    https://git.kernel.org/netdev/net-next/c/c9080abea1e6
  - [net-next,3/3] ionic: cancel delayed work earlier in remove
    https://git.kernel.org/netdev/net-next/c/52fdba899e6f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html