can: kvaser_pciefd: Simplify identification of physical CAN interfaces

PATCH In review Jimmy Assarsson 4 participants 22 messages Jul 23, 2025 – Jul 24, 2025 CAN network drivers

Share & tools

Jimmy Assarsson

author

2025-07-23 08:33 UTC #0 lore raw

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.

Jimmy Assarsson (9):
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

 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}                     | 150 +++++++++---------
 .../can/kvaser_pciefd/kvaser_pciefd_devlink.c |  61 +++++++
 6 files changed, 239 insertions(+), 74 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} (96%)
 create mode 100644 drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c

Signature · 1 line

2.49.0

Jimmy Assarsson

author

2025-07-23 08:33 UTC #1 lore raw

PATCH 1/9 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.

Reviewed-by Axel Forsman <[email protected]>

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/net/can/kvaser_pciefd.c +18 -0

index 09510663988c..c8f530ef416e 100644
@@ -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 *can
 	kvaser_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

Vincent Mailhol

2025-07-23 12:09 UTC #2 lore raw

Re: [PATCH 1/9] can: kvaser_pciefd: Add support to control CAN LEDs on device

Quoted 58 lines · Jimmy Assarsson

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.

Reviewed-by Axel Forsman <[email protected]>

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/net/can/kvaser_pciefd.c +15 -0

index 09510663988c..c8f530ef416e 100644
@@ -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 *can
 	kvaser_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;

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1008

Quoted 4 lines

drivers/net/can/kvaser_pciefd.c +1 -0

@@ -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);

Nitpick: shouldn't this line go into kvaser_pciefd_set_led() ?

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1013

Quoted 5 lines

drivers/net/can/kvaser_pciefd.c +2 -0

+		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));

Yours sincerely,
Vincent Mailhol

Jimmy Assarsson

2025-07-24 06:48 UTC #3 lore raw

Re: [PATCH 1/9] can: kvaser_pciefd: Add support to control CAN LEDs on device

Quoted 62 lines · Vincent Mailhol

Quoted 61 lines · Jimmy Assarsson

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.

Reviewed-by Axel Forsman <[email protected]>

Signed-off-by Jimmy Assarsson <[email protected]>

---
  drivers/net/can/kvaser_pciefd.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

drivers/net/can/kvaser_pciefd.c +15 -0

index 09510663988c..c8f530ef416e 100644
@@ -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 *can
  	kvaser_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);

Quoted 3 lines

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1008
Quoted 1 line
drivers/net/can/kvaser_pciefd.c +1 -0
+		can->ioc = ioread32(can->reg_base + KVASER_PCIEFD_KCAN_IOC_REG);
Nitpick: shouldn't this line go into kvaser_pciefd_set_led() ?

If we read the register here, we only need to read KVASER_PCIEFD_KCAN_IOC_REG once, and let the driver maintain the state.
So I prefer to keep it.

Best regards,
jimmy

Quoted 8 lines

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1013
Quoted 5 lines
drivers/net/can/kvaser_pciefd.c +2 -0
+		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));
Yours sincerely,
Vincent Mailhol

Jimmy Assarsson

author

2025-07-23 08:33 UTC #4 lore raw

PATCH 2/9 can: kvaser_pciefd: Add support for ethtool set_phys_id()

In reply to Jimmy Assarsson

Add support for ethtool set_phys_id(), to physically locate devices by flashing a LED on the device.

Reviewed-by Axel Forsman <[email protected]>

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

drivers/net/can/kvaser_pciefd.c +26 -0

index c8f530ef416e..c0bfeafb31f5 100644
@@ -968,8 +968,34 @@ 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);
+	int ret = 0;
+
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		ret = 3; /* 3 On/Off cycles per second */
+		break;
+	case ETHTOOL_ID_ON:
+		kvaser_pciefd_set_led(can, true);
+		break;
+	case ETHTOOL_ID_OFF:
+	case ETHTOOL_ID_INACTIVE:
+		kvaser_pciefd_set_led(can, false);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
 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

