Re: [PATCH net-next 5/5] ionic: add FLR recovery support

[Ido Schimmel <idosch@idosch.org>]

On Tue, Jul 11, 2023 at 05:20:25PM -0700, Shannon Nelson wrote:
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index b141a29177df..8679d463e98a 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -320,6 +320,8 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (err)
>  		goto err_out;
>  
> +	pci_save_state(pdev);

Can you please explain why this is needed? See more below.

> +
>  	/* Allocate and init the LIF */
>  	err = ionic_lif_size(ionic);
>  	if (err) {
> @@ -408,12 +410,68 @@ static void ionic_remove(struct pci_dev *pdev)
>  	ionic_devlink_free(ionic);
>  }
>  
> +static void ionic_reset_prepare(struct pci_dev *pdev)
> +{
> +	struct ionic *ionic = pci_get_drvdata(pdev);
> +	struct ionic_lif *lif = ionic->lif;
> +
> +	dev_dbg(ionic->dev, "%s: device stopping\n", __func__);

Nit: You can use pci_dbg(pdev, ...);

> +
> +	del_timer_sync(&ionic->watchdog_timer);
> +	cancel_work_sync(&lif->deferred.work);
> +
> +	mutex_lock(&lif->queue_lock);
> +	ionic_stop_queues_reconfig(lif);
> +	ionic_txrx_free(lif);
> +	ionic_lif_deinit(lif);
> +	ionic_qcqs_free(lif);
> +	mutex_unlock(&lif->queue_lock);
> +
> +	ionic_dev_teardown(ionic);
> +	ionic_clear_pci(ionic);
> +	ionic_debugfs_del_dev(ionic);
> +}
> +
> +static void ionic_reset_done(struct pci_dev *pdev)
> +{
> +	struct ionic *ionic = pci_get_drvdata(pdev);
> +	struct ionic_lif *lif = ionic->lif;
> +	int err;
> +
> +	err = ionic_setup_one(ionic);
> +	if (err)
> +		goto err_out;
> +
> +	pci_restore_state(pdev);
> +	pci_save_state(pdev);

It's not clear to me why this is needed. Before issuing the reset, PCI
core calls pci_dev_save_and_disable() which saves the configuration
space of the device. After the reset, but before invoking the
reset_done() handler, PCI core restores the configuration space of the
device by calling pci_restore_state(). IOW, these calls seem to be
redundant.

I'm asking because I have patches that implement these handlers as well,
but I'm not calling pci_save_state() / pci_restore_state() in this flow
and it seems to work fine.

> +
> +	ionic_debugfs_add_sizes(ionic);
> +	ionic_debugfs_add_lif(ionic->lif);
> +
> +	err = ionic_restart_lif(lif);
> +	if (err)
> +		goto err_out;
> +
> +	mod_timer(&ionic->watchdog_timer, jiffies + 1);
> +
> +err_out:
> +	dev_dbg(ionic->dev, "%s: device recovery %s\n",
> +		__func__, err ? "failed" : "done");
> +}
> +
> +static const struct pci_error_handlers ionic_err_handler = {
> +	/* FLR handling */
> +	.reset_prepare      = ionic_reset_prepare,
> +	.reset_done         = ionic_reset_done,
> +};