[PATCH 0/9] can: kvaser_pciefd: Simplify identification of physical CAN interfaces

[PATCH 0/9] can: kvaser_pciefd: Simplify identification of physical CAN interfaces

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

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

-- 
2.49.0

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

[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 <axfo@kvaser.com>
Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 09510663988c..c8f530ef416e 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -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));
-- 
2.49.0

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

[Vincent Mailhol]

On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> 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 <axfo@kvaser.com>
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index 09510663988c..c8f530ef416e 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -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);

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

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

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

[Jimmy Assarsson]

On 7/23/25 2:09 PM, Vincent Mailhol wrote:
> On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
>> 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 <axfo@kvaser.com>
>> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
>> ---
>>   drivers/net/can/kvaser_pciefd.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
>> index 09510663988c..c8f530ef416e 100644
>> --- a/drivers/net/can/kvaser_pciefd.c
>> +++ b/drivers/net/can/kvaser_pciefd.c
>> @@ -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);
> 
> 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 
>> +		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

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

[Jimmy Assarsson]

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

Reviewed-by: Axel Forsman <axfo@kvaser.com>
Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index c8f530ef416e..c0bfeafb31f5 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -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)
-- 
2.49.0

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

[Vincent Mailhol]

On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> Add support for ethtool set_phys_id(), to physically locate devices by
> flashing a LED on the device.
> 
> Reviewed-by: Axel Forsman <axfo@kvaser.com>
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index c8f530ef416e..c0bfeafb31f5 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -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;
> +}

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

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

[Jimmy Assarsson]

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

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index c0bfeafb31f5..f2722473b865 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -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,
-- 
2.49.0

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

[Jimmy Assarsson]

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

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index f2722473b865..eba19819ca43 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -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)) {
-- 
2.49.0

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

[Vincent Mailhol]

On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> Store firmware version in kvaser_pciefd_fw_version struct, specifying the
> different components of the version number.
> 
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index f2722473b865..eba19819ca43 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -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);

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().

>  	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

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

[Jimmy Assarsson]

Store device channel index in netdev.dev_id.

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index eba19819ca43..7feece6d1d41 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -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);
-- 
2.49.0

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

[Vincent Mailhol]

On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> Store device channel index in netdev.dev_id.
> 
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index eba19819ca43..7feece6d1d41 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -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:	netdev@vger.kernel.org
  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:	netdev@vger.kernel.org
  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.

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


Yours sincerely,
Vincent Mailhol

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

[Jimmy Assarsson]

On 7/23/25 2:27 PM, Vincent Mailhol wrote:
> On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
>> Store device channel index in netdev.dev_id.
>>
>> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
>> ---
>>   drivers/net/can/kvaser_pciefd.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
>> index eba19819ca43..7feece6d1d41 100644
>> --- a/drivers/net/can/kvaser_pciefd.c
>> +++ b/drivers/net/can/kvaser_pciefd.c
>> @@ -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:	netdev@vger.kernel.org
>    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:	netdev@vger.kernel.org
>    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.
> 

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

> 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

> 
>>   		init_completion(&can->start_comp);
>>   		init_completion(&can->flush_comp);
> 
> 
> Yours sincerely,
> Vincent Mailhol
> 

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

[Vincent Mailhol]

On Thu. 24 Jul. 2025 at 16:05, Jimmy Assarsson <jimmyassarsson@gmail.com> wrote:
> On 7/23/25 2:27 PM, Vincent Mailhol wrote:
> > On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> >> Store device channel index in netdev.dev_id.
> >>
> >> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> >> ---
> >>   drivers/net/can/kvaser_pciefd.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> >> index eba19819ca43..7feece6d1d41 100644
> >> --- a/drivers/net/can/kvaser_pciefd.c
> >> +++ b/drivers/net/can/kvaser_pciefd.c
> >> @@ -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:   netdev@vger.kernel.org
> >    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:   netdev@vger.kernel.org
> >    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.
> >
>
> Yes, dev_port is what we want. I looked at kvaser_usb, where dev_id is used.
>
> > 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);

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.

> 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

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

[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 <extja@kvaser.com>
---
 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%)

diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index a71db2cfe990..56138d8ddfd2 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -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/
diff --git a/drivers/net/can/kvaser_pciefd/Makefile b/drivers/net/can/kvaser_pciefd/Makefile
new file mode 100644
index 000000000000..ea1bf1000760
--- /dev/null
+++ b/drivers/net/can/kvaser_pciefd/Makefile
@@ -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
diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
new file mode 100644
index 000000000000..55bb7e078340
--- /dev/null
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
+/* kvaser_pciefd common definitions and declarations
+ *
+ * Copyright (C) 2025 KVASER AB, Sweden. All rights reserved.
+ */
+
+#ifndef _KVASER_PCIEFD_H
+#define _KVASER_PCIEFD_H
+
+#include <linux/can/dev.h>
+#include <linux/completion.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+#include <linux/types.h>
+
+#define KVASER_PCIEFD_MAX_CAN_CHANNELS 8UL
+#define KVASER_PCIEFD_DMA_COUNT 2U
+#define KVASER_PCIEFD_DMA_SIZE (4U * 1024U)
+#define KVASER_PCIEFD_CAN_TX_MAX_COUNT 17U
+
+struct kvaser_pciefd;
+
+struct kvaser_pciefd_address_offset {
+	u32 serdes;
+	u32 pci_ien;
+	u32 pci_irq;
+	u32 sysid;
+	u32 loopback;
+	u32 kcan_srb_fifo;
+	u32 kcan_srb;
+	u32 kcan_ch0;
+	u32 kcan_ch1;
+};
+
+struct kvaser_pciefd_irq_mask {
+	u32 kcan_rx0;
+	u32 kcan_tx[KVASER_PCIEFD_MAX_CAN_CHANNELS];
+	u32 all;
+};
+
+struct kvaser_pciefd_dev_ops {
+	void (*kvaser_pciefd_write_dma_map)(struct kvaser_pciefd *pcie,
+					    dma_addr_t addr, int index);
+};
+
+struct kvaser_pciefd_driver_data {
+	const struct kvaser_pciefd_address_offset *address_offset;
+	const struct kvaser_pciefd_irq_mask *irq_mask;
+	const struct kvaser_pciefd_dev_ops *ops;
+};
+
+struct kvaser_pciefd_fw_version {
+	u8 major;
+	u8 minor;
+	u16 build;
+};
+
+struct kvaser_pciefd_can {
+	struct can_priv 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;
+	u8 ack_idx;
+	int err_rep_cnt;
+	unsigned int completed_tx_pkts;
+	unsigned int completed_tx_bytes;
+	spinlock_t lock; /* Locks sensitive registers (e.g. MODE) */
+	struct timer_list bec_poll_timer;
+	struct completion start_comp, flush_comp;
+};
+
+struct kvaser_pciefd {
+	struct pci_dev *pci;
+	void __iomem *reg_base;
+	struct kvaser_pciefd_can *can[KVASER_PCIEFD_MAX_CAN_CHANNELS];
+	const struct kvaser_pciefd_driver_data *driver_data;
+	void *dma_data[KVASER_PCIEFD_DMA_COUNT];
+	u8 nr_channels;
+	u32 bus_freq;
+	u32 freq;
+	u32 freq_to_ticks_div;
+	struct kvaser_pciefd_fw_version fw_version;
+};
+
+#endif /* _KVASER_PCIEFD_H */
diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
similarity index 97%
rename from drivers/net/can/kvaser_pciefd.c
rename to drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
index 7feece6d1d41..7bdcc84bce21 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
@@ -5,6 +5,8 @@
  *  - PEAK linux canfd driver
  */
 
+#include "kvaser_pciefd.h"
+
 #include <linux/bitfield.h>
 #include <linux/can/dev.h>
 #include <linux/device.h>
@@ -27,10 +29,6 @@ MODULE_DESCRIPTION("CAN driver for Kvaser CAN/PCIe devices");
 #define KVASER_PCIEFD_WAIT_TIMEOUT msecs_to_jiffies(1000)
 #define KVASER_PCIEFD_BEC_POLL_FREQ (jiffies + msecs_to_jiffies(200))
 #define KVASER_PCIEFD_MAX_ERR_REP 256U
-#define KVASER_PCIEFD_CAN_TX_MAX_COUNT 17U
-#define KVASER_PCIEFD_MAX_CAN_CHANNELS 8UL
-#define KVASER_PCIEFD_DMA_COUNT 2U
-#define KVASER_PCIEFD_DMA_SIZE (4U * 1024U)
 
 #define KVASER_PCIEFD_VENDOR 0x1a07
 