Vincent Mailhol

2025-07-23 12:14 UTC #5 lore raw

Re: [PATCH 2/9] can: kvaser_pciefd: Add support for ethtool set_phys_id()

Quoted 31 lines · Jimmy Assarsson

Add support for ethtool set_phys_id(), to physically locate devices by flashing a LED on the device.

Reviewed-by Axel Forsman <[email protected]>

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

drivers/net/can/kvaser_pciefd.c +14 -0

index c8f530ef416e..c0bfeafb31f5 100644
@@ -968,8 +968,34 @@ 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);
+	int ret = 0;
+
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		ret = 3; /* 3 On/Off cycles per second */
+		break;
+	case ETHTOOL_ID_ON:
+		kvaser_pciefd_set_led(can, true);
+		break;
+	case ETHTOOL_ID_OFF:

drivers/net/can/kvaser_pciefd.c static const struct net_device_ops kvaser_pciefd_netdev_ops = { · line 994

Quoted 10 lines

drivers/net/can/kvaser_pciefd.c +10 -0

+	case ETHTOOL_ID_INACTIVE:
+		kvaser_pciefd_set_led(can, false);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}

Nitpick, what about this?

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

Yours sincerely,
Vincent Mailhol

Jimmy Assarsson

author

2025-07-23 08:33 UTC #6 lore raw

PATCH 3/9 can: kvaser_pciefd: Add intermediate variable for device struct in probe()

In reply to Jimmy Assarsson

Add intermediate variable, for readability and to simplify future patches.

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/net/can/kvaser_pciefd.c +4 -3

index c0bfeafb31f5..f2722473b865 100644
@@ -1815,10 +1815,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;

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

@@ -1870,7 +1871,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

Jimmy Assarsson

author

2025-07-23 08:33 UTC #7 lore raw

PATCH 4/9 can: kvaser_pciefd: Store the different firmware version components in a struct

In reply to Jimmy Assarsson

Store firmware version in kvaser_pciefd_fw_version struct, specifying the different components of the version number.

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

drivers/net/can/kvaser_pciefd.c +15 -6

