| Message ID | 20190708192532.27420-20-snelson@pensando.io |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add ionic driver | expand |
Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote: >Add a devlink interface for access to information that isn't >normally available through ethtool or the iplink interface. > >Example: > $ ./devlink -j -p dev info pci/0000:b6:00.0 > { > "info": { > "pci/0000:b6:00.0": { > "driver": "ionic", > "serial_number": "FLM18420073", > "versions": { > "fixed": { > "fw_version": "0.11.0-50", > "fw_status": "0x1", > "fw_heartbeat": "0x716ce", > "asic_type": "0x0", > "asic_rev": "0x0" > } > } > } > } > } > >Signed-off-by: Shannon Nelson <snelson@pensando.io> >--- > drivers/net/ethernet/pensando/ionic/Makefile | 2 +- > drivers/net/ethernet/pensando/ionic/ionic.h | 1 + > .../ethernet/pensando/ionic/ionic_bus_pci.c | 7 ++ > .../ethernet/pensando/ionic/ionic_devlink.c | 89 +++++++++++++++++++ > .../ethernet/pensando/ionic/ionic_devlink.h | 12 +++ > 5 files changed, 110 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.c > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.h > >diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile >index 4f3cfbf36c23..ce187c7b33a8 100644 >--- a/drivers/net/ethernet/pensando/ionic/Makefile >+++ b/drivers/net/ethernet/pensando/ionic/Makefile >@@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o > > ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o \ > ionic_lif.o ionic_rx_filter.o ionic_txrx.o ionic_debugfs.o \ >- ionic_stats.o >+ ionic_stats.o ionic_devlink.o >diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h >index cd08166f73a9..a0034bc5b4a1 100644 >--- a/drivers/net/ethernet/pensando/ionic/ionic.h >+++ b/drivers/net/ethernet/pensando/ionic/ionic.h >@@ -44,6 +44,7 @@ struct ionic { > DECLARE_BITMAP(intrs, INTR_CTRL_REGS_MAX); > struct work_struct nb_work; > struct notifier_block nb; >+ struct devlink *dl; > }; > > struct ionic_admin_ctx { >diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >index 98c12b770c7f..a8c99254489f 100644 >--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >@@ -10,6 +10,7 @@ > #include "ionic_bus.h" > #include "ionic_lif.h" > #include "ionic_debugfs.h" >+#include "ionic_devlink.h" > > /* Supported devices */ > static const struct pci_device_id ionic_id_table[] = { >@@ -212,9 +213,14 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > goto err_out_deinit_lifs; > } > >+ err = ionic_devlink_register(ionic); >+ if (err) >+ dev_err(dev, "Cannot register devlink (ignored): %d\n", err); >+ > return 0; > > err_out_deinit_lifs: >+ ionic_devlink_unregister(ionic); > ionic_lifs_deinit(ionic); > err_out_free_lifs: > ionic_lifs_free(ionic); >@@ -247,6 +253,7 @@ static void ionic_remove(struct pci_dev *pdev) > struct ionic *ionic = pci_get_drvdata(pdev); > > if (ionic) { >+ ionic_devlink_unregister(ionic); > ionic_lifs_unregister(ionic); > ionic_lifs_deinit(ionic); > ionic_lifs_free(ionic); >diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c >new file mode 100644 >index 000000000000..fbbfcdde292f >--- /dev/null >+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c >@@ -0,0 +1,89 @@ >+// SPDX-License-Identifier: GPL-2.0 >+/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >+ >+#include <linux/module.h> >+#include <linux/netdevice.h> >+ >+#include "ionic.h" >+#include "ionic_bus.h" >+#include "ionic_lif.h" >+#include "ionic_devlink.h" >+ >+struct ionic_devlink { >+ struct ionic *ionic; >+}; >+ >+static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req, >+ struct netlink_ext_ack *extack) >+{ >+ struct ionic *ionic = *(struct ionic **)devlink_priv(dl); >+ struct ionic_dev *idev = &ionic->idev; >+ char buf[16]; >+ u32 val; >+ >+ devlink_info_driver_name_put(req, DRV_NAME); >+ >+ devlink_info_version_fixed_put(req, "fw_version", >+ idev->dev_info.fw_version); >+ >+ val = ioread8(&idev->dev_info_regs->fw_status); >+ snprintf(buf, sizeof(buf), "0x%x", val); >+ devlink_info_version_fixed_put(req, "fw_status", buf); >+ >+ val = ioread32(&idev->dev_info_regs->fw_heartbeat); >+ snprintf(buf, sizeof(buf), "0x%x", val); >+ devlink_info_version_fixed_put(req, "fw_heartbeat", buf); >+ >+ snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_type); >+ devlink_info_version_fixed_put(req, "asic_type", buf); >+ >+ snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_rev); >+ devlink_info_version_fixed_put(req, "asic_rev", buf); >+ >+ devlink_info_serial_number_put(req, idev->dev_info.serial_num); >+ >+ return 0; >+} >+ >+static const struct devlink_ops ionic_dl_ops = { >+ .info_get = ionic_dl_info_get, >+}; >+ >+int ionic_devlink_register(struct ionic *ionic) >+{ >+ struct devlink *dl; >+ struct ionic **ip; >+ int err; >+ >+ dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); Oups. Something is wrong with your flow. The devlink alloc is allocating the structure that holds private data (per-device data) for you. This is misuse :/ You are missing one parent device struct apparently. Oh, I think I see something like it. The unused "struct ionic_devlink". >+ if (!dl) { >+ dev_warn(ionic->dev, "devlink_alloc failed"); >+ return -ENOMEM; >+ } >+ >+ ip = (struct ionic **)devlink_priv(dl); >+ *ip = ionic; >+ ionic->dl = dl; >+ >+ err = devlink_register(dl, ionic->dev); >+ if (err) { >+ dev_warn(ionic->dev, "devlink_register failed: %d\n", err); >+ goto err_dl_free; >+ } >+ >+ return 0; >+ >+err_dl_free: >+ ionic->dl = NULL; >+ devlink_free(dl); >+ return err; >+} >+ >+void ionic_devlink_unregister(struct ionic *ionic) >+{ >+ if (!ionic->dl) >+ return; >+ >+ devlink_unregister(ionic->dl); >+ devlink_free(ionic->dl); >+} >diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >new file mode 100644 >index 000000000000..35528884e29f >--- /dev/null >+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >@@ -0,0 +1,12 @@ >+/* SPDX-License-Identifier: GPL-2.0 */ >+/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >+ >+#ifndef _IONIC_DEVLINK_H_ >+#define _IONIC_DEVLINK_H_ >+ >+#include <net/devlink.h> >+ >+int ionic_devlink_register(struct ionic *ionic); >+void ionic_devlink_unregister(struct ionic *ionic); >+ >+#endif /* _IONIC_DEVLINK_H_ */ >-- >2.17.1 >
On 7/8/19 12:34 PM, Jiri Pirko wrote: > Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote: >> Add a devlink interface for access to information that isn't >> normally available through ethtool or the iplink interface. >> >> Example: >> $ ./devlink -j -p dev info pci/0000:b6:00.0 >> { >> "info": { >> "pci/0000:b6:00.0": { >> "driver": "ionic", >> "serial_number": "FLM18420073", >> "versions": { >> "fixed": { >> "fw_version": "0.11.0-50", >> "fw_status": "0x1", >> "fw_heartbeat": "0x716ce", >> "asic_type": "0x0", >> "asic_rev": "0x0" >> } >> } >> } >> } >> } >> >> Signed-off-by: Shannon Nelson <snelson@pensando.io> >> --- >> drivers/net/ethernet/pensando/ionic/Makefile | 2 +- >> drivers/net/ethernet/pensando/ionic/ionic.h | 1 + >> .../ethernet/pensando/ionic/ionic_bus_pci.c | 7 ++ >> .../ethernet/pensando/ionic/ionic_devlink.c | 89 +++++++++++++++++++ >> .../ethernet/pensando/ionic/ionic_devlink.h | 12 +++ >> 5 files changed, 110 insertions(+), 1 deletion(-) >> create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.c >> create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> >> diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile >> index 4f3cfbf36c23..ce187c7b33a8 100644 >> --- a/drivers/net/ethernet/pensando/ionic/Makefile >> +++ b/drivers/net/ethernet/pensando/ionic/Makefile >> @@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o >> >> ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o \ >> ionic_lif.o ionic_rx_filter.o ionic_txrx.o ionic_debugfs.o \ >> - ionic_stats.o >> + ionic_stats.o ionic_devlink.o >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h >> index cd08166f73a9..a0034bc5b4a1 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic.h >> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h >> @@ -44,6 +44,7 @@ struct ionic { >> DECLARE_BITMAP(intrs, INTR_CTRL_REGS_MAX); >> struct work_struct nb_work; >> struct notifier_block nb; >> + struct devlink *dl; >> }; >> >> struct ionic_admin_ctx { >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> index 98c12b770c7f..a8c99254489f 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> @@ -10,6 +10,7 @@ >> #include "ionic_bus.h" >> #include "ionic_lif.h" >> #include "ionic_debugfs.h" >> +#include "ionic_devlink.h" >> >> /* Supported devices */ >> static const struct pci_device_id ionic_id_table[] = { >> @@ -212,9 +213,14 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) >> goto err_out_deinit_lifs; >> } >> >> + err = ionic_devlink_register(ionic); >> + if (err) >> + dev_err(dev, "Cannot register devlink (ignored): %d\n", err); >> + >> return 0; >> >> err_out_deinit_lifs: >> + ionic_devlink_unregister(ionic); >> ionic_lifs_deinit(ionic); >> err_out_free_lifs: >> ionic_lifs_free(ionic); >> @@ -247,6 +253,7 @@ static void ionic_remove(struct pci_dev *pdev) >> struct ionic *ionic = pci_get_drvdata(pdev); >> >> if (ionic) { >> + ionic_devlink_unregister(ionic); >> ionic_lifs_unregister(ionic); >> ionic_lifs_deinit(ionic); >> ionic_lifs_free(ionic); >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c >> new file mode 100644 >> index 000000000000..fbbfcdde292f >> --- /dev/null >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c >> @@ -0,0 +1,89 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >> + >> +#include <linux/module.h> >> +#include <linux/netdevice.h> >> + >> +#include "ionic.h" >> +#include "ionic_bus.h" >> +#include "ionic_lif.h" >> +#include "ionic_devlink.h" >> + >> +struct ionic_devlink { >> + struct ionic *ionic; >> +}; >> + >> +static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req, >> + struct netlink_ext_ack *extack) >> +{ >> + struct ionic *ionic = *(struct ionic **)devlink_priv(dl); >> + struct ionic_dev *idev = &ionic->idev; >> + char buf[16]; >> + u32 val; >> + >> + devlink_info_driver_name_put(req, DRV_NAME); >> + >> + devlink_info_version_fixed_put(req, "fw_version", >> + idev->dev_info.fw_version); >> + >> + val = ioread8(&idev->dev_info_regs->fw_status); >> + snprintf(buf, sizeof(buf), "0x%x", val); >> + devlink_info_version_fixed_put(req, "fw_status", buf); >> + >> + val = ioread32(&idev->dev_info_regs->fw_heartbeat); >> + snprintf(buf, sizeof(buf), "0x%x", val); >> + devlink_info_version_fixed_put(req, "fw_heartbeat", buf); >> + >> + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_type); >> + devlink_info_version_fixed_put(req, "asic_type", buf); >> + >> + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_rev); >> + devlink_info_version_fixed_put(req, "asic_rev", buf); >> + >> + devlink_info_serial_number_put(req, idev->dev_info.serial_num); >> + >> + return 0; >> +} >> + >> +static const struct devlink_ops ionic_dl_ops = { >> + .info_get = ionic_dl_info_get, >> +}; >> + >> +int ionic_devlink_register(struct ionic *ionic) >> +{ >> + struct devlink *dl; >> + struct ionic **ip; >> + int err; >> + >> + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); > Oups. Something is wrong with your flow. The devlink alloc is allocating > the structure that holds private data (per-device data) for you. This is > misuse :/ > > You are missing one parent device struct apparently. > > Oh, I think I see something like it. The unused "struct ionic_devlink". If I'm not mistaken, the alloc is only allocating enough for a pointer, not the whole per device struct, and a few lines down from here the pointer to the new devlink struct is assigned to ionic->dl. This was based on what I found in the qed driver's qed_devlink_register(), and it all seems to work. That unused struct ionic_devlink does need to go away, it was superfluous after working out a better typecast off of devlink_priv(). I'll remove the unused struct ionic_devlink, but I think the rest is okay. sln > > >> + if (!dl) { >> + dev_warn(ionic->dev, "devlink_alloc failed"); >> + return -ENOMEM; >> + } >> + >> + ip = (struct ionic **)devlink_priv(dl); >> + *ip = ionic; >> + ionic->dl = dl; >> + >> + err = devlink_register(dl, ionic->dev); >> + if (err) { >> + dev_warn(ionic->dev, "devlink_register failed: %d\n", err); >> + goto err_dl_free; >> + } >> + >> + return 0; >> + >> +err_dl_free: >> + ionic->dl = NULL; >> + devlink_free(dl); >> + return err; >> +} >> + >> +void ionic_devlink_unregister(struct ionic *ionic) >> +{ >> + if (!ionic->dl) >> + return; >> + >> + devlink_unregister(ionic->dl); >> + devlink_free(ionic->dl); >> +} >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> new file mode 100644 >> index 000000000000..35528884e29f >> --- /dev/null >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> @@ -0,0 +1,12 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >> + >> +#ifndef _IONIC_DEVLINK_H_ >> +#define _IONIC_DEVLINK_H_ >> + >> +#include <net/devlink.h> >> + >> +int ionic_devlink_register(struct ionic *ionic); >> +void ionic_devlink_unregister(struct ionic *ionic); >> + >> +#endif /* _IONIC_DEVLINK_H_ */ >> -- >> 2.17.1 >>
Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote: >On 7/8/19 12:34 PM, Jiri Pirko wrote: >> Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote: >> > Add a devlink interface for access to information that isn't >> > normally available through ethtool or the iplink interface. >> > >> > Example: >> > $ ./devlink -j -p dev info pci/0000:b6:00.0 >> > { >> > "info": { >> > "pci/0000:b6:00.0": { >> > "driver": "ionic", >> > "serial_number": "FLM18420073", >> > "versions": { >> > "fixed": { >> > "fw_version": "0.11.0-50", >> > "fw_status": "0x1", >> > "fw_heartbeat": "0x716ce", >> > "asic_type": "0x0", >> > "asic_rev": "0x0" >> > } >> > } >> > } >> > } >> > } >> > >> > Signed-off-by: Shannon Nelson <snelson@pensando.io> >> > --- >> > drivers/net/ethernet/pensando/ionic/Makefile | 2 +- >> > drivers/net/ethernet/pensando/ionic/ionic.h | 1 + >> > .../ethernet/pensando/ionic/ionic_bus_pci.c | 7 ++ >> > .../ethernet/pensando/ionic/ionic_devlink.c | 89 +++++++++++++++++++ >> > .../ethernet/pensando/ionic/ionic_devlink.h | 12 +++ >> > 5 files changed, 110 insertions(+), 1 deletion(-) >> > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.c >> > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> > >> > diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile >> > index 4f3cfbf36c23..ce187c7b33a8 100644 >> > --- a/drivers/net/ethernet/pensando/ionic/Makefile >> > +++ b/drivers/net/ethernet/pensando/ionic/Makefile >> > @@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o >> > >> > ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o \ >> > ionic_lif.o ionic_rx_filter.o ionic_txrx.o ionic_debugfs.o \ >> > - ionic_stats.o >> > + ionic_stats.o ionic_devlink.o >> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h >> > index cd08166f73a9..a0034bc5b4a1 100644 >> > --- a/drivers/net/ethernet/pensando/ionic/ionic.h >> > +++ b/drivers/net/ethernet/pensando/ionic/ionic.h >> > @@ -44,6 +44,7 @@ struct ionic { >> > DECLARE_BITMAP(intrs, INTR_CTRL_REGS_MAX); >> > struct work_struct nb_work; >> > struct notifier_block nb; >> > + struct devlink *dl; >> > }; >> > >> > struct ionic_admin_ctx { >> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> > index 98c12b770c7f..a8c99254489f 100644 >> > --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c >> > @@ -10,6 +10,7 @@ >> > #include "ionic_bus.h" >> > #include "ionic_lif.h" >> > #include "ionic_debugfs.h" >> > +#include "ionic_devlink.h" >> > >> > /* Supported devices */ >> > static const struct pci_device_id ionic_id_table[] = { >> > @@ -212,9 +213,14 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) >> > goto err_out_deinit_lifs; >> > } >> > >> > + err = ionic_devlink_register(ionic); >> > + if (err) >> > + dev_err(dev, "Cannot register devlink (ignored): %d\n", err); >> > + >> > return 0; >> > >> > err_out_deinit_lifs: >> > + ionic_devlink_unregister(ionic); >> > ionic_lifs_deinit(ionic); >> > err_out_free_lifs: >> > ionic_lifs_free(ionic); >> > @@ -247,6 +253,7 @@ static void ionic_remove(struct pci_dev *pdev) >> > struct ionic *ionic = pci_get_drvdata(pdev); >> > >> > if (ionic) { >> > + ionic_devlink_unregister(ionic); >> > ionic_lifs_unregister(ionic); >> > ionic_lifs_deinit(ionic); >> > ionic_lifs_free(ionic); >> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c >> > new file mode 100644 >> > index 000000000000..fbbfcdde292f >> > --- /dev/null >> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c >> > @@ -0,0 +1,89 @@ >> > +// SPDX-License-Identifier: GPL-2.0 >> > +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >> > + >> > +#include <linux/module.h> >> > +#include <linux/netdevice.h> >> > + >> > +#include "ionic.h" >> > +#include "ionic_bus.h" >> > +#include "ionic_lif.h" >> > +#include "ionic_devlink.h" >> > + >> > +struct ionic_devlink { >> > + struct ionic *ionic; >> > +}; >> > + >> > +static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ionic *ionic = *(struct ionic **)devlink_priv(dl); >> > + struct ionic_dev *idev = &ionic->idev; >> > + char buf[16]; >> > + u32 val; >> > + >> > + devlink_info_driver_name_put(req, DRV_NAME); >> > + >> > + devlink_info_version_fixed_put(req, "fw_version", >> > + idev->dev_info.fw_version); >> > + >> > + val = ioread8(&idev->dev_info_regs->fw_status); >> > + snprintf(buf, sizeof(buf), "0x%x", val); >> > + devlink_info_version_fixed_put(req, "fw_status", buf); >> > + >> > + val = ioread32(&idev->dev_info_regs->fw_heartbeat); >> > + snprintf(buf, sizeof(buf), "0x%x", val); >> > + devlink_info_version_fixed_put(req, "fw_heartbeat", buf); >> > + >> > + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_type); >> > + devlink_info_version_fixed_put(req, "asic_type", buf); >> > + >> > + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_rev); >> > + devlink_info_version_fixed_put(req, "asic_rev", buf); >> > + >> > + devlink_info_serial_number_put(req, idev->dev_info.serial_num); >> > + >> > + return 0; >> > +} >> > + >> > +static const struct devlink_ops ionic_dl_ops = { >> > + .info_get = ionic_dl_info_get, >> > +}; >> > + >> > +int ionic_devlink_register(struct ionic *ionic) >> > +{ >> > + struct devlink *dl; >> > + struct ionic **ip; >> > + int err; >> > + >> > + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); >> Oups. Something is wrong with your flow. The devlink alloc is allocating >> the structure that holds private data (per-device data) for you. This is >> misuse :/ >> >> You are missing one parent device struct apparently. >> >> Oh, I think I see something like it. The unused "struct ionic_devlink". > >If I'm not mistaken, the alloc is only allocating enough for a pointer, not >the whole per device struct, and a few lines down from here the pointer to >the new devlink struct is assigned to ionic->dl. This was based on what I >found in the qed driver's qed_devlink_register(), and it all seems to work. I'm not saying your code won't work. What I say is that you should have a struct for device that would be allocated by devlink_alloc() The ionic struct should be associated with devlink_port. That you are missing too. > >That unused struct ionic_devlink does need to go away, it was superfluous >after working out a better typecast off of devlink_priv(). > >I'll remove the unused struct ionic_devlink, but I think the rest is okay. > >sln > >> >> >> > + if (!dl) { >> > + dev_warn(ionic->dev, "devlink_alloc failed"); >> > + return -ENOMEM; >> > + } >> > + >> > + ip = (struct ionic **)devlink_priv(dl); >> > + *ip = ionic; >> > + ionic->dl = dl; >> > + >> > + err = devlink_register(dl, ionic->dev); >> > + if (err) { >> > + dev_warn(ionic->dev, "devlink_register failed: %d\n", err); >> > + goto err_dl_free; >> > + } >> > + >> > + return 0; >> > + >> > +err_dl_free: >> > + ionic->dl = NULL; >> > + devlink_free(dl); >> > + return err; >> > +} >> > + >> > +void ionic_devlink_unregister(struct ionic *ionic) >> > +{ >> > + if (!ionic->dl) >> > + return; >> > + >> > + devlink_unregister(ionic->dl); >> > + devlink_free(ionic->dl); >> > +} >> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> > new file mode 100644 >> > index 000000000000..35528884e29f >> > --- /dev/null >> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> > @@ -0,0 +1,12 @@ >> > +/* SPDX-License-Identifier: GPL-2.0 */ >> > +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >> > + >> > +#ifndef _IONIC_DEVLINK_H_ >> > +#define _IONIC_DEVLINK_H_ >> > + >> > +#include <net/devlink.h> >> > + >> > +int ionic_devlink_register(struct ionic *ionic); >> > +void ionic_devlink_unregister(struct ionic *ionic); >> > + >> > +#endif /* _IONIC_DEVLINK_H_ */ >> > -- >> > 2.17.1 >> > >
On 7/8/19 1:03 PM, Jiri Pirko wrote: > Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote: >> On 7/8/19 12:34 PM, Jiri Pirko wrote: >>> Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote: >>>> >>>> + >>>> +static const struct devlink_ops ionic_dl_ops = { >>>> + .info_get = ionic_dl_info_get, >>>> +}; >>>> + >>>> +int ionic_devlink_register(struct ionic *ionic) >>>> +{ >>>> + struct devlink *dl; >>>> + struct ionic **ip; >>>> + int err; >>>> + >>>> + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); >>> Oups. Something is wrong with your flow. The devlink alloc is allocating >>> the structure that holds private data (per-device data) for you. This is >>> misuse :/ >>> >>> You are missing one parent device struct apparently. >>> >>> Oh, I think I see something like it. The unused "struct ionic_devlink". >> If I'm not mistaken, the alloc is only allocating enough for a pointer, not >> the whole per device struct, and a few lines down from here the pointer to >> the new devlink struct is assigned to ionic->dl. This was based on what I >> found in the qed driver's qed_devlink_register(), and it all seems to work. > I'm not saying your code won't work. What I say is that you should have > a struct for device that would be allocated by devlink_alloc() Is there a particular reason why? I appreciate that devlink_alloc() can give you this device specific space, just as alloc_etherdev_mq() can, but is there a specific reason why this should be used instead of setting up simply a pointer to a space that has already been allocated? There are several drivers that are using it the way I've setup here, which happened to be the first examples I followed - are they doing something different that makes this valid for them? > > The ionic struct should be associated with devlink_port. That you are > missing too. We don't support any of devlink_port features at this point, just the simple device information. sln > > >> That unused struct ionic_devlink does need to go away, it was superfluous >> after working out a better typecast off of devlink_priv(). >> >> I'll remove the unused struct ionic_devlink, but I think the rest is okay. >> >> sln >> >>> >>>> + if (!dl) { >>>> + dev_warn(ionic->dev, "devlink_alloc failed"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + ip = (struct ionic **)devlink_priv(dl); >>>> + *ip = ionic; >>>> + ionic->dl = dl; >>>> + >>>> + err = devlink_register(dl, ionic->dev); >>>> + if (err) { >>>> + dev_warn(ionic->dev, "devlink_register failed: %d\n", err); >>>> + goto err_dl_free; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> +err_dl_free: >>>> + ionic->dl = NULL; >>>> + devlink_free(dl); >>>> + return err; >>>> +} >>>> + >>>> +void ionic_devlink_unregister(struct ionic *ionic) >>>> +{ >>>> + if (!ionic->dl) >>>> + return; >>>> + >>>> + devlink_unregister(ionic->dl); >>>> + devlink_free(ionic->dl); >>>> +} >>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >>>> new file mode 100644 >>>> index 000000000000..35528884e29f >>>> --- /dev/null >>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >>>> @@ -0,0 +1,12 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >>>> + >>>> +#ifndef _IONIC_DEVLINK_H_ >>>> +#define _IONIC_DEVLINK_H_ >>>> + >>>> +#include <net/devlink.h> >>>> + >>>> +int ionic_devlink_register(struct ionic *ionic); >>>> +void ionic_devlink_unregister(struct ionic *ionic); >>>> + >>>> +#endif /* _IONIC_DEVLINK_H_ */ >>>> -- >>>> 2.17.1 >>>>
On Mon, 8 Jul 2019 12:25:32 -0700, Shannon Nelson wrote: > Add a devlink interface for access to information that isn't > normally available through ethtool or the iplink interface. > > Example: > $ ./devlink -j -p dev info pci/0000:b6:00.0 > { > "info": { > "pci/0000:b6:00.0": { > "driver": "ionic", > "serial_number": "FLM18420073", > "versions": { > "fixed": { > "fw_version": "0.11.0-50", Hm. Fixed is for hardware components. Seeing FW version reported as fixed seems counter intuitive. You probably want "running"? > "fw_status": "0x1", I don't think this is the right interface to report status-like information. Perhaps devlink health reporters? > "fw_heartbeat": "0x716ce", Ditto, perhaps best to report it in health stuff? > "asic_type": "0x0", > "asic_rev": "0x0" These seem like legit "fixed" versions 👍 > } > } > } > } > } > > Signed-off-by: Shannon Nelson <snelson@pensando.io> Isn't this a new patch? Perhaps you'd be best off upstreaming the first batch of support and add features later? It'd be easier on reviewers so we don't have to keep re-checking the first 16 patches..
Tue, Jul 09, 2019 at 12:58:00AM CEST, snelson@pensando.io wrote: >On 7/8/19 1:03 PM, Jiri Pirko wrote: >> Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote: >> > On 7/8/19 12:34 PM, Jiri Pirko wrote: >> > > Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote: >> > > > >> > > > + >> > > > +static const struct devlink_ops ionic_dl_ops = { >> > > > + .info_get = ionic_dl_info_get, >> > > > +}; >> > > > + >> > > > +int ionic_devlink_register(struct ionic *ionic) >> > > > +{ >> > > > + struct devlink *dl; >> > > > + struct ionic **ip; >> > > > + int err; >> > > > + >> > > > + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); >> > > Oups. Something is wrong with your flow. The devlink alloc is allocating >> > > the structure that holds private data (per-device data) for you. This is >> > > misuse :/ >> > > >> > > You are missing one parent device struct apparently. >> > > >> > > Oh, I think I see something like it. The unused "struct ionic_devlink". >> > If I'm not mistaken, the alloc is only allocating enough for a pointer, not >> > the whole per device struct, and a few lines down from here the pointer to >> > the new devlink struct is assigned to ionic->dl. This was based on what I >> > found in the qed driver's qed_devlink_register(), and it all seems to work. >> I'm not saying your code won't work. What I say is that you should have >> a struct for device that would be allocated by devlink_alloc() > >Is there a particular reason why? I appreciate that devlink_alloc() can give >you this device specific space, just as alloc_etherdev_mq() can, but is there Yes. Devlink manipulates with the whole device. However, alloc_etherdev_mq() allocates only net_device. These are 2 different things. devlink port relates 1:1 to net_device. However, devlink instance can have multiple ports. What I say is do it correctly. >a specific reason why this should be used instead of setting up simply a >pointer to a space that has already been allocated? There are several >drivers that are using it the way I've setup here, which happened to be the >first examples I followed - are they doing something different that makes >this valid for them? Nope. I'll look at that and fix. > >> >> The ionic struct should be associated with devlink_port. That you are >> missing too. > >We don't support any of devlink_port features at this point, just the simple >device information. No problem, you can still register devlink_port. You don't have to do much in order to do so. > >sln > >> >> >> > That unused struct ionic_devlink does need to go away, it was superfluous >> > after working out a better typecast off of devlink_priv(). >> > >> > I'll remove the unused struct ionic_devlink, but I think the rest is okay. >> > >> > sln >> > >> > > >> > > > + if (!dl) { >> > > > + dev_warn(ionic->dev, "devlink_alloc failed"); >> > > > + return -ENOMEM; >> > > > + } >> > > > + >> > > > + ip = (struct ionic **)devlink_priv(dl); >> > > > + *ip = ionic; >> > > > + ionic->dl = dl; >> > > > + >> > > > + err = devlink_register(dl, ionic->dev); >> > > > + if (err) { >> > > > + dev_warn(ionic->dev, "devlink_register failed: %d\n", err); >> > > > + goto err_dl_free; >> > > > + } >> > > > + >> > > > + return 0; >> > > > + >> > > > +err_dl_free: >> > > > + ionic->dl = NULL; >> > > > + devlink_free(dl); >> > > > + return err; >> > > > +} >> > > > + >> > > > +void ionic_devlink_unregister(struct ionic *ionic) >> > > > +{ >> > > > + if (!ionic->dl) >> > > > + return; >> > > > + >> > > > + devlink_unregister(ionic->dl); >> > > > + devlink_free(ionic->dl); >> > > > +} >> > > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> > > > new file mode 100644 >> > > > index 000000000000..35528884e29f >> > > > --- /dev/null >> > > > +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h >> > > > @@ -0,0 +1,12 @@ >> > > > +/* SPDX-License-Identifier: GPL-2.0 */ >> > > > +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ >> > > > + >> > > > +#ifndef _IONIC_DEVLINK_H_ >> > > > +#define _IONIC_DEVLINK_H_ >> > > > + >> > > > +#include <net/devlink.h> >> > > > + >> > > > +int ionic_devlink_register(struct ionic *ionic); >> > > > +void ionic_devlink_unregister(struct ionic *ionic); >> > > > + >> > > > +#endif /* _IONIC_DEVLINK_H_ */ >> > > > -- >> > > > 2.17.1 >> > > > >
On 7/8/19 6:26 PM, Jakub Kicinski wrote: > On Mon, 8 Jul 2019 12:25:32 -0700, Shannon Nelson wrote: >> Add a devlink interface for access to information that isn't >> normally available through ethtool or the iplink interface. >> >> Example: >> $ ./devlink -j -p dev info pci/0000:b6:00.0 >> { >> "info": { >> "pci/0000:b6:00.0": { >> "driver": "ionic", >> "serial_number": "FLM18420073", >> "versions": { >> "fixed": { >> "fw_version": "0.11.0-50", > Hm. Fixed is for hardware components. Seeing FW version reported as > fixed seems counter intuitive. You probably want "running"? Sure. > >> "fw_status": "0x1", > I don't think this is the right interface to report status-like > information. Perhaps devlink health reporters? > >> "fw_heartbeat": "0x716ce", > Ditto, perhaps best to report it in health stuff? I haven't dug too far into the health stuff, but on the surface it looks like a lot of infrastructure for a couple of simple values. I'm tempted to put them back into debugfs for the moment rather than add that much more interface goo. > >> "asic_type": "0x0", >> "asic_rev": "0x0" > These seem like legit "fixed" versions 👍 > >> } >> } >> } >> } >> } >> >> Signed-off-by: Shannon Nelson <snelson@pensando.io> > Isn't this a new patch? Perhaps you'd be best off upstreaming the > first batch of support and add features later? It'd be easier on > reviewers so we don't have to keep re-checking the first 16 patches.. Yes, and I commented about this in v2 notes: in the tension between trying to address comments, keep the line count down, keep the basic feature set, and keep the patches self-consistent and simple, I added this one patch for the devlink goodies that were requested. At least the total line count went down. sln
On 7/8/19 11:56 PM, Jiri Pirko wrote: > Tue, Jul 09, 2019 at 12:58:00AM CEST, snelson@pensando.io wrote: >> On 7/8/19 1:03 PM, Jiri Pirko wrote: >>> Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote: >>>> On 7/8/19 12:34 PM, Jiri Pirko wrote: >>>>> Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote: >>>>>> + >>>>>> +static const struct devlink_ops ionic_dl_ops = { >>>>>> + .info_get = ionic_dl_info_get, >>>>>> +}; >>>>>> + >>>>>> +int ionic_devlink_register(struct ionic *ionic) >>>>>> +{ >>>>>> + struct devlink *dl; >>>>>> + struct ionic **ip; >>>>>> + int err; >>>>>> + >>>>>> + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); >>>>> Oups. Something is wrong with your flow. The devlink alloc is allocating >>>>> the structure that holds private data (per-device data) for you. This is >>>>> misuse :/ >>>>> >>>>> You are missing one parent device struct apparently. >>>>> >>>>> Oh, I think I see something like it. The unused "struct ionic_devlink". >>>> If I'm not mistaken, the alloc is only allocating enough for a pointer, not >>>> the whole per device struct, and a few lines down from here the pointer to >>>> the new devlink struct is assigned to ionic->dl. This was based on what I >>>> found in the qed driver's qed_devlink_register(), and it all seems to work. >>> I'm not saying your code won't work. What I say is that you should have >>> a struct for device that would be allocated by devlink_alloc() >> Is there a particular reason why? I appreciate that devlink_alloc() can give >> you this device specific space, just as alloc_etherdev_mq() can, but is there > Yes. Devlink manipulates with the whole device. However, > alloc_etherdev_mq() allocates only net_device. These are 2 different > things. devlink port relates 1:1 to net_device. However, devlink > instance can have multiple ports. What I say is do it correctly. So what you are saying is that anyone who wants to add even the smallest devlink feature to their driver needs to rework their basic device memory setup to do it the devlink way. I can see where some folks may have a problem with this. > > >> a specific reason why this should be used instead of setting up simply a >> pointer to a space that has already been allocated? There are several >> drivers that are using it the way I've setup here, which happened to be the >> first examples I followed - are they doing something different that makes >> this valid for them? > Nope. I'll look at that and fix. > > >>> The ionic struct should be associated with devlink_port. That you are >>> missing too. >> We don't support any of devlink_port features at this point, just the simple >> device information. > No problem, you can still register devlink_port. You don't have to do > much in order to do so. Is there any write-up to help guide developers new to devlink in using the interface correctly? I haven't found much yet, but perhaps I've missed something. The manpages are somewhat useful in showing what the user might do, but they really don't help much in guiding the developer through these details. sln
Tue, Jul 09, 2019 at 09:13:53PM CEST, snelson@pensando.io wrote: >On 7/8/19 11:56 PM, Jiri Pirko wrote: >> Tue, Jul 09, 2019 at 12:58:00AM CEST, snelson@pensando.io wrote: >> > On 7/8/19 1:03 PM, Jiri Pirko wrote: >> > > Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote: >> > > > On 7/8/19 12:34 PM, Jiri Pirko wrote: >> > > > > Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote: >> > > > > > + >> > > > > > +static const struct devlink_ops ionic_dl_ops = { >> > > > > > + .info_get = ionic_dl_info_get, >> > > > > > +}; >> > > > > > + >> > > > > > +int ionic_devlink_register(struct ionic *ionic) >> > > > > > +{ >> > > > > > + struct devlink *dl; >> > > > > > + struct ionic **ip; >> > > > > > + int err; >> > > > > > + >> > > > > > + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); >> > > > > Oups. Something is wrong with your flow. The devlink alloc is allocating >> > > > > the structure that holds private data (per-device data) for you. This is >> > > > > misuse :/ >> > > > > >> > > > > You are missing one parent device struct apparently. >> > > > > >> > > > > Oh, I think I see something like it. The unused "struct ionic_devlink". >> > > > If I'm not mistaken, the alloc is only allocating enough for a pointer, not >> > > > the whole per device struct, and a few lines down from here the pointer to >> > > > the new devlink struct is assigned to ionic->dl. This was based on what I >> > > > found in the qed driver's qed_devlink_register(), and it all seems to work. >> > > I'm not saying your code won't work. What I say is that you should have >> > > a struct for device that would be allocated by devlink_alloc() >> > Is there a particular reason why? I appreciate that devlink_alloc() can give >> > you this device specific space, just as alloc_etherdev_mq() can, but is there >> Yes. Devlink manipulates with the whole device. However, >> alloc_etherdev_mq() allocates only net_device. These are 2 different >> things. devlink port relates 1:1 to net_device. However, devlink >> instance can have multiple ports. What I say is do it correctly. > >So what you are saying is that anyone who wants to add even the smallest >devlink feature to their driver needs to rework their basic device memory >setup to do it the devlink way. I can see where some folks may have a >problem with this. It's just about having a structure to hold device data. You don't have to rework anything, just add this small one. > >> >> >> > a specific reason why this should be used instead of setting up simply a >> > pointer to a space that has already been allocated? There are several >> > drivers that are using it the way I've setup here, which happened to be the >> > first examples I followed - are they doing something different that makes >> > this valid for them? >> Nope. I'll look at that and fix. >> >> >> > > The ionic struct should be associated with devlink_port. That you are >> > > missing too. >> > We don't support any of devlink_port features at this point, just the simple >> > device information. >> No problem, you can still register devlink_port. You don't have to do >> much in order to do so. > >Is there any write-up to help guide developers new to devlink in using the >interface correctly? I haven't found much yet, but perhaps I've missed >something. The manpages are somewhat useful in showing what the user might >do, but they really don't help much in guiding the developer through these >details. That is not job of a manpage. See the rest of the code to get inspired. > >sln >
On 7/9/19 11:48 PM, Jiri Pirko wrote: > Tue, Jul 09, 2019 at 09:13:53PM CEST, snelson@pensando.io wrote: >> On 7/8/19 11:56 PM, Jiri Pirko wrote: >>> Tue, Jul 09, 2019 at 12:58:00AM CEST, snelson@pensando.io wrote: >>>> On 7/8/19 1:03 PM, Jiri Pirko wrote: >>>>> Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote: >>>>>> If I'm not mistaken, the alloc is only allocating enough for a pointer, not >>>>>> the whole per device struct, and a few lines down from here the pointer to >>>>>> the new devlink struct is assigned to ionic->dl. This was based on what I >>>>>> found in the qed driver's qed_devlink_register(), and it all seems to work. >>>>> I'm not saying your code won't work. What I say is that you should have >>>>> a struct for device that would be allocated by devlink_alloc() >>>> Is there a particular reason why? I appreciate that devlink_alloc() can give >>>> you this device specific space, just as alloc_etherdev_mq() can, but is there >>> Yes. Devlink manipulates with the whole device. However, >>> alloc_etherdev_mq() allocates only net_device. These are 2 different >>> things. devlink port relates 1:1 to net_device. However, devlink >>> instance can have multiple ports. What I say is do it correctly. >> So what you are saying is that anyone who wants to add even the smallest >> devlink feature to their driver needs to rework their basic device memory >> setup to do it the devlink way. I can see where some folks may have a >> problem with this. > It's just about having a structure to hold device data. You don't have > to rework anything, just add this small one. Well, there's a bit of logic rework to and a little data twiddling - not too bad in our case. Others may not be thrilled depending on how they've already implemented their drivers. >>>>> The ionic struct should be associated with devlink_port. That you are >>>>> missing too. >>>> We don't support any of devlink_port features at this point, just the simple >>>> device information. >>> No problem, you can still register devlink_port. You don't have to do >>> much in order to do so. >> Is there any write-up to help guide developers new to devlink in using the >> interface correctly? I haven't found much yet, but perhaps I've missed >> something. The manpages are somewhat useful in showing what the user might >> do, but they really don't help much in guiding the developer through these >> details. > That is not job of a manpage. See the rest of the code to get inspired. > Sure, we should all be able to poke through the code and figure out the basics - "use the Force, read the source" - but as software engineers we should be including some bits of documentation to help those new to the feature to steer away from pitfalls and use the feature correctly. We're all busy with our own projects and only have limited time to dig into and understand someone else's code; if there's not a guide, we'll do what we can to get it working and then move on, with no guarantee that we followed the original intent. There's a Documentation page on the devlink-health feature, and a brief bit on devlink-params, but I haven't seen anything yet that spells out the "proper" way to use the devlink framework. Of course, the open-source spirit is for me to scratch my own itch and take care of the need myself: I'd be happy to get a brief doc started, but if the original developers can take a few minutes to at least sketch some notes down about important bits like "the device struct should be associated with devlink_port" and why it should, then we have a chance at saving a lot of other people's time, and perhaps we can fill out the details correctly and not miss something important. sln
diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile index 4f3cfbf36c23..ce187c7b33a8 100644 --- a/drivers/net/ethernet/pensando/ionic/Makefile +++ b/drivers/net/ethernet/pensando/ionic/Makefile @@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o \ ionic_lif.o ionic_rx_filter.o ionic_txrx.o ionic_debugfs.o \ - ionic_stats.o + ionic_stats.o ionic_devlink.o diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h index cd08166f73a9..a0034bc5b4a1 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic.h +++ b/drivers/net/ethernet/pensando/ionic/ionic.h @@ -44,6 +44,7 @@ struct ionic { DECLARE_BITMAP(intrs, INTR_CTRL_REGS_MAX); struct work_struct nb_work; struct notifier_block nb; + struct devlink *dl; }; struct ionic_admin_ctx { diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c index 98c12b770c7f..a8c99254489f 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c @@ -10,6 +10,7 @@ #include "ionic_bus.h" #include "ionic_lif.h" #include "ionic_debugfs.h" +#include "ionic_devlink.h" /* Supported devices */ static const struct pci_device_id ionic_id_table[] = { @@ -212,9 +213,14 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) goto err_out_deinit_lifs; } + err = ionic_devlink_register(ionic); + if (err) + dev_err(dev, "Cannot register devlink (ignored): %d\n", err); + return 0; err_out_deinit_lifs: + ionic_devlink_unregister(ionic); ionic_lifs_deinit(ionic); err_out_free_lifs: ionic_lifs_free(ionic); @@ -247,6 +253,7 @@ static void ionic_remove(struct pci_dev *pdev) struct ionic *ionic = pci_get_drvdata(pdev); if (ionic) { + ionic_devlink_unregister(ionic); ionic_lifs_unregister(ionic); ionic_lifs_deinit(ionic); ionic_lifs_free(ionic); diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c new file mode 100644 index 000000000000..fbbfcdde292f --- /dev/null +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ + +#include <linux/module.h> +#include <linux/netdevice.h> + +#include "ionic.h" +#include "ionic_bus.h" +#include "ionic_lif.h" +#include "ionic_devlink.h" + +struct ionic_devlink { + struct ionic *ionic; +}; + +static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req, + struct netlink_ext_ack *extack) +{ + struct ionic *ionic = *(struct ionic **)devlink_priv(dl); + struct ionic_dev *idev = &ionic->idev; + char buf[16]; + u32 val; + + devlink_info_driver_name_put(req, DRV_NAME); + + devlink_info_version_fixed_put(req, "fw_version", + idev->dev_info.fw_version); + + val = ioread8(&idev->dev_info_regs->fw_status); + snprintf(buf, sizeof(buf), "0x%x", val); + devlink_info_version_fixed_put(req, "fw_status", buf); + + val = ioread32(&idev->dev_info_regs->fw_heartbeat); + snprintf(buf, sizeof(buf), "0x%x", val); + devlink_info_version_fixed_put(req, "fw_heartbeat", buf); + + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_type); + devlink_info_version_fixed_put(req, "asic_type", buf); + + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_rev); + devlink_info_version_fixed_put(req, "asic_rev", buf); + + devlink_info_serial_number_put(req, idev->dev_info.serial_num); + + return 0; +} + +static const struct devlink_ops ionic_dl_ops = { + .info_get = ionic_dl_info_get, +}; + +int ionic_devlink_register(struct ionic *ionic) +{ + struct devlink *dl; + struct ionic **ip; + int err; + + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *)); + if (!dl) { + dev_warn(ionic->dev, "devlink_alloc failed"); + return -ENOMEM; + } + + ip = (struct ionic **)devlink_priv(dl); + *ip = ionic; + ionic->dl = dl; + + err = devlink_register(dl, ionic->dev); + if (err) { + dev_warn(ionic->dev, "devlink_register failed: %d\n", err); + goto err_dl_free; + } + + return 0; + +err_dl_free: + ionic->dl = NULL; + devlink_free(dl); + return err; +} + +void ionic_devlink_unregister(struct ionic *ionic) +{ + if (!ionic->dl) + return; + + devlink_unregister(ionic->dl); + devlink_free(ionic->dl); +} diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h new file mode 100644 index 000000000000..35528884e29f --- /dev/null +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */ + +#ifndef _IONIC_DEVLINK_H_ +#define _IONIC_DEVLINK_H_ + +#include <net/devlink.h> + +int ionic_devlink_register(struct ionic *ionic); +void ionic_devlink_unregister(struct ionic *ionic); + +#endif /* _IONIC_DEVLINK_H_ */
Add a devlink interface for access to information that isn't normally available through ethtool or the iplink interface. Example: $ ./devlink -j -p dev info pci/0000:b6:00.0 { "info": { "pci/0000:b6:00.0": { "driver": "ionic", "serial_number": "FLM18420073", "versions": { "fixed": { "fw_version": "0.11.0-50", "fw_status": "0x1", "fw_heartbeat": "0x716ce", "asic_type": "0x0", "asic_rev": "0x0" } } } } } Signed-off-by: Shannon Nelson <snelson@pensando.io> --- drivers/net/ethernet/pensando/ionic/Makefile | 2 +- drivers/net/ethernet/pensando/ionic/ionic.h | 1 + .../ethernet/pensando/ionic/ionic_bus_pci.c | 7 ++ .../ethernet/pensando/ionic/ionic_devlink.c | 89 +++++++++++++++++++ .../ethernet/pensando/ionic/ionic_devlink.h | 12 +++ 5 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.c create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.h