| Message ID | 20230712002025.24444-6-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ionic: add FLR support | expand |
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, > +};
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, >> +};
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); + /* 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__); + + 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); + + 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, +}; + static struct pci_driver ionic_driver = { .name = IONIC_DRV_NAME, .id_table = ionic_id_table, .probe = ionic_probe, .remove = ionic_remove, .sriov_configure = ionic_sriov_configure, + .err_handler = &ionic_err_handler }; int ionic_bus_register_driver(void) diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c index 57ab25b9108e..e70952e508bf 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c @@ -434,7 +434,7 @@ static void ionic_qcq_free(struct ionic_lif *lif, struct ionic_qcq *qcq) } } -static void ionic_qcqs_free(struct ionic_lif *lif) +void ionic_qcqs_free(struct ionic_lif *lif) { struct device *dev = lif->ionic->dev; struct ionic_qcq *adminqcq; @@ -1759,7 +1759,7 @@ static int ionic_set_mac_address(struct net_device *netdev, void *sa) return ionic_lif_addr_add(netdev_priv(netdev), mac); } -static void ionic_stop_queues_reconfig(struct ionic_lif *lif) +void ionic_stop_queues_reconfig(struct ionic_lif *lif) { /* Stop and clean the queues before reconfiguration */ netif_device_detach(lif->netdev); @@ -2014,7 +2014,7 @@ static void ionic_txrx_deinit(struct ionic_lif *lif) } } -static void ionic_txrx_free(struct ionic_lif *lif) +void ionic_txrx_free(struct ionic_lif *lif) { unsigned int i; @@ -3271,7 +3271,7 @@ static void ionic_lif_handle_fw_down(struct ionic_lif *lif) dev_info(ionic->dev, "FW Down: LIFs stopped\n"); } -static int ionic_restart_lif(struct ionic_lif *lif) +int ionic_restart_lif(struct ionic_lif *lif) { struct ionic *ionic = lif->ionic; int err; diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h index fd2ea670e7d8..457c24195ca6 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h @@ -325,6 +325,11 @@ void ionic_lif_deinit(struct ionic_lif *lif); int ionic_lif_addr_add(struct ionic_lif *lif, const u8 *addr); int ionic_lif_addr_del(struct ionic_lif *lif, const u8 *addr); +void ionic_stop_queues_reconfig(struct ionic_lif *lif); +void ionic_txrx_free(struct ionic_lif *lif); +void ionic_qcqs_free(struct ionic_lif *lif); +int ionic_restart_lif(struct ionic_lif *lif); + int ionic_lif_register(struct ionic_lif *lif); void ionic_lif_unregister(struct ionic_lif *lif); int ionic_lif_identify(struct ionic *ionic, u8 lif_type,
Add support for the PCI reset handlers in order to manage an FLR event. Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> --- .../ethernet/pensando/ionic/ionic_bus_pci.c | 58 +++++++++++++++++++ .../net/ethernet/pensando/ionic/ionic_lif.c | 8 +-- .../net/ethernet/pensando/ionic/ionic_lif.h | 5 ++ 3 files changed, 67 insertions(+), 4 deletions(-)