index f2722473b865..eba19819ca43 100644
@@ -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 {

@@ -1207,14 +1214,16 @@ 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);
+	dev_dbg(&pcie->pci->dev, "Version %u.%u.%u\n",
+		pcie->fw_version.major,
+		pcie->fw_version.minor,
+		pcie->fw_version.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

Vincent Mailhol

2025-07-23 12:17 UTC #8 lore raw

Re: [PATCH 4/9] can: kvaser_pciefd: Store the different firmware version components in a struct

Quoted 43 lines · Jimmy Assarsson

Store firmware version in kvaser_pciefd_fw_version struct, specifying the different components of the version number.

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

drivers/net/can/kvaser_pciefd.c +8 -3

index f2722473b865..eba19819ca43 100644
@@ -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 {

@@ -1207,14 +1214,16 @@ 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",

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_board(struct kvaser_pciefd *pcie) · line 1226

Quoted 10 lines

drivers/net/can/kvaser_pciefd.c +7 -3

-		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);
+	dev_dbg(&pcie->pci->dev, "Version %u.%u.%u\n",
+		pcie->fw_version.major,
+		pcie->fw_version.minor,
+		pcie->fw_version.build);

At the end of this series, the firmware version will be available through the devlink interface, so this debug message loses it purpose. If you do not mind, better to remove that dev_dbg().

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_board(struct kvaser_pciefd *pcie) · line 1229

Quoted 2 lines

drivers/net/can/kvaser_pciefd.c +0 -0

 	srb_status = ioread32(KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_STAT_REG);
 	if (!(srb_status & KVASER_PCIEFD_SRB_STAT_DMA)) {

Yours sincerely,
Vincent Mailhol

Jimmy Assarsson

author

2025-07-23 08:33 UTC #9 lore raw

PATCH 5/9 can: kvaser_pciefd: Store device channel index

In reply to Jimmy Assarsson

Store device channel index in netdev.dev_id.

Signed-off-by Jimmy Assarsson <[email protected]>

---
 drivers/net/can/kvaser_pciefd.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/can/kvaser_pciefd.c +1 -0

index eba19819ca43..7feece6d1d41 100644
@@ -1030,6 +1030,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_id = i;
 
 		init_completion(&can->start_comp);
 		init_completion(&can->flush_comp);

Signature · 1 line

2.49.0

Vincent Mailhol

2025-07-23 12:27 UTC #10 lore raw

Re: [PATCH 5/9] can: kvaser_pciefd: Store device channel index

Quoted 12 lines · Jimmy Assarsson

Store device channel index in netdev.dev_id.
Signed-off-by Jimmy Assarsson <[email protected]>
---
 drivers/net/can/kvaser_pciefd.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/can/kvaser_pciefd.c +0 -0
index eba19819ca43..7feece6d1d41 100644

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1033

Quoted 4 lines

drivers/net/can/kvaser_pciefd.c +1 -0

@@ -1030,6 +1030,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_id = i;

Isn't dev_port a better fit here?

See the description here:

https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-net

What: /sys/class/net/<iface>/dev_id
Date: April 2008
KernelVersion: 2.6.26
Contact: [email protected]
Description:
Indicates the device unique identifier. Format is an hexadecimal value. This is used to disambiguate interfaces which might be stacked (e.g: VLAN interfaces) but still have the same MAC address as their parent device.

What: /sys/class/net/<iface>/dev_port
Date: February 2014
KernelVersion: 3.15
Contact: [email protected]
Description:
Indicates the port number of this network device, formatted as a decimal value. Some NICs have multiple independent ports on the same PCI bus, device and function. This attribute allows userspace to distinguish the respective interfaces.

Note: some device drivers started to use 'dev_id' for this purpose since long before 3.15 and have not adopted the new attribute ever since. To query the port number, some tools look exclusively at 'dev_port', while others only consult 'dev_id'. If a network device has multiple client adapter ports as described in the previous paragraph and does not set this attribute to its port number, it's a kernel bug.

Also, not populating dev_port is a kernel bug, meaning that you should probably add a fix tag.

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1036

Quoted 2 lines

drivers/net/can/kvaser_pciefd.c +0 -0

 		init_completion(&can->start_comp);
 		init_completion(&can->flush_comp);

Yours sincerely,
Vincent Mailhol

Jimmy Assarsson

2025-07-24 07:05 UTC #11 lore raw

Re: [PATCH 5/9] can: kvaser_pciefd: Store device channel index

Quoted 44 lines · Vincent Mailhol

Quoted 12 lines · Jimmy Assarsson
Store device channel index in netdev.dev_id.
Signed-off-by Jimmy Assarsson <[email protected]>
---
  drivers/net/can/kvaser_pciefd.c | 1 +
  1 file changed, 1 insertion(+)
drivers/net/can/kvaser_pciefd.c +0 -0
index eba19819ca43..7feece6d1d41 100644
drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1033
Quoted 4 lines
drivers/net/can/kvaser_pciefd.c +1 -0
@@ -1030,6 +1030,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_id = i;
Isn't dev_port a better fit here?
See the description here:
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-net
What: /sys/class/net/<iface>/dev_id
Date: April 2008
KernelVersion: 2.6.26
Contact: [email protected]
Description:
Indicates the device unique identifier. Format is an hexadecimal value. This is used to disambiguate interfaces which might be stacked (e.g: VLAN interfaces) but still have the same MAC address as their parent device.
What: /sys/class/net/<iface>/dev_port
Date: February 2014
KernelVersion: 3.15
Contact: [email protected]
Description:
Indicates the port number of this network device, formatted as a decimal value. Some NICs have multiple independent ports on the same PCI bus, device and function. This attribute allows userspace to distinguish the respective interfaces.

Quoted 8 lines

Note: some device drivers started to use 'dev_id' for this purpose since long before 3.15 and have not adopted the new attribute ever since. To query the port number, some tools look exclusively at 'dev_port', while others only consult 'dev_id'. If a network device has multiple client adapter ports as described in the previous paragraph and does not set this attribute to its port number, it's a kernel bug.

Yes, dev_port is what we want. I looked at kvaser_usb, where dev_id is used.

Quoted 2 lines

Also, not populating dev_port is a kernel bug, meaning that you should probably add a fix tag.

Looks like there are more SocketCAN drivers using dev_id instead of dev_port:

  $ grep -r 'dev_id ='
  rockchip/rockchip_canfd-core.c:	dev_id = rkcanfd_read(priv, RKCANFD_REG_RTL_VERSION);
  peak_canfd/peak_canfd.c:	ndev->dev_id = index;
  softing/softing_main.c:		netdev->dev_id = j;
  usb/gs_usb.c:	netdev->dev_id = channel;
  usb/peak_usb/pcan_usb_core.c:	netdev->dev_id = ctrl_idx;
  usb/esd_usb.c:	netdev->dev_id = index;
  usb/kvaser_usb/kvaser_usb_core.c:	netdev->dev_id = channel;
  sja1000/f81601.c:		dev->dev_id = i;
  sja1000/ems_pcmcia.c:		dev->dev_id = i;
  sja1000/kvaser_pci.c:	dev->dev_id = channel;
  sja1000/plx_pci.c:			dev->dev_id = i;
  sja1000/peak_pcmcia.c:		netdev->dev_id = i;
  sja1000/sja1000_isa.c:	dev->dev_id = idx;
  sja1000/peak_pci.c:		dev->dev_id = i;
  sja1000/ems_pci.c:			dev->dev_id = i;
  spi/mcp251xfd/mcp251xfd-core.c:	*dev_id = get_unaligned_le32(buf_rx->data);

I'll also assign dev_port in kvaser_usb, but keep the assignment of dev_id to avoid potential regressions.

Or do you got other suggestions?

Best regards,
jimmy

Quoted 8 lines

drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1036
Quoted 3 lines
drivers/net/can/kvaser_pciefd.c +0 -0
  		init_completion(&can->start_comp);
  		init_completion(&can->flush_comp);
Yours sincerely,
Vincent Mailhol

Vincent Mailhol

2025-07-24 07:48 UTC #12 lore raw

Re: [PATCH 5/9] can: kvaser_pciefd: Store device channel index

Quoted 69 lines · Jimmy Assarsson

Quoted 44 lines · Vincent Mailhol
Quoted 12 lines · Jimmy Assarsson
Store device channel index in netdev.dev_id.
Signed-off-by Jimmy Assarsson <[email protected]>
---
  drivers/net/can/kvaser_pciefd.c | 1 +
  1 file changed, 1 insertion(+)
drivers/net/can/kvaser_pciefd.c +0 -0
index eba19819ca43..7feece6d1d41 100644
drivers/net/can/kvaser_pciefd.c static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) · line 1033
Quoted 4 lines
drivers/net/can/kvaser_pciefd.c +1 -0
@@ -1030,6 +1030,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_id = i;
Isn't dev_port a better fit here?
See the description here:
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-net
   What:              /sys/class/net/<iface>/dev_id
   Date:              April 2008
   KernelVersion:     2.6.26
Contact: [email protected]
Description:
Indicates the device unique identifier. Format is an hexadecimal value. This is used to disambiguate interfaces which might be stacked (e.g: VLAN interfaces) but still have the same MAC address as their parent device.
   What:              /sys/class/net/<iface>/dev_port
   Date:              February 2014
   KernelVersion:     3.15
Contact: [email protected]
Description:
Indicates the port number of this network device, formatted as a decimal value. Some NICs have multiple independent ports on the same PCI bus, device and function. This attribute allows userspace to distinguish the respective interfaces.
Quoted 8 lines
Note: some device drivers started to use 'dev_id' for this purpose since long before 3.15 and have not adopted the new attribute ever since. To query the port number, some tools look exclusively at 'dev_port', while others only consult 'dev_id'. If a network device has multiple client adapter ports as described in the previous paragraph and does not set this attribute to its port number, it's a kernel bug.
Yes, dev_port is what we want. I looked at kvaser_usb, where dev_id is used.
Quoted 2 lines
Also, not populating dev_port is a kernel bug, meaning that you should probably add a fix tag.
Looks like there are more SocketCAN drivers using dev_id instead of dev_port:
   $ grep -r 'dev_id ='
   rockchip/rockchip_canfd-core.c:      dev_id = rkcanfd_read(priv, RKCANFD_REG_RTL_VERSION);
   peak_canfd/peak_canfd.c:     ndev->dev_id = index;
   softing/softing_main.c:              netdev->dev_id = j;
   usb/gs_usb.c:        netdev->dev_id = channel;
   usb/peak_usb/pcan_usb_core.c:        netdev->dev_id = ctrl_idx;
   usb/esd_usb.c:       netdev->dev_id = index;
   usb/kvaser_usb/kvaser_usb_core.c:    netdev->dev_id = channel;
   sja1000/f81601.c:            dev->dev_id = i;

Quoted 8 lines

   sja1000/ems_pcmcia.c:                dev->dev_id = i;
   sja1000/kvaser_pci.c:        dev->dev_id = channel;
   sja1000/plx_pci.c:                   dev->dev_id = i;
   sja1000/peak_pcmcia.c:               netdev->dev_id = i;
   sja1000/sja1000_isa.c:       dev->dev_id = idx;
   sja1000/peak_pci.c:          dev->dev_id = i;
   sja1000/ems_pci.c:                   dev->dev_id = i;
   spi/mcp251xfd/mcp251xfd-core.c:      *dev_id = get_unaligned_le32(buf_rx->data);

Thanks for checking. I guess those will need a fix.

There are also some additional multi-channel devices which set none of dev_id nor dev_port which and which thus do not appear in your list.

Quoted 4 lines

I'll also assign dev_port in kvaser_usb, but keep the assignment of dev_id to avoid potential regressions.
Or do you got other suggestions?

No, I agree with your approach: for anything new, only use the dev_port. For anything old, keep the dev_id to avoid regression but add dev_port for compliance.

Yours sincerely,
Vincennt Mailhol

Jimmy Assarsson

author

2025-07-23 08:33 UTC #13 lore raw

PATCH 6/9 can: kvaser_pciefd: Split driver into C-file and header-file.

In reply to Jimmy Assarsson

Split driver into C-file and header-file, to simplify future patches. Move common definitions and declarations to a header file.

Signed-off-by Jimmy Assarsson <[email protected]>

---
 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

index a71db2cfe990..56138d8ddfd2 100644
@@ -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

new file mode 100644
index 000000000000..ea1bf1000760
@@ -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

Jimmy Assarsson

author

2025-07-23 08:33 UTC #14 lore raw

PATCH 7/9 can: kvaser_pciefd: Add devlink support

In reply to Jimmy Assarsson

Add 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

Signed-off-by Jimmy Assarsson <[email protected]>

---
 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 | 10 ++++++++++
 5 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c

drivers/net/can/Kconfig +1 -0

index cf989bea9aa3..b37d80bf7270 100644
@@ -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

index ea1bf1000760..8c5b8cdc6b5f 100644
@@ -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

index 55bb7e078340..34ba393d6093 100644
@@ -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

index 7bdcc84bce21..b553fc1fc3d7 100644
@@ -1757,14 +1757,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;

@@ -1772,7 +1774,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)

