can: kvaser_pciefd: Simplify identification of physical CAN interfaces
Subsystems (5)
v4 cover can: kvaser_pciefd: Simplify identification of physical CAN interfaces
This patch series simplifies the process of identifying which network interface (can0..canX) corresponds to which physical CAN channel on Kvaser PCIe based CAN interfaces.
Changes in v4:
- Fix transient Sparse warning
- Add tag Reviewed-by Vincent Mailhol
Changes in v3:
- Fixed typo; kvaser_pcied -> kvaser_pciefd in documentation patch
Changes in v2:
- Replace use of netdev.dev_id with netdev.dev_port
- Formatting and refactoring
- New patch with devlink documentation
Jimmy Assarsson (10):
can: kvaser_pciefd: Add support to control CAN LEDs on device
can: kvaser_pciefd: Add support for ethtool set_phys_id()
can: kvaser_pciefd: Add intermediate variable for device struct in
probe()
can: kvaser_pciefd: Store the different firmware version components in
a struct
can: kvaser_pciefd: Store device channel index
can: kvaser_pciefd: Split driver into C-file and header-file.
can: kvaser_pciefd: Add devlink support
can: kvaser_pciefd: Expose device firmware version via devlink
info_get() can: kvaser_pciefd: Add devlink port support
Documentation: devlink: add devlink documentation for the
kvaser_pciefd driver
Documentation/networking/devlink/index.rst | 1 +
.../networking/devlink/kvaser_pciefd.rst | 24 +++
drivers/net/can/Kconfig | 1 +
drivers/net/can/Makefile | 2 +-
drivers/net/can/kvaser_pciefd/Makefile | 3 +
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h | 96 ++++++++++++
.../kvaser_pciefd_core.c} | 144 +++++++++---------
.../can/kvaser_pciefd/kvaser_pciefd_devlink.c | 60 ++++++++
8 files changed, 257 insertions(+), 74 deletions(-)
create mode 100644 Documentation/networking/devlink/kvaser_pciefd.rst
create mode 100644 drivers/net/can/kvaser_pciefd/Makefile
create mode 100644 drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
rename drivers/net/can/{kvaser_pciefd.c => kvaser_pciefd/kvaser_pciefd_core.c} (96%)
create mode 100644 drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.cSignature · 1 line
2.49.0
PATCH v4 1/10 can: kvaser_pciefd: Add support to control CAN LEDs on device
Add support to turn on/off CAN LEDs on device.
Turn off all CAN LEDs in probe, since they are default on after a reset or power on.
---
Changes in v4:
- Add tag Reviewed-by Vincent Mailhol
drivers/net/can/kvaser_pciefd.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/net/can/kvaser_pciefd.c +18 -0
@@ -66,6 +66,7 @@ MODULE_DESCRIPTION("CAN driver for Kvaser CAN/PCIe devices");#define KVASER_PCIEFD_KCAN_FIFO_LAST_REG 0x180#define KVASER_PCIEFD_KCAN_CTRL_REG 0x2c0#define KVASER_PCIEFD_KCAN_CMD_REG 0x400 +#define KVASER_PCIEFD_KCAN_IOC_REG 0x404#define KVASER_PCIEFD_KCAN_IEN_REG 0x408#define KVASER_PCIEFD_KCAN_IRQ_REG 0x410#define KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG 0x414
@@ -136,6 +137,9 @@ MODULE_DESCRIPTION("CAN driver for Kvaser CAN/PCIe devices");/* Request status packet */ #define KVASER_PCIEFD_KCAN_CMD_SRQ BIT(0) +/* Control CAN LED, active low */ +#define KVASER_PCIEFD_KCAN_IOC_LED BIT(0) +/* Transmitter unaligned */ #define KVASER_PCIEFD_KCAN_IRQ_TAL BIT(17)/* Tx FIFO empty */
@@ -410,6 +414,7 @@ struct kvaser_pciefd_can {struct kvaser_pciefd *kv_pcie;void __iomem *reg_base;struct can_berr_counter bec; +u32 ioc;u8 cmd_seq;u8 tx_max_count;u8 tx_idx;
@@ -528,6 +533,16 @@ static inline void kvaser_pciefd_abort_flush_reset(struct kvaser_pciefd_can *cankvaser_pciefd_send_kcan_cmd (can,KVASER_PCIEFD_KCAN_CMD_AT ); } +static inline void kvaser_pciefd_set_led (struct kvaser_pciefd_can *can,bool on) +{ +if (on) + can->ioc &= ~KVASER_PCIEFD_KCAN_IOC_LED ; +else + can->ioc |=KVASER_PCIEFD_KCAN_IOC_LED ; + +iowrite32 (can->ioc, can->reg_base +KVASER_PCIEFD_KCAN_IOC_REG ); +} +static void kvaser_pciefd_enable_err_gen (struct kvaser_pciefd_can *can) {u32 mode;
@@ -990,6 +1005,9 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)/* Disable Bus load reporting */ iowrite32 (0 , can->reg_base +KVASER_PCIEFD_KCAN_BUS_LOAD_REG ); + can->ioc =ioread32 (can->reg_base +KVASER_PCIEFD_KCAN_IOC_REG ); +kvaser_pciefd_set_led (can, false); + tx_nr_packets_max =FIELD_GET (KVASER_PCIEFD_KCAN_TX_NR_PACKETS_MAX_MASK ,ioread32 (can->reg_base +KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG ));
Signature · 1 line
2.49.0
PATCH v4 2/10 can: kvaser_pciefd: Add support for ethtool set_phys_id()
In reply to Jimmy AssarssonAdd support for ethtool set_phys_id(), to physically locate devices by flashing a LED on the device.
---
Changes in v4:
- Add tag Reviewed-by Vincent Mailhol
Changes in v2:
- Return inside the switch-case. Suggested by Vincent Mailhol [1]
[1] https://lore.kernel.org/linux-can/[email protected]/T/#md10566c624e75c59ec735fed16d5ec4cbdb38430
drivers/net/can/kvaser_pciefd.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
drivers/net/can/kvaser_pciefd.c +24 -0
@@ -968,8 +968,32 @@ static const struct net_device_ops kvaser_pciefd_netdev_ops = { .ndo_change_mtu = can_change_mtu, }; +static int kvaser_pciefd_set_phys_id (struct net_device *netdev, +enum ethtool_phys_id_state state) +{ +struct kvaser_pciefd_can *can =netdev_priv (netdev); + +switch (state) { +case ETHTOOL_ID_ACTIVE : +return 3 ;/* 3 On/Off cycles per second */ + +case ETHTOOL_ID_ON : +kvaser_pciefd_set_led (can, true); +return 0 ; + +case ETHTOOL_ID_OFF : +case ETHTOOL_ID_INACTIVE : +kvaser_pciefd_set_led (can, false); +return 0 ; + +default : +return -EINVAL ; + } +} +static const struct ethtool_ops kvaser_pciefd_ethtool_ops = { .get_ts_info = can_ethtool_op_get_ts_info_hwts, + .set_phys_id = kvaser_pciefd_set_phys_id, };static int kvaser_pciefd_setup_can_ctrls (struct kvaser_pciefd *pcie)
Signature · 1 line
2.49.0
PATCH v4 3/10 can: kvaser_pciefd: Add intermediate variable for device struct in probe()
In reply to Jimmy AssarssonAdd intermediate variable, for readability and to simplify future patches.
---
Changes in v4:
- Add tag Reviewed-by Vincent Mailhol
drivers/net/can/kvaser_pciefd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/net/can/kvaser_pciefd.c +4 -3
@@ -1813,10 +1813,11 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, const struct pci_device_id *id) {int ret; +struct device *dev = &pdev->dev;struct kvaser_pciefd *pcie;const struct kvaser_pciefd_irq_mask *irq_mask; - pcie =devm_kzalloc (&pdev->dev,sizeof (*pcie),GFP_KERNEL ); + pcie =devm_kzalloc (dev,sizeof (*pcie),GFP_KERNEL );if (!pcie)return -ENOMEM ;
@@ -1855,7 +1856,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, ret =pci_alloc_irq_vectors (pcie->pci,1 ,1 ,PCI_IRQ_INTX |PCI_IRQ_MSI );if (ret <0 ) { -dev_err (&pcie->pci->dev,"Failed to allocate IRQ vectors.\n" ); +dev_err (dev,"Failed to allocate IRQ vectors.\n" ); goto err_teardown_can_ctrls; }
@@ -1868,7 +1869,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, ret =request_irq (pcie->pci->irq, kvaser_pciefd_irq_handler,IRQF_SHARED ,KVASER_PCIEFD_DRV_NAME , pcie);if (ret) { -dev_err (&pcie->pci->dev,"Failed to request IRQ %d\n" , pcie->pci->irq); +dev_err (dev,"Failed to request IRQ %d\n" , pcie->pci->irq); goto err_pci_free_irq_vectors; } iowrite32(KVASER_PCIEFD_SRB_IRQ_DPD0 | KVASER_PCIEFD_SRB_IRQ_DPD1,
Signature · 1 line
2.49.0
PATCH v4 4/10 can: kvaser_pciefd: Store the different firmware version components in a struct
In reply to Jimmy AssarssonStore firmware version in kvaser_pciefd_fw_version struct, specifying the different components of the version number.
And drop debug prinout of firmware version, since later patches will expose it via the devlink interface.
---
Changes in v4:
- Add tag Reviewed-by Vincent Mailhol
Changes in v2:
- Drop debug prinout, since it will be exposed via devlink. Suggested by Vincent Mailhol [1]
[1] https://lore.kernel.org/linux-can/[email protected]/T/#m2003548deedfeafee5c57ee2e2f610d364220fae
drivers/net/can/kvaser_pciefd.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/net/can/kvaser_pciefd.c +11 -6
@@ -325,6 +325,12 @@ struct kvaser_pciefd_driver_data {const struct kvaser_pciefd_dev_ops *ops; }; +struct kvaser_pciefd_fw_version { +u8 major; +u8 minor; +u16 build; +}; +static const struct kvaser_pciefd_address_offset kvaser_pciefd_altera_address_offset = { .serdes =0x1000 , .pci_ien =0x50 ,
@@ -437,6 +443,7 @@ struct kvaser_pciefd {u32 bus_freq;u32 freq;u32 freq_to_ticks_div; +struct kvaser_pciefd_fw_version fw_version; };struct kvaser_pciefd_rx_packet {
@@ -1205,14 +1212,12 @@ static int kvaser_pciefd_setup_board(struct kvaser_pciefd *pcie)u32 version, srb_status, build; version =ioread32 (KVASER_PCIEFD_SYSID_ADDR (pcie) +KVASER_PCIEFD_SYSID_VERSION_REG ); + build =ioread32 (KVASER_PCIEFD_SYSID_ADDR (pcie) +KVASER_PCIEFD_SYSID_BUILD_REG ); pcie->nr_channels =min (KVASER_PCIEFD_MAX_CAN_CHANNELS ,FIELD_GET (KVASER_PCIEFD_SYSID_VERSION_NR_CHAN_MASK , version)); - - build =ioread32 (KVASER_PCIEFD_SYSID_ADDR (pcie) +KVASER_PCIEFD_SYSID_BUILD_REG ); -dev_dbg (&pcie->pci->dev,"Version %lu.%lu.%lu\n" , -FIELD_GET (KVASER_PCIEFD_SYSID_VERSION_MAJOR_MASK , version), -FIELD_GET (KVASER_PCIEFD_SYSID_VERSION_MINOR_MASK , version), -FIELD_GET (KVASER_PCIEFD_SYSID_BUILD_SEQ_MASK , build)); + pcie->fw_version.major =FIELD_GET (KVASER_PCIEFD_SYSID_VERSION_MAJOR_MASK , version); + pcie->fw_version.minor =FIELD_GET (KVASER_PCIEFD_SYSID_VERSION_MINOR_MASK , version); + pcie->fw_version.build =FIELD_GET (KVASER_PCIEFD_SYSID_BUILD_SEQ_MASK , build); srb_status =ioread32 (KVASER_PCIEFD_SRB_ADDR (pcie) +KVASER_PCIEFD_SRB_STAT_REG );if (!(srb_status &KVASER_PCIEFD_SRB_STAT_DMA )) {
Signature · 1 line
2.49.0
PATCH v4 5/10 can: kvaser_pciefd: Store device channel index
In reply to Jimmy AssarssonStore device channel index in netdev.dev_port.
---
Changes in v4:
- Add tag Reviewed-by Vincent Mailhol
Changes in v2:
- Add Fixes tag.
drivers/net/can/kvaser_pciefd.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/can/kvaser_pciefd.c +1 -0
@@ -1028,6 +1028,7 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) can->completed_tx_bytes =0 ; can->bec.txerr =0 ; can->bec.rxerr =0 ; + can->can.dev->dev_port = i;init_completion (&can->start_comp);init_completion (&can->flush_comp);
Signature · 1 line
2.49.0
PATCH v4 6/10 can: kvaser_pciefd: Split driver into C-file and header-file.
In reply to Jimmy AssarssonSplit driver into C-file and header-file, to simplify future patches. Move common definitions and declarations to a header file.
---
Changes in v4:
- Add tag Reviewed-by Vincent Mailhol
drivers/net/can/Makefile | 2 +-
drivers/net/can/kvaser_pciefd/Makefile | 3 +
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h | 90 +++++++++++++++++++
.../kvaser_pciefd_core.c} | 72 +--------------
4 files changed, 96 insertions(+), 71 deletions(-)
create mode 100644 drivers/net/can/kvaser_pciefd/Makefile
create mode 100644 drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
rename drivers/net/can/{kvaser_pciefd.c => kvaser_pciefd/kvaser_pciefd_core.c} (97%)drivers/net/can/Makefile +1 -1
@@ -25,7 +25,7 @@ obj-$(CONFIG_CAN_FLEXCAN) += flexcan/ obj-$(CONFIG_CAN_GRCAN) += grcan.o obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o -obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o +obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd/ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_M_CAN) += m_can/ obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/
drivers/net/can/kvaser_pciefd/Makefile new file +3 -0
@@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o +kvaser_pciefd-y = kvaser_pciefd_core.o
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h new file +90 -0
90 lines not on this page · raw message
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c renamed from drivers/net/can/kvaser_pciefd.c +2 -70
96 lines not on this page · raw message
Signature · 1 line
2.49.0
PATCH v4 7/10 can: kvaser_pciefd: Add devlink support
In reply to Jimmy AssarssonAdd devlink support at device level.
Example output:
$ devlink dev pci/0000:07:00.0 pci/0000:08:00.0 pci/0000:09:00.0
$ devlink dev info
pci/0000:07:00.0:
driver kvaser_pciefd
pci/0000:08:00.0:
driver kvaser_pciefd
pci/0000:09:00.0:
driver kvaser_pciefd---
Changes in v4:
- Include kvaser_picefd.h to avoid transient Sparse warning reported by Simon Horman [2]
- Add tag Reviewed-by Vincent Mailhol
Changes in v2:
- Add two space indentation to terminal output.
Suggested by Vincent Mailhol [1]
[1] https://lore.kernel.org/linux-can/[email protected]/T/#m31ee4aad13ee29d5559b56fdce842609ae4f67c5
[2] https://lore.kernel.org/linux-can/[email protected]/T/#mbdd00e79c5765136b0a91cf38f0814a46c50a09b
drivers/net/can/Kconfig | 1 + drivers/net/can/kvaser_pciefd/Makefile | 2 +- drivers/net/can/kvaser_pciefd/kvaser_pciefd.h | 2 ++ .../net/can/kvaser_pciefd/kvaser_pciefd_core.c | 15 ++++++++++++--- .../net/can/kvaser_pciefd/kvaser_pciefd_devlink.c | 11 +++++++++++ 5 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
drivers/net/can/Kconfig +1 -0
@@ -154,6 +154,7 @@ config CAN_JANZ_ICAN3 config CAN_KVASER_PCIEFD depends on PCI tristate "Kvaser PCIe FD cards" + select NET_DEVLINK help This is a driver for the Kvaser PCI Express CAN FD family.
drivers/net/can/kvaser_pciefd/Makefile +1 -1
@@ -1,3 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o -kvaser_pciefd-y = kvaser_pciefd_core.o +kvaser_pciefd-y = kvaser_pciefd_core.o kvaser_pciefd_devlink.o
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +2 -0
@@ -13,6 +13,7 @@#include <linux/spinlock.h> #include <linux/timer.h> #include <linux/types.h> +#include <net/devlink.h> #define KVASER_PCIEFD_MAX_CAN_CHANNELS 8UL#define KVASER_PCIEFD_DMA_COUNT 2U
@@ -87,4 +88,5 @@ struct kvaser_pciefd { struct kvaser_pciefd_fw_version fw_version; }; +extern const struct devlink_ops kvaser_pciefd_devlink_ops;#endif /* _KVASER_PCIEFD_H */
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +12 -3
@@ -1751,14 +1751,16 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, const struct pci_device_id *id) {int ret; +struct devlink *devlink;struct device *dev = &pdev->dev;struct kvaser_pciefd *pcie;const struct kvaser_pciefd_irq_mask *irq_mask; - pcie =devm_kzalloc (dev,sizeof (*pcie),GFP_KERNEL ); -if (!pcie) + devlink =devlink_alloc (&kvaser_pciefd_devlink_ops,sizeof (*pcie), dev); +if (!devlink)return -ENOMEM ; + pcie =devlink_priv (devlink);pci_set_drvdata (pdev, pcie); pcie->pci = pdev; pcie->driver_data = (const struct kvaser_pciefd_driver_data *)id->driver_data;
@@ -1766,7 +1768,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, ret =pci_enable_device (pdev);if (ret) -return ret; + goto err_free_devlink; ret =pci_request_regions (pdev,KVASER_PCIEFD_DRV_NAME );if (ret)
@@ -1830,6 +1832,8 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,if (ret) goto err_free_irq; +devlink_register (devlink); +return 0 ; err_free_irq:
@@ -1853,6 +1857,9 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, err_disable_pci:pci_disable_device (pdev); +err_free_devlink: +devlink_free (devlink); +return ret; }
@@ -1876,6 +1883,8 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)for (i =0 ; i < pcie->nr_channels; ++i)free_candev (pcie->can[i]->can.dev); +devlink_unregister (priv_to_devlink (pcie)); +devlink_free (priv_to_devlink (pcie));pci_iounmap (pdev, pcie->reg_base);pci_release_regions (pdev);pci_disable_device (pdev);
drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c new file +11 -0
@@ -0,0 +1,11 @@ +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +/* kvaser_pciefd devlink functions +* +* Copyright (C) 2025 KVASER AB, Sweden. All rights reserved. +*/ +#include "kvaser_pciefd.h" + +#include <net/devlink.h> + +const struct devlink_ops kvaser_pciefd_devlink_ops = { +};
Signature · 1 line
2.49.0
Marc Kleine-Budde
maintainerRe: [PATCH v4 07/10] can: kvaser_pciefd: Add devlink support
Quoted 50 lines · Jimmy Assarsson
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +10
-3@@ -1751,14 +1751,16 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, const struct pci_device_id *id) {int ret; +struct devlink *devlink;struct device *dev = &pdev->dev;struct kvaser_pciefd *pcie;const struct kvaser_pciefd_irq_mask *irq_mask; - pcie =devm_kzalloc (dev,sizeof (*pcie),GFP_KERNEL ); -if (!pcie) + devlink =devlink_alloc (&kvaser_pciefd_devlink_ops,sizeof (*pcie), dev); +if (!devlink)return -ENOMEM ; + pcie =devlink_priv (devlink);pci_set_drvdata (pdev, pcie); pcie->pci = pdev; pcie->driver_data = (const struct kvaser_pciefd_driver_data *)id->driver_data;@@ -1766,7 +1768,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, ret =pci_enable_device (pdev);if (ret) -return ret; + goto err_free_devlink; ret =pci_request_regions (pdev,KVASER_PCIEFD_DRV_NAME );if (ret)@@ -1830,6 +1832,8 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,if (ret) goto err_free_irq; +devlink_register (devlink); +return 0 ; err_free_irq:@@ -1853,6 +1857,9 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, err_disable_pci:pci_disable_device (pdev); +err_free_devlink: +devlink_free (devlink); +return ret; }
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c static void kvaser_pciefd_remove(struct pci_dev *pdev) · line 1888
Quoted 6 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +2
-0@@ -1876,6 +1883,8 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)for (i =0 ; i < pcie->nr_channels; ++i)free_candev (pcie->can[i]->can.dev); +devlink_unregister (priv_to_devlink (pcie)); +devlink_free (priv_to_devlink (pcie));pci_iounmap (pdev, pcie->reg_base);
^^^^
This smells like a use after free. Please call the cleanup function in reverse order of allocation functions, i.e. move devlink_free() to the end of this function.
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c static void kvaser_pciefd_remove(struct pci_dev *pdev) · line 1890
Quoted 2 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +0
-0pci_release_regions (pdev);pci_disable_device (pdev);
regards,
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Re: [PATCH v4 07/10] can: kvaser_pciefd: Add devlink support
Quoted 63 lines · Marc Kleine-Budde
Quoted 50 lines · Jimmy Assarsson
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +10
-3@@ -1751,14 +1751,16 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, const struct pci_device_id *id) {int ret; +struct devlink *devlink;struct device *dev = &pdev->dev;struct kvaser_pciefd *pcie;const struct kvaser_pciefd_irq_mask *irq_mask; - pcie =devm_kzalloc (dev,sizeof (*pcie),GFP_KERNEL ); -if (!pcie) + devlink =devlink_alloc (&kvaser_pciefd_devlink_ops,sizeof (*pcie), dev); +if (!devlink)return -ENOMEM ; + pcie =devlink_priv (devlink);pci_set_drvdata (pdev, pcie); pcie->pci = pdev; pcie->driver_data = (const struct kvaser_pciefd_driver_data *)id->driver_data;@@ -1766,7 +1768,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, ret =pci_enable_device (pdev);if (ret) -return ret; + goto err_free_devlink; ret =pci_request_regions (pdev,KVASER_PCIEFD_DRV_NAME );if (ret)@@ -1830,6 +1832,8 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,if (ret) goto err_free_irq; +devlink_register (devlink); +return 0 ; err_free_irq:@@ -1853,6 +1857,9 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev, err_disable_pci:pci_disable_device (pdev); +err_free_devlink: +devlink_free (devlink); +return ret; }drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c static void kvaser_pciefd_remove(struct pci_dev *pdev) · line 1888
Quoted 6 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +2
-0@@ -1876,6 +1883,8 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)for (i =0 ; i < pcie->nr_channels; ++i)free_candev (pcie->can[i]->can.dev); +devlink_unregister (priv_to_devlink (pcie)); +devlink_free (priv_to_devlink (pcie));pci_iounmap (pdev, pcie->reg_base);^^^^This smells like a use after free. Please call the cleanup function in reverse order of allocation functions, i.e. move
devlink_free()to the end of this function.
Quoted 5 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c static void kvaser_pciefd_remove(struct pci_dev *pdev) · line 1890
Quoted 2 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +0
-0pci_release_regions (pdev);pci_disable_device (pdev);regards,
Marc
I agree. Thanks for finding this!
I've tested moving devlink_free() to the end of the function, without any issues.
If you don't find any other problems, do you mind making this change before sending the PR? Otherwise I need to wait for the netdev 24h grace period, as Paolo pointed out [1], before sending v5.
[1] https://lore.kernel.org/linux-can/[email protected]/T/#m174402d37840f225b2799fbc53d7658ccc27be72
Best regards,
jimmy
Marc Kleine-Budde
maintainerRe: [PATCH v4 07/10] can: kvaser_pciefd: Add devlink support
Quoted 12 lines · Jimmy Assarsson
Quoted 12 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c static void kvaser_pciefd_remove(struct pci_dev *pdev) · line 1887
Quoted 6 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +2
-0@@ -1876,6 +1883,8 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)for (i =0 ; i < pcie->nr_channels; ++i)free_candev (pcie->can[i]->can.dev); +devlink_unregister (priv_to_devlink (pcie)); +devlink_free (priv_to_devlink (pcie));pci_iounmap (pdev, pcie->reg_base);^^^^This smells like a use after free. Please call the cleanup function in reverse order of allocation functions, i.e. move
devlink_free()to the end of this function.
Quoted 6 lines
Quoted 2 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c static void kvaser_pciefd_remove(struct pci_dev *pdev) · line 1890
Quoted 2 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +0
-0pci_release_regions (pdev);pci_disable_device (pdev);I agree. Thanks for finding this!
I've tested movingdevlink_free()to the end of the function, without any issues.
Thanks.
Quoted 3 lines
If you don't find any other problems, do you mind making this change before sending the PR? Otherwise I need to wait for the netdev 24h grace period, as Paolo pointed out [1], before sending v5.
No problem. I've fixed the patch while applying the whole series to can-next and including it in my PR. Thanks for your big contribution!
regards,
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
PATCH v4 8/10 can: kvaser_pciefd: Expose device firmware version via devlink info_get()
In reply to Jimmy AssarssonExpose device firmware version via devlink info_get().
Example output:
$ devlink dev pci/0000:07:00.0 pci/0000:08:00.0 pci/0000:09:00.0
$ devlink dev info
pci/0000:07:00.0:
driver kvaser_pciefd
versions:
running:
fw 1.3.75
pci/0000:08:00.0:
driver kvaser_pciefd
versions:
running:
fw 2.4.29
pci/0000:09:00.0:
driver kvaser_pciefd
versions:
running:
fw 1.3.72---
Changes in v4:
- Move include of kvaser_picefd.h to previous patch to avoid transient Sparse warning reported by Simon Horman [3]
- Add tag Reviewed-by Vincent Mailhol
Changes in v2:
- Add two space indentation to terminal output.
Suggested by Vincent Mailhol [1]
- Replaced fixed-size char array with a string literal to let the compiler determine the buffer size automatically. Suggested by Vincent Mailhol [2]
[1] https://lore.kernel.org/linux-can/[email protected]/T/#m31ee4aad13ee29d5559b56fdce842609ae4f67c5
[2] https://lore.kernel.org/linux-can/[email protected]/T/#m97df78a8b0bafa6fe888f5fc0c27d0a05877bdaf
[3] https://lore.kernel.org/linux-can/[email protected]/T/#mbdd00e79c5765136b0a91cf38f0814a46c50a09b
.../can/kvaser_pciefd/kvaser_pciefd_devlink.c | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+)
drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c +24 -0
@@ -7,5 +7,29 @@#include <net/devlink.h> +static int kvaser_pciefd_devlink_info_get (struct devlink *devlink, +struct devlink_info_req *req, +struct netlink_ext_ack *extack) +{ +struct kvaser_pciefd *pcie =devlink_priv (devlink); +char buf[] ="xxx.xxx.xxxxx" ; +int ret; + +if (pcie->fw_version.major) { +snprintf (buf,sizeof (buf),"%u.%u.%u" , + pcie->fw_version.major, + pcie->fw_version.minor, + pcie->fw_version.build); + ret =devlink_info_version_running_put (req, +DEVLINK_INFO_VERSION_GENERIC_FW , + buf); +if (ret) +return ret; + } + +return 0 ; +} +const struct devlink_ops kvaser_pciefd_devlink_ops = { + .info_get = kvaser_pciefd_devlink_info_get, };
Signature · 1 line
2.49.0
PATCH v4 9/10 can: kvaser_pciefd: Add devlink port support
In reply to Jimmy AssarssonRegister each CAN channel of the device as an devlink physical port. This makes it easier to get device information for a given network interface (i.e. can2).
Example output:
$ devlink dev pci/0000:07:00.0 pci/0000:08:00.0 pci/0000:09:00.0
$ devlink port pci/0000:07:00.0/0: type eth netdev can0 flavour physical port 0 splittable false pci/0000:07:00.0/1: type eth netdev can1 flavour physical port 1 splittable false pci/0000:07:00.0/2: type eth netdev can2 flavour physical port 2 splittable false pci/0000:07:00.0/3: type eth netdev can3 flavour physical port 3 splittable false pci/0000:08:00.0/0: type eth netdev can4 flavour physical port 0 splittable false pci/0000:08:00.0/1: type eth netdev can5 flavour physical port 1 splittable false pci/0000:09:00.0/0: type eth netdev can6 flavour physical port 0 splittable false pci/0000:09:00.0/1: type eth netdev can7 flavour physical port 1 splittable false pci/0000:09:00.0/2: type eth netdev can8 flavour physical port 2 splittable false pci/0000:09:00.0/3: type eth netdev can9 flavour physical port 3 splittable false
$ devlink port show can2 pci/0000:07:00.0/2: type eth netdev can2 flavour physical port 2 splittable false
$ devlink dev info
pci/0000:07:00.0:
driver kvaser_pciefd
versions:
running:
fw 1.3.75
pci/0000:08:00.0:
driver kvaser_pciefd
versions:
running:
fw 2.4.29
pci/0000:09:00.0:
driver kvaser_pciefd
versions:
running:
fw 1.3.72$ sudo ethtool -i can2 driver: kvaser_pciefd version: 6.8.0-40-generic firmware-version: 1.3.75 expansion-rom-version: bus-info: 0000:07:00.0 supports-statistics: no supports-test: no supports-eeprom-access: no supports-register-dump: no supports-priv-flags: no
---
Changes in v4:
- Add tag Reviewed-by Vincent Mailhol
Changes in v2:
- Add two space indentation to terminal output. Suggested by Vincent Mailhol [1]
- Replace netdev.dev_id with netdev.dev_port, to reflect changes in previous patch.
[1] https://lore.kernel.org/linux-can/[email protected]/T/#m31ee4aad13ee29d5559b56fdce842609ae4f67c5
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h | 4 +++ .../can/kvaser_pciefd/kvaser_pciefd_core.c | 8 ++++++ .../can/kvaser_pciefd/kvaser_pciefd_devlink.c | 25 +++++++++++++++++++ 3 files changed, 37 insertions(+)
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +4 -0
@@ -59,6 +59,7 @@ struct kvaser_pciefd_fw_version {struct kvaser_pciefd_can {struct can_priv can; +struct devlink_port devlink_port;struct kvaser_pciefd *kv_pcie;void __iomem *reg_base;struct can_berr_counter bec;
@@ -89,4 +90,7 @@ struct kvaser_pciefd { };extern const struct devlink_ops kvaser_pciefd_devlink_ops; + +int kvaser_pciefd_devlink_port_register (struct kvaser_pciefd_can *can); +void kvaser_pciefd_devlink_port_unregister (struct kvaser_pciefd_can *can);#endif /* _KVASER_PCIEFD_H */
drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +8 -0
@@ -943,6 +943,7 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)struct net_device *netdev;struct kvaser_pciefd_can *can;u32 status, tx_nr_packets_max; +int ret; netdev =alloc_candev (sizeof (struct kvaser_pciefd_can ),roundup_pow_of_two (KVASER_PCIEFD_CAN_TX_MAX_COUNT ));
@@ -1013,6 +1014,11 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) pcie->can[i] = can;kvaser_pciefd_pwm_start (can); + ret =kvaser_pciefd_devlink_port_register (can); +if (ret) { +dev_err (&pcie->pci->dev,"Failed to register devlink port\n" ); +return ret; + } }return 0 ;
@@ -1732,6 +1738,7 @@ static void kvaser_pciefd_teardown_can_ctrls(struct kvaser_pciefd *pcie)if (can) {iowrite32 (0 , can->reg_base +KVASER_PCIEFD_KCAN_IEN_REG );kvaser_pciefd_pwm_stop (can); +kvaser_pciefd_devlink_port_unregister (can);free_candev (can->can.dev); } }
@@ -1874,6 +1881,7 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)unregister_candev (can->can.dev);timer_delete (&can->bec_poll_timer);kvaser_pciefd_pwm_stop (can); +kvaser_pciefd_devlink_port_unregister (can); }kvaser_pciefd_disable_irq_srcs (pcie);
drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c +25 -0
@@ -5,6 +5,7 @@*/ #include "kvaser_pciefd.h" +#include <linux/netdevice.h> #include <net/devlink.h> static int kvaser_pciefd_devlink_info_get(struct devlink *devlink,
@@ -33,3 +34,27 @@ static int kvaser_pciefd_devlink_info_get(struct devlink *devlink,const struct devlink_ops kvaser_pciefd_devlink_ops = { .info_get = kvaser_pciefd_devlink_info_get, }; + +int kvaser_pciefd_devlink_port_register (struct kvaser_pciefd_can *can) +{ +int ret; +struct devlink_port_attrs attrs = { + .flavour =DEVLINK_PORT_FLAVOUR_PHYSICAL , + .phys.port_number = can->can.dev->dev_port, + }; +devlink_port_attrs_set (&can->devlink_port, &attrs); + + ret =devlink_port_register (priv_to_devlink (can->kv_pcie), + &can->devlink_port, can->can.dev->dev_port); +if (ret) +return ret; + +SET_NETDEV_DEVLINK_PORT (can->can.dev, &can->devlink_port); + +return 0 ; +} + +void kvaser_pciefd_devlink_port_unregister (struct kvaser_pciefd_can *can) +{ +devlink_port_unregister (&can->devlink_port); +}
Signature · 1 line
2.49.0
PATCH v4 10/10 Documentation: devlink: add devlink documentation for the kvaser_pciefd driver
In reply to Jimmy AssarssonList the version information reported by the kvaser_pciefd driver through devlink.
---
Changes in v4:
- Add tags {Reviewed,Suggested}-by Vincent Mailhol
Changes in v2:
- New in v2. Suggested by Vincent Mailhol [1]
[1] https://lore.kernel.org/linux-can/[email protected]/T/#mb9ede2edcf5f7adcb76bc6331f5f27bafb79294f
Documentation/networking/devlink/index.rst | 1 + .../networking/devlink/kvaser_pciefd.rst | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 Documentation/networking/devlink/kvaser_pciefd.rst
Documentation/networking/devlink/index.rst +1 -0
@@ -85,6 +85,7 @@ parameters, info versions, and other features it supports. ionic ice ixgbe + kvaser_pciefd mlx4 mlx5 mlxsw
Documentation/networking/devlink/kvaser_pciefd.rst new file +24 -0
@@ -0,0 +1,24 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================= +kvaser_pciefd devlink support +============================= + +This document describes the devlink features implemented by the +``kvaser_pciefd`` device driver. + +Info versions +============= + +The ``kvaser_pciefd`` driver reports the following versions + +.. list-table:: devlink info versions implemented + :widths: 5 5 90 + + * - Name + - Type + - Description + * - ``fw`` + - running + - Version of the firmware running on the device. Also available + through ``ethtool -i`` as ``firmware-version``.
Signature · 1 line
2.49.0