@@ -296,41 +294,6 @@ static void kvaser_pciefd_write_dma_map_sf2(struct kvaser_pciefd *pcie,
 static void kvaser_pciefd_write_dma_map_xilinx(struct kvaser_pciefd *pcie,
 					       dma_addr_t addr, int index);
 
-struct kvaser_pciefd_address_offset {
-	u32 serdes;
-	u32 pci_ien;
-	u32 pci_irq;
-	u32 sysid;
-	u32 loopback;
-	u32 kcan_srb_fifo;
-	u32 kcan_srb;
-	u32 kcan_ch0;
-	u32 kcan_ch1;
-};
-
-struct kvaser_pciefd_dev_ops {
-	void (*kvaser_pciefd_write_dma_map)(struct kvaser_pciefd *pcie,
-					    dma_addr_t addr, int index);
-};
-
-struct kvaser_pciefd_irq_mask {
-	u32 kcan_rx0;
-	u32 kcan_tx[KVASER_PCIEFD_MAX_CAN_CHANNELS];
-	u32 all;
-};
-
-struct kvaser_pciefd_driver_data {
-	const struct kvaser_pciefd_address_offset *address_offset;
-	const struct kvaser_pciefd_irq_mask *irq_mask;
-	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,
@@ -415,37 +378,6 @@ static const struct kvaser_pciefd_driver_data kvaser_pciefd_xilinx_driver_data =
 	.ops = &kvaser_pciefd_xilinx_dev_ops,
 };
 
-struct kvaser_pciefd_can {
-	struct can_priv 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;
-	u8 ack_idx;
-	int err_rep_cnt;
-	unsigned int completed_tx_pkts;
-	unsigned int completed_tx_bytes;
-	spinlock_t lock; /* Locks sensitive registers (e.g. MODE) */
-	struct timer_list bec_poll_timer;
-	struct completion start_comp, flush_comp;
-};
-
-struct kvaser_pciefd {
-	struct pci_dev *pci;
-	void __iomem *reg_base;
-	struct kvaser_pciefd_can *can[KVASER_PCIEFD_MAX_CAN_CHANNELS];
-	const struct kvaser_pciefd_driver_data *driver_data;
-	void *dma_data[KVASER_PCIEFD_DMA_COUNT];
-	u8 nr_channels;
-	u32 bus_freq;
-	u32 freq;
-	u32 freq_to_ticks_div;
-	struct kvaser_pciefd_fw_version fw_version;
-};
-
 struct kvaser_pciefd_rx_packet {
 	u32 header[2];
 	u64 timestamp;
-- 
2.49.0

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

[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 <extja@kvaser.com>
---
 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

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index cf989bea9aa3..b37d80bf7270 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -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.
 
diff --git a/drivers/net/can/kvaser_pciefd/Makefile b/drivers/net/can/kvaser_pciefd/Makefile
index ea1bf1000760..8c5b8cdc6b5f 100644
--- a/drivers/net/can/kvaser_pciefd/Makefile
+++ b/drivers/net/can/kvaser_pciefd/Makefile
@@ -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
diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
index 55bb7e078340..34ba393d6093 100644
--- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
@@ -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 */
diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
index 7bdcc84bce21..b553fc1fc3d7 100644
--- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
@@ -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);
diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
new file mode 100644
index 000000000000..8145d25943de
--- /dev/null
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
@@ -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 = {
+};
-- 
2.49.0

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

[Vincent Mailhol]

On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> 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

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

> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  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
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index cf989bea9aa3..b37d80bf7270 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -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.
>  
> diff --git a/drivers/net/can/kvaser_pciefd/Makefile b/drivers/net/can/kvaser_pciefd/Makefile
> index ea1bf1000760..8c5b8cdc6b5f 100644
> --- a/drivers/net/can/kvaser_pciefd/Makefile
> +++ b/drivers/net/can/kvaser_pciefd/Makefile
> @@ -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
> diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
> index 55bb7e078340..34ba393d6093 100644
> --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
> @@ -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;

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

>  #endif /* _KVASER_PCIEFD_H */
> diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> index 7bdcc84bce21..b553fc1fc3d7 100644
> --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> @@ -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);
> diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
> new file mode 100644
> index 000000000000..8145d25943de
> --- /dev/null
> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
> @@ -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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

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

[...]

> > diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
> > index 55bb7e078340..34ba393d6093 100644
> > --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
> > +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
> > @@ -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;
> 
> Nitpick: I would rather like to see a kvaser_pciefd_devlink.h instead of this.

IMHO 1 header file is enough.

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vincent Mailhol]

On 23/07/2025 at 21:36, Marc Kleine-Budde wrote:
> On 23.07.2025 21:31:31, Vincent Mailhol wrote:
> 
> [...]
> 
>>> diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
>>> index 55bb7e078340..34ba393d6093 100644
>>> --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
>>> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
>>> @@ -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;
>>
>> 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

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

[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 <extja@kvaser.com>
---
 .../can/kvaser_pciefd/kvaser_pciefd_devlink.c | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
index 8145d25943de..b6d3745089d4 100644
--- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
@@ -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,
 };
-- 
2.49.0

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

[Vincent Mailhol]

On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> 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 <extja@kvaser.com>
> ---
>  .../can/kvaser_pciefd/kvaser_pciefd_devlink.c | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
> index 8145d25943de..b6d3745089d4 100644
> --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
> @@ -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 */

Nitpick:

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

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

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

Yours sincerely,
Vincent Mailhol

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

[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 <extja@kvaser.com>
---
 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(+)

diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
index 34ba393d6093..08c9ddc1ee85 100644
--- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd.h
@@ -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 */
diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
index b553fc1fc3d7..d99708a9f00c 100644
--- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
@@ -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);
diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
index b6d3745089d4..194157e6b135 100644
--- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_devlink.c
@@ -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);
+}
-- 
2.49.0

Re: [PATCH 0/9] can: kvaser_pciefd: Simplify identification of physical CAN interfaces

[Vincent Mailhol]

On 23/07/2025 at 17:32, Jimmy Assarsson wrote:
> 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