@@ -1836,6 +1838,8 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
 	if (ret)
 		goto err_free_irq;
 
+	devlink_register(devlink);
+
 	return 0;
 
 err_free_irq:

@@ -1859,6 +1863,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;
 }

@@ -1882,6 +1889,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 +10 -0

new file mode 100644
index 000000000000..8145d25943de
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+/* kvaser_pciefd devlink functions
+ *
+ * Copyright (C) 2025 KVASER AB, Sweden. All rights reserved.
+ */
+
+#include <net/devlink.h>
+
+const struct devlink_ops kvaser_pciefd_devlink_ops = {
+};

Signature · 1 line

2.49.0

Vincent Mailhol

2025-07-23 12:31 UTC #15 lore raw

Re: [PATCH 7/9] can: kvaser_pciefd: Add devlink support

Quoted 8 lines · Jimmy Assarsson

Add devlink support at device level.
Example output:
$ devlink dev
pci/0000:07:00.0
pci/0000:08:00.0
pci/0000:09:00.0

Quoted 7 lines

$ 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

Nitpick: can you add two space indentation before the quoted terminal output? Like this:

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

Quoted 44 lines

Signed-off-by Jimmy Assarsson <[email protected]>

---
 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 | 10 ++++++++++
 5 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c

drivers/net/can/Kconfig +1 -0

