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

[Shannon Nelson <shannon.nelson@amd.com>]

On 7/12/23 2:01 AM, Ido Schimmel wrote:
> 
> 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.

I want to say that I took that from other examples, but I suspect I was 
looking at some older .slot_reset handlers which use them.

Yes, I believe you are right that these are already handled in the pci.c 
code that calls into our handlers.  It is redundant - probably doesn't 
hurt, but isn't needed.  I can pull those out in a v2.

Thanks,
sln


> 
>> +
>> +     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,
>> +};