[PATCH 0/6] can: peak_usb: add ethtool interface to flashed value

[PATCH 0/6] can: peak_usb: add ethtool interface to flashed value

[Stephane Grosjean]

This series of patches offers the user the possibility to access in
read/write mode a flashed numerical value for each channel of a PEAK-System
USB - CAN/CANFD interface, through the eeprom access ethtool interface.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>

Stephane Grosjean (6):
  can: peak_usb: rename a callback to a more explicit name
  can: peak_usb: add callback to read user value of CANFD devices
  can: peak_usb: correction of a wrong field name
  can: peak_usb: allow flashing of user defined value
  can: peak_usb: replace unregister_netdev() with unregister_candev()
  can: peak_usb: add ethtool interface to user defined flashed device
    number

 drivers/net/can/usb/peak_usb/pcan_usb.c      | 35 ++++++++-
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 75 +++++++++++++++++++-
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |  9 ++-
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 51 +++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  | 26 +++++--
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h  |  3 +-
 6 files changed, 187 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/6] can: peak_usb: rename a callback to a more explicit name

[Stephane Grosjean]

The so-called "device id" is in fact a value defined by the user.
Therefore, in order not to confuse it with the device id used by the USB,
the functions for reading this user-defined value are renamed.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c      | 6 +++---
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 4 ++--
 drivers/net/can/usb/peak_usb/pcan_usb_core.h | 2 +-
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  | 6 +++---
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 17dc178f555b..954abf43d552 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -381,9 +381,9 @@ static int pcan_usb_get_serial(struct peak_usb_device *dev, u32 *serial_number)
 }
 
 /*
- * read device id from device
+ * read user device id from device
  */