index cf989bea9aa3..b37d80bf7270 100644
@@ -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

index ea1bf1000760..8c5b8cdc6b5f 100644
@@ -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 +1 -0

index 55bb7e078340..34ba393d6093 100644
@@ -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

drivers/net/can/kvaser_pciefd/kvaser_pciefd.h struct kvaser_pciefd { · line 91

Quoted 4 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +1 -0

@@ -87,4 +88,5 @@ struct kvaser_pciefd {
 	struct kvaser_pciefd_fw_version fw_version;
 };
 
+extern const struct devlink_ops kvaser_pciefd_devlink_ops;

Nitpick: I would rather like to see a kvaser_pciefd_devlink.h instead of this.

Quoted 67 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +0 -0

 #endif /* _KVASER_PCIEFD_H */

drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c +12 -3

index 7bdcc84bce21..b553fc1fc3d7 100644
@@ -1757,14 +1757,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;

@@ -1772,7 +1774,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)

@@ -1836,6 +1838,8 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
 	if (ret)
 		goto err_free_irq;
 
+	devlink_register(devlink);
+
 	return 0;
 
 err_free_irq:

@@ -1859,6 +1863,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;
 }

@@ -1882,6 +1889,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 +0 -0

new file mode 100644
index 000000000000..8145d25943de

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c line 10

Quoted 10 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c new file +10 -0

@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+/* kvaser_pciefd devlink functions
+ *
+ * Copyright (C) 2025 KVASER AB, Sweden. All rights reserved.
+ */
+
+#include <net/devlink.h>
+
+const struct devlink_ops kvaser_pciefd_devlink_ops = {
+};

Yours sincerely,
Vincent Mailhol

Marc Kleine-Budde

maintainer

2025-07-23 12:36 UTC #16 lore raw

Re: [PATCH 7/9] can: kvaser_pciefd: Add devlink support

On 23.07.2025 21:31:31, Vincent Mailhol wrote:

[...]

Quoted 16 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +1 -0

index 55bb7e078340..34ba393d6093 100644
@@ -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;
 };

Quoted 3 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd.h struct kvaser_pciefd { · line 91
Quoted 1 line
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +1 -0
+extern const struct devlink_ops kvaser_pciefd_devlink_ops;
Nitpick: I would rather like to see a kvaser_pciefd_devlink.h instead of this.

IMHO 1 header file is enough.

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   |

Vincent Mailhol

2025-07-23 12:46 UTC #17 lore raw

Re: [PATCH 7/9] can: kvaser_pciefd: Add devlink support

Quoted 20 lines · Marc Kleine-Budde

On 23.07.2025 21:31:31, Vincent Mailhol wrote:

[...]

Quoted 16 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +1 -0

index 55bb7e078340..34ba393d6093 100644
@@ -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;
 };