-static int pcan_usb_get_device_id(struct peak_usb_device *dev, u32 *device_id)
+static int pcan_usb_get_user_devid(struct peak_usb_device *dev, u32 *device_id)
 {
 	u8 args[PCAN_USB_CMD_ARGS_LEN];
 	int err;
@@ -1015,7 +1015,7 @@ const struct peak_usb_adapter pcan_usb = {
 	.dev_init = pcan_usb_init,
 	.dev_set_bus = pcan_usb_write_mode,
 	.dev_set_bittiming = pcan_usb_set_bittiming,
-	.dev_get_device_id = pcan_usb_get_device_id,
+	.dev_get_user_devid = pcan_usb_get_user_devid,
 	.dev_decode_buf = pcan_usb_decode_buf,
 	.dev_encode_msg = pcan_usb_encode_msg,
 	.dev_start = pcan_usb_start,
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index b850ff8fe4bd..871a01e158bc 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -881,8 +881,8 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
 	}
 
 	/* get device number early */
-	if (dev->adapter->dev_get_device_id)
-		dev->adapter->dev_get_device_id(dev, &dev->device_number);
+	if (dev->adapter->dev_get_user_devid)
+		dev->adapter->dev_get_user_devid(dev, &dev->device_number);
 
 	netdev_info(netdev, "attached to %s channel %u (device %u)\n",
 			peak_usb_adapter->name, ctrl_idx, dev->device_number);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index f60af573a2e0..52bea954f2a7 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -60,7 +60,7 @@ struct peak_usb_adapter {
 	int (*dev_set_data_bittiming)(struct peak_usb_device *dev,
 				      struct can_bittiming *bt);
 	int (*dev_set_bus)(struct peak_usb_device *dev, u8 onoff);
-	int (*dev_get_device_id)(struct peak_usb_device *dev, u32 *device_id);
+	int (*dev_get_user_devid)(struct peak_usb_device *dev, u32 *device_id);
 	int (*dev_decode_buf)(struct peak_usb_device *dev, struct urb *urb);
 	int (*dev_encode_msg)(struct peak_usb_device *dev, struct sk_buff *skb,
 					u8 *obuf, size_t *size);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index ebe087f258e3..4a62cf34acb0 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -419,8 +419,8 @@ static int pcan_usb_pro_set_led(struct peak_usb_device *dev, u8 mode,
 	return pcan_usb_pro_send_cmd(dev, &um);
 }
 
-static int pcan_usb_pro_get_device_id(struct peak_usb_device *dev,
-				      u32 *device_id)
+static int pcan_usb_pro_get_user_devid(struct peak_usb_device *dev,
+				       u32 *device_id)
 {
 	struct pcan_usb_pro_devid *pdn;
 	struct pcan_usb_pro_msg um;
@@ -1075,7 +1075,7 @@ const struct peak_usb_adapter pcan_usb_pro = {
 	.dev_free = pcan_usb_pro_free,
 	.dev_set_bus = pcan_usb_pro_set_bus,
 	.dev_set_bittiming = pcan_usb_pro_set_bittiming,
-	.dev_get_device_id = pcan_usb_pro_get_device_id,
+	.dev_get_user_devid = pcan_usb_pro_get_user_devid,
 	.dev_decode_buf = pcan_usb_pro_decode_buf,
 	.dev_encode_msg = pcan_usb_pro_encode_msg,
 	.dev_start = pcan_usb_pro_start,
-- 
2.25.1

[PATCH 2/6] can: peak_usb: add callback to read user value of CANFD devices

[Stephane Grosjean]

This patch adds the specific function that allows to read a user defined
value from the non volatile memory of the USB CANFD interfaces of
PEAK-System.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 65487ec33566..ab1a8b797ece 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -420,6 +420,24 @@ static int pcan_usb_fd_set_bittiming_fast(struct peak_usb_device *dev,
 	return pcan_usb_fd_send_cmd(dev, ++cmd);
 }
 
+/* read user device id from device */
+static int pcan_usb_fd_get_user_devid(struct peak_usb_device *dev,
+				      u32 *device_id)
+{
+	struct pcan_usb_fd_device *pdev =
+		container_of(dev, struct pcan_usb_fd_device, dev);
+	struct pcan_ufd_fw_info *fw_info = &pdev->usb_if->fw_info;
+	int err;
+
+	err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
+				    PCAN_USBPRO_INFO_FW,
+				    fw_info, sizeof(*fw_info));
+	if (!err)
+		*device_id = le32_to_cpu(fw_info->dev_id[dev->ctrl_idx]);
+
+	return err;
+}
+
 /* handle restart but in asynchronously way
  * (uses PCAN-USB Pro code to complete asynchronous request)
  */
@@ -1099,6 +1117,7 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	.dev_set_bus = pcan_usb_fd_set_bus,
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
+	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
@@ -1173,6 +1192,7 @@ const struct peak_usb_adapter pcan_usb_chip = {
 	.dev_set_bus = pcan_usb_fd_set_bus,
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
+	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
@@ -1247,6 +1267,7 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	.dev_set_bus = pcan_usb_fd_set_bus,
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
+	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
@@ -1321,6 +1342,7 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	.dev_set_bus = pcan_usb_fd_set_bus,
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
+	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
-- 
2.25.1

[PATCH 3/6] can: peak_usb: correction of a wrong field name

[Stephane Grosjean]

This field read from the PCAN-USB Pro device's flash memory contains a
user value rather than a serial number.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 2 +-
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 4a62cf34acb0..6c1f7f8c8f45 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -439,7 +439,7 @@ static int pcan_usb_pro_get_user_devid(struct peak_usb_device *dev,
 		return err;
 
 	pdn = (struct pcan_usb_pro_devid *)pc;
-	*device_id = le32_to_cpu(pdn->serial_num);
+	*device_id = le32_to_cpu(pdn->dev_num);
 
 	return err;
 }
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
index 5d4cf14eb9d9..a34e0fc021c9 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
@@ -112,7 +112,7 @@ struct __packed pcan_usb_pro_devid {
 	u8 data_type;
 	u8 channel;
 	__le16 dummy;
-	__le32 serial_num;
+	__le32 dev_num;
 };
 
 #define PCAN_USBPRO_LED_DEVICE		0x00
-- 
2.25.1

[PATCH 4/6] can: peak_usb: allow flashing of user defined value

[Stephane Grosjean]

This series of patches adds a callback that allows the user to flash a
self-defined value to all USB - CAN/CANFD interfaces of PEAK-System managed
by this driver, namely:
- PCAN-USB
- PCAN-USB FD
- PCAN-USB Pro FD
- PCAN-USB X6
- PCAN-Chip USB
- PCAN-USB Pro

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c      | 20 +++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |  1 +
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 26 ++++++++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  | 15 +++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h  |  1 +
 5 files changed, 63 insertions(+)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 954abf43d552..b29daaab2e6e 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -398,6 +398,25 @@ static int pcan_usb_get_user_devid(struct peak_usb_device *dev, u32 *device_id)
 	return err;
 }
 
+/* set a new user device id in the flash memory of the device */
+static int pcan_usb_set_user_devid(struct peak_usb_device *dev, u32 devid)
+{
+	u8 args[PCAN_USB_CMD_ARGS_LEN];
+
+	/* this kind of device supports 8-bit values only */
+	if (devid > 255)
+		return -EINVAL;
+
+	/* during the flash process the device disconnects during ~1.25 s.:
+	 * prohibit access when interface is UP
+	 */
+	if (dev->netdev->flags & IFF_UP)
+		return -EBUSY;
+
+	args[0] = (u8)devid;
+	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_DEVID, PCAN_USB_SET, args);
+}
+
 /*
  * update current time ref with received timestamp
  */
@@ -1016,6 +1035,7 @@ const struct peak_usb_adapter pcan_usb = {
 	.dev_set_bus = pcan_usb_write_mode,
 	.dev_set_bittiming = pcan_usb_set_bittiming,
 	.dev_get_user_devid = pcan_usb_get_user_devid,
+	.dev_set_user_devid = pcan_usb_set_user_devid,
 	.dev_decode_buf = pcan_usb_decode_buf,
 	.dev_encode_msg = pcan_usb_encode_msg,
 	.dev_start = pcan_usb_start,
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index 52bea954f2a7..7fdc779986f0 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -61,6 +61,7 @@ struct peak_usb_adapter {
 				      struct can_bittiming *bt);
 	int (*dev_set_bus)(struct peak_usb_device *dev, u8 onoff);
 	int (*dev_get_user_devid)(struct peak_usb_device *dev, u32 *device_id);
+	int (*dev_set_user_devid)(struct peak_usb_device *dev, u32 device_id);
 	int (*dev_decode_buf)(struct peak_usb_device *dev, struct urb *urb);
 	int (*dev_encode_msg)(struct peak_usb_device *dev, struct sk_buff *skb,
 					u8 *obuf, size_t *size);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index ab1a8b797ece..7440d5b145b5 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -136,6 +136,15 @@ struct __packed pcan_ufd_ovr_msg {
 	u8	unused[3];
 };
 
+#define PCAN_UFD_CMD_DEVID_SET		0x81
+
+struct __packed pcan_ufd_device_id {
+	__le16	opcode_channel;
+
+	u16	unused;
+	__le32	device_id;
+};
+
 static inline int pufd_omsg_get_channel(struct pcan_ufd_ovr_msg *om)
 {
 	return om->channel & 0xf;
@@ -438,6 +447,19 @@ static int pcan_usb_fd_get_user_devid(struct peak_usb_device *dev,
 	return err;
 }
 
+/* set a new user device id in the flash memory of the device */
+static int pcan_usb_fd_set_user_devid(struct peak_usb_device *dev, u32 devid)
+{
+	struct pcan_ufd_device_id *cmd = pcan_usb_fd_cmd_buffer(dev);
+
+	cmd->opcode_channel = pucan_cmd_opcode_channel(dev->ctrl_idx,
+						       PCAN_UFD_CMD_DEVID_SET);
+	cmd->device_id = cpu_to_le32(devid);
+
+	/* send the command */
+	return pcan_usb_fd_send_cmd(dev, ++cmd);
+}
+
 /* handle restart but in asynchronously way
  * (uses PCAN-USB Pro code to complete asynchronous request)
  */
@@ -1118,6 +1140,7 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
 	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
+	.dev_set_user_devid = pcan_usb_fd_set_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
@@ -1193,6 +1216,7 @@ const struct peak_usb_adapter pcan_usb_chip = {
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
 	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
+	.dev_set_user_devid = pcan_usb_fd_set_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
@@ -1268,6 +1292,7 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
 	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
+	.dev_set_user_devid = pcan_usb_fd_set_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
@@ -1343,6 +1368,7 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
 	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
 	.dev_get_user_devid = pcan_usb_fd_get_user_devid,
+	.dev_set_user_devid = pcan_usb_fd_set_user_devid,
 	.dev_decode_buf = pcan_usb_fd_decode_buf,
 	.dev_start = pcan_usb_fd_start,
 	.dev_stop = pcan_usb_fd_stop,
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 6c1f7f8c8f45..e98b08746e04 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -76,6 +76,7 @@ static u16 pcan_usb_pro_sizeof_rec[256] = {
 	[PCAN_USBPRO_SETFILTR] = sizeof(struct pcan_usb_pro_filter),
 	[PCAN_USBPRO_SETTS] = sizeof(struct pcan_usb_pro_setts),
 	[PCAN_USBPRO_GETDEVID] = sizeof(struct pcan_usb_pro_devid),
+	[PCAN_USBPRO_SETDEVID] = sizeof(struct pcan_usb_pro_devid),
 	[PCAN_USBPRO_SETLED] = sizeof(struct pcan_usb_pro_setled),
 	[PCAN_USBPRO_RXMSG8] = sizeof(struct pcan_usb_pro_rxmsg),
 	[PCAN_USBPRO_RXMSG4] = sizeof(struct pcan_usb_pro_rxmsg) - 4,
@@ -149,6 +150,7 @@ static int pcan_msg_add_rec(struct pcan_usb_pro_msg *pm, int id, ...)
 
 	case PCAN_USBPRO_SETBTR:
 	case PCAN_USBPRO_GETDEVID:
+	case PCAN_USBPRO_SETDEVID:
 		*pc++ = va_arg(ap, int);
 		pc += 2;
 		*(__le32 *)pc = cpu_to_le32(va_arg(ap, u32));
@@ -444,6 +446,18 @@ static int pcan_usb_pro_get_user_devid(struct peak_usb_device *dev,
 	return err;
 }
 
+static int pcan_usb_pro_set_user_devid(struct peak_usb_device *dev,
+				       u32 device_id)
+{
+	struct pcan_usb_pro_msg um;
+
+	pcan_msg_init_empty(&um, dev->cmd_buf, PCAN_USB_MAX_CMD_LEN);
+	pcan_msg_add_rec(&um, PCAN_USBPRO_SETDEVID, dev->ctrl_idx,
+			 device_id);
+
+	return pcan_usb_pro_send_cmd(dev, &um);
+}
+
 static int pcan_usb_pro_set_bittiming(struct peak_usb_device *dev,
 				      struct can_bittiming *bt)
 {
@@ -1076,6 +1090,7 @@ const struct peak_usb_adapter pcan_usb_pro = {
 	.dev_set_bus = pcan_usb_pro_set_bus,
 	.dev_set_bittiming = pcan_usb_pro_set_bittiming,
 	.dev_get_user_devid = pcan_usb_pro_get_user_devid,
+	.dev_set_user_devid = pcan_usb_pro_set_user_devid,
 	.dev_decode_buf = pcan_usb_pro_decode_buf,
 	.dev_encode_msg = pcan_usb_pro_encode_msg,
 	.dev_start = pcan_usb_pro_start,
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
index a34e0fc021c9..28e740af905d 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
@@ -62,6 +62,7 @@ struct __packed pcan_usb_pro_fwinfo {
 #define PCAN_USBPRO_SETBTR	0x02
 #define PCAN_USBPRO_SETBUSACT	0x04
 #define PCAN_USBPRO_SETSILENT	0x05
+#define PCAN_USBPRO_SETDEVID	0x06
 #define PCAN_USBPRO_SETFILTR	0x0a
 #define PCAN_USBPRO_SETTS	0x10
 #define PCAN_USBPRO_GETDEVID	0x12
-- 
2.25.1

[PATCH 5/6] can: peak_usb: replace unregister_netdev() with unregister_candev()

[Stephane Grosjean]

This patch changes call to unregister_netdev() with unregister_candev().

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 871a01e158bc..aa8bcdcfa2fb 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -923,7 +923,7 @@ static void peak_usb_disconnect(struct usb_interface *intf)
 		dev->state &= ~PCAN_USB_STATE_CONNECTED;
 		strlcpy(name, netdev->name, IFNAMSIZ);
 
-		unregister_netdev(netdev);
+		unregister_candev(netdev);
 
 		kfree(dev->cmd_buf);
 		dev->next_siblings = NULL;
-- 
2.25.1

[PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Stephane Grosjean]

This patch introduces 3 new functions implementing support for eeprom
access of USB - CAN network interfaces managed by the driver, through the
ethtool interface. All of them (except the PCAN-USB interface) interpret
the 4 data bytes as a 32-bit value to be read/write in the non-volatile
memory of the device. The PCAN-USB only manages a single byte value.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c      |  9 +++
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 69 ++++++++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |  6 ++
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   |  3 +
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  |  3 +
 5 files changed, 90 insertions(+)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index b29daaab2e6e..60c9329701a5 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -981,8 +981,17 @@ static int pcan_usb_set_phys_id(struct net_device *netdev,
 	return err;
 }
 
+/* This device only handles 8-bit user device id. */
+static int pcan_usb_get_eeprom_len(struct net_device *netdev)
+{
+	return sizeof(u8);
+}
+
 static const struct ethtool_ops pcan_usb_ethtool_ops = {
 	.set_phys_id = pcan_usb_set_phys_id,
+	.get_eeprom_len	= pcan_usb_get_eeprom_len,
+	.get_eeprom = peak_usb_get_eeprom,
+	.set_eeprom = peak_usb_set_eeprom,
 };
 
 /*
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index aa8bcdcfa2fb..4e858d592e59 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -782,6 +782,75 @@ static const struct net_device_ops peak_usb_netdev_ops = {
 	.ndo_change_mtu = can_change_mtu,
 };
 
+/* CAN-USB devices generally handle 32-bit user device id.
+ * In case one doesn't, then it have to overload this function.
+ */
+int peak_usb_get_eeprom_len(struct net_device *netdev)
+{
+	return sizeof(u32);
+}
+
+/* Every CAN-USB device exports the dev_get_user_devid() operation. It is used
+ * here to fill the data buffer with the user defined device number.
+ */
+int peak_usb_get_eeprom(struct net_device *netdev,
+			struct ethtool_eeprom *eeprom, u8 *data)
+{
+	struct peak_usb_device *dev = netdev_priv(netdev);
+	u32 devid;
+	int err;
+
+	if (!eeprom->len)
+		return -EINVAL;
+
+	err = dev->adapter->dev_get_user_devid(dev, &devid);
+	if (!err) {
+		memcpy(data, (u8 *)&devid + eeprom->offset, eeprom->len);
+
+		/* update cached value */
+		dev->device_number = devid;
+	}
+
+	return err;
+}
+
+/* Every CAN-USB device exports the dev_get_user_devid()/dev_set_user_devid()
+ * operations. They are used here to set the new user defined device number.
+ */
+int peak_usb_set_eeprom(struct net_device *netdev,
+			struct ethtool_eeprom *eeprom, u8 *data)
+{
+	struct peak_usb_device *dev = netdev_priv(netdev);
+	u32 devid;
+	int err;
+
+	if (!eeprom->len)
+		return -EINVAL;
+
+	/* first, read the current user defined device value number */
+	err = dev->adapter->dev_get_user_devid(dev, &devid);
+	if (err) {
+		netdev_err(netdev, "Failed to init device id (err %d)\n", err);
+		return err;
+	}
+
+	/* do update the value with user given bytes */
+	memcpy((u8 *)&devid + eeprom->offset, data, eeprom->len);
+
+	/* flash the new value now */
+	err = dev->adapter->dev_set_user_devid(dev, devid);
+	if (err) {
+		netdev_err(netdev, "Failed to write new device id (err %d)\n",
+			   err);
+		return err;
+	}
+
+	/* update cached value with the new one */
+	dev->device_number = devid;
+
+	return 0;
+}
+
 /*
  * create one device which is attached to CAN controller #ctrl_idx of the
  * usb adapter.
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index 7fdc779986f0..4f4394733208 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -147,4 +147,10 @@ int peak_usb_netif_rx_64(struct sk_buff *skb, u32 ts_low, u32 ts_high);
 void peak_usb_async_complete(struct urb *urb);
 void peak_usb_restart_complete(struct peak_usb_device *dev);
 
+/* common 32-bit devid ethtool management */
+int peak_usb_get_eeprom_len(struct net_device *netdev);
+int peak_usb_get_eeprom(struct net_device *netdev,
+			struct ethtool_eeprom *eeprom, u8 *data);
+int peak_usb_set_eeprom(struct net_device *netdev,
+			struct ethtool_eeprom *eeprom, u8 *data);
 #endif
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 7440d5b145b5..cec09c7ffce2 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -1072,6 +1072,9 @@ static int pcan_usb_fd_set_phys_id(struct net_device *netdev,
 
 static const struct ethtool_ops pcan_usb_fd_ethtool_ops = {
 	.set_phys_id = pcan_usb_fd_set_phys_id,
+	.get_eeprom_len	= peak_usb_get_eeprom_len,
+	.get_eeprom = peak_usb_get_eeprom,
+	.set_eeprom = peak_usb_set_eeprom,
 };
 
 /* describes the PCAN-USB FD adapter */
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index e98b08746e04..35177a3d1eba 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -1036,6 +1036,9 @@ static int pcan_usb_pro_set_phys_id(struct net_device *netdev,
 
 static const struct ethtool_ops pcan_usb_pro_ethtool_ops = {
 	.set_phys_id = pcan_usb_pro_set_phys_id,
+	.get_eeprom_len	= peak_usb_get_eeprom_len,
+	.get_eeprom = peak_usb_get_eeprom,
+	.set_eeprom = peak_usb_set_eeprom,
 };
 
 /*
-- 
2.25.1

Re: [PATCH 2/6] can: peak_usb: add callback to read user value of CANFD devices

[Marc Kleine-Budde]

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

On 28.01.2022 16:01:53, Stephane Grosjean wrote:
> This patch adds the specific function that allows to read a user defined
> value from the non volatile memory of the USB CANFD interfaces of
> PEAK-System.
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index 65487ec33566..ab1a8b797ece 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -420,6 +420,24 @@ static int pcan_usb_fd_set_bittiming_fast(struct peak_usb_device *dev,
>  	return pcan_usb_fd_send_cmd(dev, ++cmd);
>  }
>  
> +/* read user device id from device */
> +static int pcan_usb_fd_get_user_devid(struct peak_usb_device *dev,
> +				      u32 *device_id)
> +{
> +	struct pcan_usb_fd_device *pdev =
> +		container_of(dev, struct pcan_usb_fd_device, dev);
> +	struct pcan_ufd_fw_info *fw_info = &pdev->usb_if->fw_info;
> +	int err;
> +
> +	err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
> +				    PCAN_USBPRO_INFO_FW,
> +				    fw_info, sizeof(*fw_info));
> +	if (!err)
> +		*device_id = le32_to_cpu(fw_info->dev_id[dev->ctrl_idx]);

Nitpick: please use the more common return on error:

        if (err)
                return err;

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 4/6] can: peak_usb: allow flashing of user defined value

[Marc Kleine-Budde]

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

On 28.01.2022 16:01:55, Stephane Grosjean wrote:
> This series of patches adds a callback that allows the user to flash a
> self-defined value to all USB - CAN/CANFD interfaces of PEAK-System managed
> by this driver, namely:
> - PCAN-USB
> - PCAN-USB FD
> - PCAN-USB Pro FD
> - PCAN-USB X6
> - PCAN-Chip USB
> - PCAN-USB Pro
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb.c      | 20 +++++++++++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_core.h |  1 +
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 26 ++++++++++++++++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c  | 15 +++++++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.h  |  1 +
>  5 files changed, 63 insertions(+)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 954abf43d552..b29daaab2e6e 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -398,6 +398,25 @@ static int pcan_usb_get_user_devid(struct peak_usb_device *dev, u32 *device_id)
>  	return err;
>  }
>  
> +/* set a new user device id in the flash memory of the device */
> +static int pcan_usb_set_user_devid(struct peak_usb_device *dev, u32 devid)
> +{
> +	u8 args[PCAN_USB_CMD_ARGS_LEN];
> +
> +	/* this kind of device supports 8-bit values only */
> +	if (devid > 255)
> +		return -EINVAL;

please use U8_MAX.

> +
> +	/* during the flash process the device disconnects during ~1.25 s.:
> +	 * prohibit access when interface is UP
> +	 */
> +	if (dev->netdev->flags & IFF_UP)
> +		return -EBUSY;
> +
> +	args[0] = (u8)devid;

cast not needed.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Marc Kleine-Budde]

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

On 28.01.2022 16:01:57, Stephane Grosjean wrote:
> This patch introduces 3 new functions implementing support for eeprom
> access of USB - CAN network interfaces managed by the driver, through the
> ethtool interface. All of them (except the PCAN-USB interface) interpret
> the 4 data bytes as a 32-bit value to be read/write in the non-volatile
> memory of the device. The PCAN-USB only manages a single byte value.
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb.c      |  9 +++
>  drivers/net/can/usb/peak_usb/pcan_usb_core.c | 69 ++++++++++++++++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_core.h |  6 ++
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c   |  3 +
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c  |  3 +
>  5 files changed, 90 insertions(+)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index b29daaab2e6e..60c9329701a5 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -981,8 +981,17 @@ static int pcan_usb_set_phys_id(struct net_device *netdev,
>  	return err;
>  }
>  
> +/* This device only handles 8-bit user device id. */
> +static int pcan_usb_get_eeprom_len(struct net_device *netdev)
> +{
> +	return sizeof(u8);
> +}
> +
>  static const struct ethtool_ops pcan_usb_ethtool_ops = {
>  	.set_phys_id = pcan_usb_set_phys_id,
> +	.get_eeprom_len	= pcan_usb_get_eeprom_len,
> +	.get_eeprom = peak_usb_get_eeprom,
> +	.set_eeprom = peak_usb_set_eeprom,
>  };
>  
>  /*
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index aa8bcdcfa2fb..4e858d592e59 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -782,6 +782,75 @@ static const struct net_device_ops peak_usb_netdev_ops = {
>  	.ndo_change_mtu = can_change_mtu,
>  };
>  
> +/* CAN-USB devices generally handle 32-bit user device id.
> + * In case one doesn't, then it have to overload this function.
> + */
> +int peak_usb_get_eeprom_len(struct net_device *netdev)
> +{
> +	return sizeof(u32);
> +}
> +
> +/* Every CAN-USB device exports the dev_get_user_devid() operation. It is used
> + * here to fill the data buffer with the user defined device number.
> + */
> +int peak_usb_get_eeprom(struct net_device *netdev,
> +			struct ethtool_eeprom *eeprom, u8 *data)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	u32 devid;
> +	int err;
> +
> +	if (!eeprom->len)
> +		return -EINVAL;

There already is a check for len == 0.

> +	err = dev->adapter->dev_get_user_devid(dev, &devid);
> +	if (!err) {

Please return on error.

> +		memcpy(data, (u8 *)&devid + eeprom->offset, eeprom->len);

cast not needed.
> +
> +		/* update cached value */
> +		dev->device_number = devid;
> +	}
> +
> +	return err;
> +}
> +
> +/* Every CAN-USB device exports the dev_get_user_devid()/dev_set_user_devid()
> + * operations. They are used here to set the new user defined device number.
> + */
> +int peak_usb_set_eeprom(struct net_device *netdev,
> +			struct ethtool_eeprom *eeprom, u8 *data)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	u32 devid;
> +	int err;
> +
> +	if (!eeprom->len)
> +		return -EINVAL;

There already is a check for len == 0.

> +
> +	/* first, read the current user defined device value number */
> +	err = dev->adapter->dev_get_user_devid(dev, &devid);
> +	if (err) {
> +		netdev_err(netdev, "Failed to init device id (err %d)\n", err);
> +		return err;
> +	}
> +
> +	/* do update the value with user given bytes */
> +	memcpy((u8 *)&devid + eeprom->offset, data, eeprom->len);

cast not needed.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Vincent Mailhol]

On Tue. 22 Feb 2022 à 01:39, Stéphane Grosjean
<s.grosjean@peak-system.com> wrote:
>
> Hi Marc,
>
> >On 15.02.2022 16:10:45, Marc Kleine-Budde wrote:
> >> On 11.02.2022 10:57:34, Stéphane Grosjean wrote:
> >> > endianess is handled by lower level functions (see for ex
> >> > pcan_usb_fd_get_user_devid()/pcan_usb_fd_set_user_devid() in PATCH
> >> > 2/6).
> >> >
> >> > This data is really a number and must be treated as such.
> >>
> >> What's the use case for the data/number? What's the big picture?
>
> >| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0: PEAK-System PCAN-USB FD v1 fw v3.2.0 (1 channels)
> >| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0 can0: attached to PCAN-USB FD channel 0 (device 1144201745)
> >                                                                                                     ^^^^^^^^^^
>
> >But that is something different than the serial number, right?
>
> Yep! This is a number that can be used to uniquely identify the device, regardless of the USB port or the order in which it is connected. The purpose is to allow the user to name the network interface according to this number. This is for example what is done by the "historical" driver "pcan" which is freely downloadable from www.peak-system.com.

FYI, for the dual interfaces (PCAN-USB Pro), you might want to
populate net_device::dev_port as well.
c.f. https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?h=testing&id=e233640cd3034ae65924316a0d95ccacb86ae4bd


Yours sincerely,
Vincent Mailhol

RE: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Stéphane Grosjean]

Hi Marc,

>On 15.02.2022 16:10:45, Marc Kleine-Budde wrote:
>> On 11.02.2022 10:57:34, Stéphane Grosjean wrote:
>> > endianess is handled by lower level functions (see for ex
>> > pcan_usb_fd_get_user_devid()/pcan_usb_fd_set_user_devid() in PATCH
>> > 2/6).
>> >
>> > This data is really a number and must be treated as such.
>>
>> What's the use case for the data/number? What's the big picture?

>| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0: PEAK-System PCAN-USB FD v1 fw v3.2.0 (1 channels)
>| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0 can0: attached to PCAN-USB FD channel 0 (device 1144201745)
>                                                                                                     ^^^^^^^^^^

>But that is something different than the serial number, right?

Yep! This is a number that can be used to uniquely identify the device, regardless of the USB port or the order in which it is connected. The purpose is to allow the user to name the network interface according to this number. This is for example what is done by the "historical" driver "pcan" which is freely downloadable from www.peak-system.com.

>On a little endian system this gives:
>
>| ➜ (pts/0) frogger@rpi4b4:~ (master) sudo ethtool -e can0
>| Offset          Values
>| ------          ------
>| 0x0000:         11 22 33 44
>
>On a big endian we see:
>
>| root@DistroKit:~ ethtool -e can0
>| Offset                Values
>| ------                ------
>| 0x0000:               44 33 22 11

I admit that it is quite disturbing indeed to imagine that 0x11 is not necessarily in eeprom[0]... But:

>| root@DistroKit:~ ethtool -e can0 raw on|hexdump -v -e '1 "%u\n"'
>| 1144201745
>| ➜ (pts/0) frogger@rpi4b4:~ (master) sudo ethtool -e can0 raw on|hexdump -v -e '1 "%u\n"'
>| 1144201745

is the goal: can interface name will be the same in both systems.

In any case, thanks for the tests.

--- Stephane


            De: Marc Kleine-Budde
Envoyé: Mercredi 16 février 2022 10:51
À: Stéphane Grosjean
Cc: linux-can Mailing List
Objet: Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number




On 15.02.2022 16:10:45, Marc Kleine-Budde wrote:

> On 11.02.2022 10:57:34, Stéphane Grosjean wrote:

> > endianess is handled by lower level functions (see for ex

> > pcan_usb_fd_get_user_devid()/pcan_usb_fd_set_user_devid() in PATCH

> > 2/6).

> >

> > This data is really a number and must be treated as such.

>

> What's the use case for the data/number? What's the big picture?



| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0: PEAK-System PCAN-USB FD v1 fw v3.2.0 (1 channels)

| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0 can0: attached to PCAN-USB FD channel 0 (device 1144201745)

                                                                                                     ^^^^^^^^^^



But that is something different than the serial number, right?



regards,

Marc



--

Pengutronix e.K.                 | Marc Kleine-Budde           |

Embedded Linux                   | https://www.pengutronix.de  |

Vertretung West/Dortmund         | Phone: +49-231-2826-924     |

Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html

Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Marc Kleine-Budde]

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

On 15.02.2022 16:10:45, Marc Kleine-Budde wrote:
> On 11.02.2022 10:57:34, Stéphane Grosjean wrote:
> > endianess is handled by lower level functions (see for ex
> > pcan_usb_fd_get_user_devid()/pcan_usb_fd_set_user_devid() in PATCH
> > 2/6).
> >
> > This data is really a number and must be treated as such.
> 
> What's the use case for the data/number? What's the big picture?

| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0: PEAK-System PCAN-USB FD v1 fw v3.2.0 (1 channels)
| Jan 01 05:16:21 DistroKit kernel: peak_usb 1-1:1.0 can0: attached to PCAN-USB FD channel 0 (device 1144201745)
                                                                                                     ^^^^^^^^^^

But that is something different than the serial number, right?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Marc Kleine-Budde]

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

On 11.02.2022 10:57:34, Stéphane Grosjean wrote:
> endianess is handled by lower level functions (see for ex
> pcan_usb_fd_get_user_devid()/pcan_usb_fd_set_user_devid() in PATCH
> 2/6).
>
> This data is really a number and must be treated as such.

What's the use case for the data/number? What's the big picture?

> The "ethtool -e" interface only displays a silly memory dump not very
> practical to read a number (especially a 32-bit one), requiring the
> use of hexdump such as:
> 
> ethtool -e can1 raw on | hexdump -v -e '1 "%u\n"'

On a little endian system this gives:

| ➜ (pts/0) frogger@rpi4b4:~ (master) sudo ethtool -e can0
| Offset          Values
| ------          ------
| 0x0000:         11 22 33 44 

On a big endian we see:

| root@DistroKit:~ ethtool -e can0
| Offset		Values
| ------		------
| 0x0000:		44 33 22 11

However, if we pass it through hexdump it's always the same:

| root@DistroKit:~ ethtool -e can0 raw on|hexdump -v -e '1 "%u\n"'
| 1144201745
| ➜ (pts/0) frogger@rpi4b4:~ (master) sudo ethtool -e can0 raw on|hexdump -v -e '1 "%u\n"'
| 1144201745

Why does the hexdump give the same number? I think it interprets the
memory in native endianness.

> to have a usable display. Unfortunately, these formats do not take
> endianess into account (AFAIK). Maybe you know another way?

I think from the ethtool's point of view the "EEPROM" contents is a
stream of bytes.

With your patches the EEPROM contents is not the same on big and little
endian systems, rather the EEPROM contents is a 32 bit number in native
endianness. From my point of view I think it's more consistent to have
the EEPROM contain a u32 in little endian. But I'm really interested in
the use cases.

Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

RE: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Stéphane Grosjean]

Hi Marc,

Any news about this please?

Stephane

________________________________________
De : Stéphane Grosjean <s.grosjean@peak-system.com>
Envoyé : vendredi 11 février 2022 11:57
À : Marc Kleine-Budde
Cc : linux-can Mailing List
Objet : RE: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

Hi Marc,

endianess is handled by lower level functions (see for ex pcan_usb_fd_get_user_devid()/pcan_usb_fd_set_user_devid() in PATCH 2/6).

This data is really a number and must be treated as such. The "ethtool -e" interface only displays a silly memory dump not very practical to read a number (especially a 32-bit one), requiring the use of hexdump such as:

ethtool -e can1 raw on | hexdump -v -e '1 "%u\n"'

to have a usable display. Unfortunately, these formats do not take endianess into account (AFAIK). Maybe you know another way?

Regards,


— Stéphane


            De: Marc Kleine-Budde
Envoyé: Lundi 31 janvier 2022 15:31
À: Stéphane Grosjean
Cc: linux-can Mailing List
Objet: Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number




On 31.01.2022 14:25:28, Stéphane Grosjean wrote:

> >> +     u32 devid;

> >>

> >> +             memcpy(data, (u8 *)&devid + eeprom->offset, eeprom->len);

> >

> > cast not needed.

> >

>

> We need to cast the u32 * into a u8 * because eeprom->offset is a count of bytes, isn't it?



Doh! right.



What about endianness? I think it's better to use an array of bytes

everywhere.



Marc



--

Pengutronix e.K.                 | Marc Kleine-Budde           |

Embedded Linux                   | https://www.pengutronix.de  |

Vertretung West/Dortmund         | Phone: +49-231-2826-924     |

Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html

RE: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Stéphane Grosjean]

Hi Marc,

endianess is handled by lower level functions (see for ex pcan_usb_fd_get_user_devid()/pcan_usb_fd_set_user_devid() in PATCH 2/6).

This data is really a number and must be treated as such. The "ethtool -e" interface only displays a silly memory dump not very practical to read a number (especially a 32-bit one), requiring the use of hexdump such as:

ethtool -e can1 raw on | hexdump -v -e '1 "%u\n"'

to have a usable display. Unfortunately, these formats do not take endianess into account (AFAIK). Maybe you know another way?

Regards,


— Stéphane


            De: Marc Kleine-Budde
Envoyé: Lundi 31 janvier 2022 15:31
À: Stéphane Grosjean
Cc: linux-can Mailing List
Objet: Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number




On 31.01.2022 14:25:28, Stéphane Grosjean wrote:

> >> +     u32 devid;

> >>

> >> +             memcpy(data, (u8 *)&devid + eeprom->offset, eeprom->len);

> >

> > cast not needed.

> >

>

> We need to cast the u32 * into a u8 * because eeprom->offset is a count of bytes, isn't it?



Doh! right.



What about endianness? I think it's better to use an array of bytes

everywhere.



Marc



--

Pengutronix e.K.                 | Marc Kleine-Budde           |

Embedded Linux                   | https://www.pengutronix.de  |

Vertretung West/Dortmund         | Phone: +49-231-2826-924     |

Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html

Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Marc Kleine-Budde]

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

On 31.01.2022 14:25:28, Stéphane Grosjean wrote:
> >> +     u32 devid;
> >>
> >> +             memcpy(data, (u8 *)&devid + eeprom->offset, eeprom->len);
> >
> > cast not needed.
> >
> 
> We need to cast the u32 * into a u8 * because eeprom->offset is a count of bytes, isn't it?

Doh! right.

What about endianness? I think it's better to use an array of bytes
everywhere.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

RE: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number

[Stéphane Grosjean]

Hi,

>> +     u32 devid;
>>
>> +             memcpy(data, (u8 *)&devid + eeprom->offset, eeprom->len);
>
> cast not needed.
>

We need to cast the u32 * into a u8 * because eeprom->offset is a count of bytes, isn't it?

Regards,

— Stéphane






            De: Marc Kleine-Budde
Envoyé: Samedi 29 janvier 2022 14:58
À: Stéphane Grosjean
Cc: linux-can Mailing List
Objet: Re: [PATCH 6/6] can: peak_usb: add ethtool interface to user defined flashed device number




On 28.01.2022 16:01:57, Stephane Grosjean wrote:

> This patch introduces 3 new functions implementing support for eeprom

> access of USB - CAN network interfaces managed by the driver, through the

> ethtool interface. All of them (except the PCAN-USB interface) interpret

> the 4 data bytes as a 32-bit value to be read/write in the non-volatile

> memory of the device. The PCAN-USB only manages a single byte value.

>

> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>

> ---

>  drivers/net/can/usb/peak_usb/pcan_usb.c      |  9 +++

>  drivers/net/can/usb/peak_usb/pcan_usb_core.c | 69 ++++++++++++++++++++

>  drivers/net/can/usb/peak_usb/pcan_usb_core.h |  6 ++

>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c   |  3 +

>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c  |  3 +

>  5 files changed, 90 insertions(+)

>

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c

> index b29daaab2e6e..60c9329701a5 100644

> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c

> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c

> @@ -981,8 +981,17 @@ static int pcan_usb_set_phys_id(struct net_device *netdev,

>        return err;

>  }

>

> +/* This device only handles 8-bit user device id. */

> +static int pcan_usb_get_eeprom_len(struct net_device *netdev)

> +{

> +     return sizeof(u8);

> +}

> +

>  static const struct ethtool_ops pcan_usb_ethtool_ops = {

>        .set_phys_id = pcan_usb_set_phys_id,

> +     .get_eeprom_len = pcan_usb_get_eeprom_len,

> +     .get_eeprom = peak_usb_get_eeprom,

> +     .set_eeprom = peak_usb_set_eeprom,

>  };

>

>  /*

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> index aa8bcdcfa2fb..4e858d592e59 100644

> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> @@ -782,6 +782,75 @@ static const struct net_device_ops peak_usb_netdev_ops = {

>        .ndo_change_mtu = can_change_mtu,

>  };

>

> +/* CAN-USB devices generally handle 32-bit user device id.

> + * In case one doesn't, then it have to overload this function.

> + */

> +int peak_usb_get_eeprom_len(struct net_device *netdev)

> +{

> +     return sizeof(u32);

> +}

> +

> +/* Every CAN-USB device exports the dev_get_user_devid() operation. It is used

> + * here to fill the data buffer with the user defined device number.

> + */

> +int peak_usb_get_eeprom(struct net_device *netdev,

> +                     struct ethtool_eeprom *eeprom, u8 *data)

> +{

> +     struct peak_usb_device *dev = netdev_priv(netdev);

> +     u32 devid;

> +     int err;

> +

> +     if (!eeprom->len)

> +             return -EINVAL;



There already is a check for len == 0.



> +     err = dev->adapter->dev_get_user_devid(dev, &devid);

> +     if (!err) {



Please return on error.



> +             memcpy(data, (u8 *)&devid + eeprom->offset, eeprom->len);



cast not needed.

> +

> +             /* update cached value */

> +             dev->device_number = devid;

> +     }

> +

> +     return err;

> +}

> +

> +/* Every CAN-USB device exports the dev_get_user_devid()/dev_set_user_devid()

> + * operations. They are used here to set the new user defined device number.

> + */

> +int peak_usb_set_eeprom(struct net_device *netdev,

> +                     struct ethtool_eeprom *eeprom, u8 *data)

> +{

> +     struct peak_usb_device *dev = netdev_priv(netdev);

> +     u32 devid;

> +     int err;

> +

> +     if (!eeprom->len)

> +             return -EINVAL;



There already is a check for len == 0.



> +

> +     /* first, read the current user defined device value number */

> +     err = dev->adapter->dev_get_user_devid(dev, &devid);

> +     if (err) {

> +             netdev_err(netdev, "Failed to init device id (err %d)\n", err);

> +             return err;

> +     }

> +

> +     /* do update the value with user given bytes */

> +     memcpy((u8 *)&devid + eeprom->offset, data, eeprom->len);



cast not needed.



regards,

Marc



--

Pengutronix e.K.                 | Marc Kleine-Budde           |

Embedded Linux                   | https://www.pengutronix.de  |

Vertretung West/Dortmund         | Phone: +49-231-2826-924     |

Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html