Quoted 5 lines

Quoted 3 lines
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h struct kvaser_pciefd { · line 91
Quoted 1 line
drivers/net/can/kvaser_pciefd/kvaser_pciefd.h +1 -0
+extern const struct devlink_ops kvaser_pciefd_devlink_ops;
Nitpick: I would rather like to see a kvaser_pciefd_devlink.h instead of this.
IMHO 1 header file is enough.

Ah, I thought this was added to kvaser_pciefd.c, not kvaser_pciefd.h my bad.

Let me withdraw this comment then. It is fine as-is.

Yours sincerely,
Vincent Mailhol

Jimmy Assarsson

author

2025-07-23 08:33 UTC #18 lore raw

PATCH 8/9 can: kvaser_pciefd: Expose device firmware version via devlink info_get()

In reply to Jimmy Assarsson

Expose 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

Signed-off-by Jimmy Assarsson <[email protected]>

---
 .../can/kvaser_pciefd/kvaser_pciefd_devlink.c | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c +26 -0

index 8145d25943de..b6d3745089d4 100644
@@ -4,7 +4,33 @@
  * Copyright (C) 2025 KVASER AB, Sweden. All rights reserved.
  */
 
+#include "kvaser_pciefd.h"
+
 #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[14]; /* 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

Vincent Mailhol

2025-07-23 12:35 UTC #19 lore raw

Re: [PATCH 8/9] can: kvaser_pciefd: Expose device firmware version via devlink info_get()

Quoted 38 lines · Jimmy Assarsson

Expose 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

Signed-off-by Jimmy Assarsson <[email protected]>

---
 .../can/kvaser_pciefd/kvaser_pciefd_devlink.c | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c +0 -0

index 8145d25943de..b6d3745089d4 100644
@@ -4,7 +4,33 @@
  * Copyright (C) 2025 KVASER AB, Sweden. All rights reserved.
  */

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c line 16

Quoted 10 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c +8 -0

+#include "kvaser_pciefd.h"
+
 #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[14]; /* xxx.xxx.xxxxx */

Nitpick:

  char buf[] = "xxx.xxx.xxxxx";

This way, you do not have to count the characters :)

Quoted 10 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c +10 -0

+	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);

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c line 36

Quoted 10 lines

drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c +8 -0

+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 const struct devlink_ops kvaser_pciefd_devlink_ops = {
+	.info_get = kvaser_pciefd_devlink_info_get,
 };

Yours sincerely,
Vincent Mailhol

Jimmy Assarsson

author

2025-07-23 08:33 UTC #20 lore raw

PATCH 9/9 can: kvaser_pciefd: Add devlink port support

In reply to Jimmy Assarsson

Register 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

Signed-off-by Jimmy Assarsson <[email protected]>

---
 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

index 34ba393d6093..08c9ddc1ee85 100644
@@ -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

index b553fc1fc3d7..d99708a9f00c 100644
@@ -945,6 +945,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));

@@ -1015,6 +1016,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;

@@ -1738,6 +1744,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);
 		}
 	}

@@ -1880,6 +1887,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

index b6d3745089d4..194157e6b135 100644
@@ -6,6 +6,7 @@
 
 #include "kvaser_pciefd.h"
 
+#include <linux/netdevice.h>
 #include <net/devlink.h>
 
 static int kvaser_pciefd_devlink_info_get(struct devlink *devlink,

@@ -34,3 +35,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_id,
+	};
+	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_id);
+	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

Vincent Mailhol

2025-07-23 12:38 UTC #21 lore raw

In reply to Jimmy Assarsson

Quoted 3 lines · Jimmy Assarsson

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.

I completed my review.

My only real concern is on patch 5 where you are using the dev_id instead of the dev_port.

My other comments are all nitpicks. If you have a strong opinion, I am actually fine if you keep things as they are. No need to argue on these :)

Yours sincerely,
Vincent Mailhol