All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/5] kvaser_{pciefd,usb} fixes
@ 2021-11-19 13:19 Jimmy Assarsson
  2021-11-19 13:19 ` [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-19 13:19 UTC (permalink / raw)
  To: linux-can; +Cc: Jimmy Assarsson, Jimmy Assarsson

The first patches fix net_device_stats when receiving error frames.
The last two patches fix the CAN clock frequency for Kvaser Leaf devices.

Note that this series contains patches for both kvaser_usb and
kvaser_pciefd driver.

Jimmy Assarsson (5):
  can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for
    error frames
  can: kvaser_pciefd: Increase correct stats->{rx,tx}_errors counter
  can: kvaser_usb: Do not increase stats->rx_{packets,bytes} for error
    frames
  can: kvaser_usb: Use CAN_MHZ define in assignment of frequency
  can: kvaser_usb: Get CAN clock frequency from device

 drivers/net/can/kvaser_pciefd.c               | 11 +--
 .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 13 ++--
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 70 ++++++++++++++++---
 3 files changed, 72 insertions(+), 22 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames
  2021-11-19 13:19 [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Jimmy Assarsson
@ 2021-11-19 13:19 ` Jimmy Assarsson
  2021-11-20  3:06   ` Vincent Mailhol
  2021-11-19 13:19 ` [PATCH v1 2/5] can: kvaser_pciefd: Increase correct stats->{rx,tx}_errors counter Jimmy Assarsson
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-19 13:19 UTC (permalink / raw)
  To: linux-can; +Cc: Jimmy Assarsson, Jimmy Assarsson

Do not increase net_device_stats rx_{packets,bytes} when receiving
error frames.

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

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 74d9899fc904..2c98befcf2a0 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -1304,9 +1304,6 @@ static int kvaser_pciefd_rx_error_frame(struct kvaser_pciefd_can *can,
 	cf->data[6] = bec.txerr;
 	cf->data[7] = bec.rxerr;
 
-	stats->rx_packets++;
-	stats->rx_bytes += cf->len;
-
 	netif_rx(skb);
 	return 0;
 }
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 2/5] can: kvaser_pciefd: Increase correct stats->{rx,tx}_errors counter
  2021-11-19 13:19 [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Jimmy Assarsson
  2021-11-19 13:19 ` [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
@ 2021-11-19 13:19 ` Jimmy Assarsson
  2021-11-19 13:19 ` [PATCH v1 3/5] can: kvaser_usb: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-19 13:19 UTC (permalink / raw)
  To: linux-can; +Cc: Jimmy Assarsson, Jimmy Assarsson

Check the direction bit in the error frame packet (EPACK) to determine
which net_device_stats {rx,tx}_errors counter to increase.

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

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 2c98befcf2a0..6fb18ced1806 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -248,6 +248,9 @@ MODULE_DESCRIPTION("CAN driver for Kvaser CAN/PCIe devices");
 #define KVASER_PCIEFD_SPACK_EWLR BIT(23)
 #define KVASER_PCIEFD_SPACK_EPLR BIT(24)
 
+/* Kvaser KCAN_EPACK second word */
+#define KVASER_PCIEFD_EPACK_DIR_TX BIT(0)
+
 struct kvaser_pciefd;
 
 struct kvaser_pciefd_can {
@@ -1285,7 +1288,10 @@ static int kvaser_pciefd_rx_error_frame(struct kvaser_pciefd_can *can,
 
 	can->err_rep_cnt++;
 	can->can.can_stats.bus_error++;
-	stats->rx_errors++;
+	if (p->header[1] & KVASER_PCIEFD_EPACK_DIR_TX)
+		stats->tx_errors++;
+	else
+		stats->rx_errors++;
 
 	can->bec.txerr = bec.txerr;
 	can->bec.rxerr = bec.rxerr;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 3/5] can: kvaser_usb: Do not increase stats->rx_{packets,bytes} for error frames
  2021-11-19 13:19 [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Jimmy Assarsson
  2021-11-19 13:19 ` [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
  2021-11-19 13:19 ` [PATCH v1 2/5] can: kvaser_pciefd: Increase correct stats->{rx,tx}_errors counter Jimmy Assarsson
@ 2021-11-19 13:19 ` Jimmy Assarsson
  2021-11-19 13:19 ` [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency Jimmy Assarsson
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-19 13:19 UTC (permalink / raw)
  To: linux-can; +Cc: Jimmy Assarsson, Jimmy Assarsson

Do not increase net_device_stats rx_{packets,bytes} when receiving
error frames.

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index dcee8dc828ec..aa81b92237ca 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -920,8 +920,6 @@ static void kvaser_usb_hydra_update_state(struct kvaser_usb_net_priv *priv,
 	cf->data[7] = bec->rxerr;
 
 	stats = &netdev->stats;
-	stats->rx_packets++;
-	stats->rx_bytes += cf->len;
 	netif_rx(skb);
 }
 
@@ -1074,8 +1072,6 @@ kvaser_usb_hydra_error_frame(struct kvaser_usb_net_priv *priv,
 	cf->data[6] = bec.txerr;
 	cf->data[7] = bec.rxerr;
 
-	stats->rx_packets++;
-	stats->rx_bytes += cf->len;
 	netif_rx(skb);
 
 	priv->bec.txerr = bec.txerr;
@@ -1109,8 +1105,6 @@ static void kvaser_usb_hydra_one_shot_fail(struct kvaser_usb_net_priv *priv,
 	}
 
 	stats->tx_errors++;
-	stats->rx_packets++;
-	stats->rx_bytes += cf->len;
 	netif_rx(skb);
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency
  2021-11-19 13:19 [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Jimmy Assarsson
                   ` (2 preceding siblings ...)
  2021-11-19 13:19 ` [PATCH v1 3/5] can: kvaser_usb: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
@ 2021-11-19 13:19 ` Jimmy Assarsson
  2021-11-19 13:30   ` Jimmy Assarsson
  2021-11-19 13:19 ` [PATCH v1 5/5] can: kvaser_usb: Get CAN clock frequency from device Jimmy Assarsson
  2021-12-08  9:27 ` [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Marc Kleine-Budde
  5 siblings, 1 reply; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-19 13:19 UTC (permalink / raw)
  To: linux-can; +Cc: Jimmy Assarsson, Jimmy Assarsson

Use the CAN_MHZ define when assigning frequencies.

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 7 ++++---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index aa81b92237ca..221339338414 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -25,6 +25,7 @@
 #include <linux/usb.h>
 
 #include <linux/can.h>
+#include <linux/can/bittiming.h>
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/netlink.h>
@@ -2034,7 +2035,7 @@ const struct kvaser_usb_dev_ops kvaser_usb_hydra_dev_ops = {
 
 static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_kcan = {
 	.clock = {
-		.freq = 80000000,
+		.freq = 80 * CAN_MHZ,
 	},
 	.timestamp_freq = 80,
 	.bittiming_const = &kvaser_usb_hydra_kcan_bittiming_c,
@@ -2043,7 +2044,7 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_kcan = {
 
 static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_flexc = {
 	.clock = {
-		.freq = 24000000,
+		.freq = 24 * CAN_MHZ,
 	},
 	.timestamp_freq = 1,
 	.bittiming_const = &kvaser_usb_hydra_flexc_bittiming_c,
@@ -2051,7 +2052,7 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_flexc = {
 
 static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_rt = {
 	.clock = {
-		.freq = 80000000,
+		.freq = 80 * CAN_MHZ,
 	},
 	.timestamp_freq = 24,
 	.bittiming_const = &kvaser_usb_hydra_rt_bittiming_c,
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 59ba7c7beec0..506e4b76d119 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -22,6 +22,7 @@
 #include <linux/usb.h>
 
 #include <linux/can.h>
+#include <linux/can/bittiming.h>
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/netlink.h>
@@ -31,7 +32,7 @@
 /* Forward declaration */
 static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg;
 
-#define CAN_USB_CLOCK			8000000
+#define CAN_USB_CLOCK			(8 * CAN_MHZ)
 #define MAX_USBCAN_NET_DEVICES		2
 
 /* Command header size */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v1 5/5] can: kvaser_usb: Get CAN clock frequency from device
  2021-11-19 13:19 [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Jimmy Assarsson
                   ` (3 preceding siblings ...)
  2021-11-19 13:19 ` [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency Jimmy Assarsson
@ 2021-11-19 13:19 ` Jimmy Assarsson
  2021-12-08  9:27 ` [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Marc Kleine-Budde
  5 siblings, 0 replies; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-19 13:19 UTC (permalink / raw)
  To: linux-can; +Cc: Jimmy Assarsson, Jimmy Assarsson

Get the CAN clock frequency from device, since the various Kvaser Leaf
products use different CAN clocks.

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 69 ++++++++++++++++---
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 506e4b76d119..5153af71c47a 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -29,10 +29,12 @@
 
 #include "kvaser_usb.h"
 
-/* Forward declaration */
-static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg;
+/* Forward declarations */
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_8mhz;
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_16mhz;
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_24mhz;
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_32mhz;
 
-#define CAN_USB_CLOCK			(8 * CAN_MHZ)
 #define MAX_USBCAN_NET_DEVICES		2
 
 /* Command header size */
@@ -81,6 +83,12 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg;
 
 #define CMD_LEAF_LOG_MESSAGE		106
 
+/* Leaf frequency options */
+#define KVASER_USB_LEAF_SWOPTION_FREQ_MASK 0x60
+#define KVASER_USB_LEAF_SWOPTION_FREQ_16_MHZ_CLK 0
+#define KVASER_USB_LEAF_SWOPTION_FREQ_32_MHZ_CLK BIT(5)
+#define KVASER_USB_LEAF_SWOPTION_FREQ_24_MHZ_CLK BIT(6)
+
 /* error factors */
 #define M16C_EF_ACKE			BIT(0)
 #define M16C_EF_CRCE			BIT(1)
@@ -472,6 +480,27 @@ static int kvaser_usb_leaf_send_simple_cmd(const struct kvaser_usb *dev,
 	return rc;
 }
 
+static void kvaser_usb_leaf_get_software_info_leaf(struct kvaser_usb *dev,
+						   const struct leaf_cmd_softinfo *softinfo)
+{
+	u32 sw_options = le32_to_cpu(softinfo->sw_options);
+
+	dev->fw_version = le32_to_cpu(softinfo->fw_version);
+	dev->max_tx_urbs = le16_to_cpu(softinfo->max_outstanding_tx);
+
+	switch (sw_options & KVASER_USB_LEAF_SWOPTION_FREQ_MASK) {
+	case KVASER_USB_LEAF_SWOPTION_FREQ_16_MHZ_CLK:
+		dev->cfg = &kvaser_usb_leaf_dev_cfg_16mhz;
+		break;
+	case KVASER_USB_LEAF_SWOPTION_FREQ_24_MHZ_CLK:
+		dev->cfg = &kvaser_usb_leaf_dev_cfg_24mhz;
+		break;
+	case KVASER_USB_LEAF_SWOPTION_FREQ_32_MHZ_CLK:
+		dev->cfg = &kvaser_usb_leaf_dev_cfg_32mhz;
+		break;
+	}
+}
+
 static int kvaser_usb_leaf_get_software_info_inner(struct kvaser_usb *dev)
 {
 	struct kvaser_cmd cmd;
@@ -487,14 +516,13 @@ static int kvaser_usb_leaf_get_software_info_inner(struct kvaser_usb *dev)
 
 	switch (dev->card_data.leaf.family) {
 	case KVASER_LEAF:
-		dev->fw_version = le32_to_cpu(cmd.u.leaf.softinfo.fw_version);
-		dev->max_tx_urbs =
-			le16_to_cpu(cmd.u.leaf.softinfo.max_outstanding_tx);
+		kvaser_usb_leaf_get_software_info_leaf(dev, &cmd.u.leaf.softinfo);
 		break;
 	case KVASER_USBCAN:
 		dev->fw_version = le32_to_cpu(cmd.u.usbcan.softinfo.fw_version);
 		dev->max_tx_urbs =
 			le16_to_cpu(cmd.u.usbcan.softinfo.max_outstanding_tx);
+		dev->cfg = &kvaser_usb_leaf_dev_cfg_8mhz;
 		break;
 	}
 
@@ -1226,7 +1254,6 @@ static int kvaser_usb_leaf_init_card(struct kvaser_usb *dev)
 {
 	struct kvaser_usb_dev_card_data *card_data = &dev->card_data;
 
-	dev->cfg = &kvaser_usb_leaf_dev_cfg;
 	card_data->ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
 
 	return 0;
@@ -1350,9 +1377,33 @@ const struct kvaser_usb_dev_ops kvaser_usb_leaf_dev_ops = {
 	.dev_frame_to_cmd = kvaser_usb_leaf_frame_to_cmd,
 };
 
-static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg = {
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_8mhz = {
+	.clock = {
+		.freq = 8 * CAN_MHZ,
+	},
+	.timestamp_freq = 1,
+	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
+};
+
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_16mhz = {
+	.clock = {
+		.freq = 16 * CAN_MHZ,
+	},
+	.timestamp_freq = 1,
+	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
+};
+
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_24mhz = {
+	.clock = {
+		.freq = 24 * CAN_MHZ,
+	},
+	.timestamp_freq = 1,
+	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
+};
+
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_32mhz = {
 	.clock = {
-		.freq = CAN_USB_CLOCK,
+		.freq = 32 * CAN_MHZ,
 	},
 	.timestamp_freq = 1,
 	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency
  2021-11-19 13:19 ` [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency Jimmy Assarsson
@ 2021-11-19 13:30   ` Jimmy Assarsson
  2021-11-19 15:42     ` Vincent MAILHOL
  0 siblings, 1 reply; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-19 13:30 UTC (permalink / raw)
  To: linux-can, Vincent MAILHOL; +Cc: Jimmy Assarsson

On 2021-11-19 14:19, Jimmy Assarsson wrote:
> Use the CAN_MHZ define when assigning frequencies.

Maybe we should use the HZ_PER_MHZ define introduced in kernel 5.15 in
linux/units.h. It also got defines for KILO and MEGA, which we also got
in linux/can/bittiming.h.

What do you think?

...

>   static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_kcan = {
>   	.clock = {
> -		.freq = 80000000,
> +		.freq = 80 * CAN_MHZ,
>   	},

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency
  2021-11-19 13:30   ` Jimmy Assarsson
@ 2021-11-19 15:42     ` Vincent MAILHOL
  2021-11-19 15:48       ` Vincent MAILHOL
  0 siblings, 1 reply; 13+ messages in thread
From: Vincent MAILHOL @ 2021-11-19 15:42 UTC (permalink / raw)
  To: Jimmy Assarsson; +Cc: linux-can, Jimmy Assarsson

On Fri. 19 Nov 2021 at 22:30, Jimmy Assarsson <extja@kvaser.com> wrote:
>
> On 2021-11-19 14:19, Jimmy Assarsson wrote:
> > Use the CAN_MHZ define when assigning frequencies.
>
> Maybe we should use the HZ_PER_MHZ define introduced in kernel 5.15 in
> linux/units.h. It also got defines for KILO and MEGA, which we also got
> in linux/can/bittiming.h.
>
> What do you think?

With the recent changes in linux/units.h, I am perfectly fine to
remove the CAN_MHZ, CAN_KBPS and CAN_MBPS macros from
linux/can/bittiming.h.

I am just bothered by the name of the macro HZ_PER_MHZ. Hertz per Mega
Hertz is a ratio without a unit. The clock speed is expressed in
hertz. So:

.frep = 80 * MEGA,

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency
  2021-11-19 15:42     ` Vincent MAILHOL
@ 2021-11-19 15:48       ` Vincent MAILHOL
  0 siblings, 0 replies; 13+ messages in thread
From: Vincent MAILHOL @ 2021-11-19 15:48 UTC (permalink / raw)
  To: Jimmy Assarsson; +Cc: linux-can, Jimmy Assarsson

(I inadvertently sent my previous message while I was still
writing it, resending with full comments)

On Fri. 19 Nov 2021 at 22:30, Jimmy Assarsson <extja@kvaser.com> wrote:
>
> On 2021-11-19 14:19, Jimmy Assarsson wrote:
> > Use the CAN_MHZ define when assigning frequencies.
>
> Maybe we should use the HZ_PER_MHZ define introduced in kernel 5.15 in
> linux/units.h. It also got defines for KILO and MEGA, which we also got
> in linux/can/bittiming.h.
>
> What do you think?

With the recent changes in linux/units.h, I am perfectly fine to
remove the CAN_MHZ, CAN_KBPS and CAN_MBPS macros from
linux/can/bittiming.h.

I am just bothered by the name of the macro HZ_PER_MHZ. Hertz per Mega
Hertz is a ratio without a unit (physically speaking). The
clock speed is expressed in hertz. So:

| .frep = 80 * MEGA,

would make more sense.

I see HZ_PER_MHZ as something used for conversion, e.g.:
| freq_in_hz = HZ_PER_MHZ * frep_in_mhz;

Yours sincerely,
Vincent Mailhol

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames
  2021-11-19 13:19 ` [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
@ 2021-11-20  3:06   ` Vincent Mailhol
  2021-11-23  7:44     ` Jimmy Assarsson
  0 siblings, 1 reply; 13+ messages in thread
From: Vincent Mailhol @ 2021-11-20  3:06 UTC (permalink / raw)
  To: extja, linux-can; +Cc: jimmyassarsson, Vincent Mailhol

On Fri. 19 Nov 2021 at 05:20, Jimmy Assarsson <extja@kvaser.com> wrote:
> Do not increase net_device_stats rx_{packets,bytes} when receiving
> error frames.
> 
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index 74d9899fc904..2c98befcf2a0 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -1304,9 +1304,6 @@ static int kvaser_pciefd_rx_error_frame(struct kvaser_pciefd_can *can,
>  	cf->data[6] = bec.txerr;
>  	cf->data[7] = bec.rxerr;
>  
> -	stats->rx_packets++;
> -	stats->rx_bytes += cf->len;
> -
>  	netif_rx(skb);
>  	return 0;
>  }

I think that this patch makes sense because the CAN error frames do
not exists on the wire: only an error flag is transmitted.

However, the current consensus is that the rx_packets and rx_bytes
statistics should be incremented for CAN error frames. And I think
that consistency between the drivers is the first priority.

I inquired here in the past to ask if it made sense to stop increasing
the rx stats for CAN error frames:

https://lore.kernel.org/linux-can/CAMZ6Rq+8YSRqXU7CPrT9FKnWZ1G9xkSr3wt185r2CswmxhXPVg@mail.gmail.com/t/#u

But the discussion did not raise interest. I am fine to send a tree
wide cleaning patch, but first, I would like to have people agree on
this.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames
  2021-11-20  3:06   ` Vincent Mailhol
@ 2021-11-23  7:44     ` Jimmy Assarsson
  0 siblings, 0 replies; 13+ messages in thread
From: Jimmy Assarsson @ 2021-11-23  7:44 UTC (permalink / raw)
  To: Vincent Mailhol, linux-can; +Cc: jimmyassarsson

On 2021-11-20 04:06, Vincent Mailhol wrote:
> On Fri. 19 Nov 2021 at 05:20, Jimmy Assarsson <extja@kvaser.com> wrote:
>> Do not increase net_device_stats rx_{packets,bytes} when receiving
>> error frames.
>>
>> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
>> ---
>>   drivers/net/can/kvaser_pciefd.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
>> index 74d9899fc904..2c98befcf2a0 100644
>> --- a/drivers/net/can/kvaser_pciefd.c
>> +++ b/drivers/net/can/kvaser_pciefd.c
>> @@ -1304,9 +1304,6 @@ static int kvaser_pciefd_rx_error_frame(struct kvaser_pciefd_can *can,
>>   	cf->data[6] = bec.txerr;
>>   	cf->data[7] = bec.rxerr;
>>   
>> -	stats->rx_packets++;
>> -	stats->rx_bytes += cf->len;
>> -
>>   	netif_rx(skb);
>>   	return 0;
>>   }
> 
> I think that this patch makes sense because the CAN error frames do
> not exists on the wire: only an error flag is transmitted.
> 
> However, the current consensus is that the rx_packets and rx_bytes
> statistics should be incremented for CAN error frames. And I think
> that consistency between the drivers is the first priority.
> 
> I inquired here in the past to ask if it made sense to stop increasing
> the rx stats for CAN error frames:
> 
> https://lore.kernel.org/linux-can/CAMZ6Rq+8YSRqXU7CPrT9FKnWZ1G9xkSr3wt185r2CswmxhXPVg@mail.gmail.com/t/#u
> 
> But the discussion did not raise interest. I am fine to send a tree
> wide cleaning patch, but first, I would like to have people agree on
> this.

Thanks, I've not seen that discussion.
I agree, it doesn't make sense for a user to see the rx_packets and
rx_bytes increase, when in reality they didn't receive any valid frames.

This change started as a request from one of our customers, that got
confused by the increase of rx_packets, when connected to a bus with
wrong bitrate.

I'm in favour of a tree wide patch.

Best regards,
jimmy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 0/5] kvaser_{pciefd,usb} fixes
  2021-11-19 13:19 [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Jimmy Assarsson
                   ` (4 preceding siblings ...)
  2021-11-19 13:19 ` [PATCH v1 5/5] can: kvaser_usb: Get CAN clock frequency from device Jimmy Assarsson
@ 2021-12-08  9:27 ` Marc Kleine-Budde
  2021-12-08 11:13   ` Jimmy Assarsson
  5 siblings, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2021-12-08  9:27 UTC (permalink / raw)
  To: Jimmy Assarsson; +Cc: linux-can, Jimmy Assarsson

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

On 19.11.2021 14:19:10, Jimmy Assarsson wrote:
> The first patches fix net_device_stats when receiving error frames.
> The last two patches fix the CAN clock frequency for Kvaser Leaf devices.
> 
> Note that this series contains patches for both kvaser_usb and
> kvaser_pciefd driver.

What's left of this series, after applying Vincent's stats cleanup [1]
series?

[1]
https://lore.kernel.org/all/20211207121531.42941-1-mailhol.vincent@wanadoo.fr/

Can you send a series only containing the remaining patches?

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 0/5] kvaser_{pciefd,usb} fixes
  2021-12-08  9:27 ` [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Marc Kleine-Budde
@ 2021-12-08 11:13   ` Jimmy Assarsson
  0 siblings, 0 replies; 13+ messages in thread
From: Jimmy Assarsson @ 2021-12-08 11:13 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: linux-can, Jimmy Assarsson

On 2021-12-08 10:27, Marc Kleine-Budde wrote:
> On 19.11.2021 14:19:10, Jimmy Assarsson wrote:
>> The first patches fix net_device_stats when receiving error frames.
>> The last two patches fix the CAN clock frequency for Kvaser Leaf devices.
>>
>> Note that this series contains patches for both kvaser_usb and
>> kvaser_pciefd driver.
> 
> What's left of this series, after applying Vincent's stats cleanup [1]
> series?
> 
> [1]
> https://lore.kernel.org/all/20211207121531.42941-1-mailhol.vincent@wanadoo.fr/
> 
> Can you send a series only containing the remaining patches?

Fixed https://lore.kernel.org/all/20211208110940.185629-1-extja@kvaser.com

Best regards,
jimmy

> 
> regards,
> Marc


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-12-08 11:13 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 13:19 [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Jimmy Assarsson
2021-11-19 13:19 ` [PATCH v1 1/5] can: kvaser_pciefd: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
2021-11-20  3:06   ` Vincent Mailhol
2021-11-23  7:44     ` Jimmy Assarsson
2021-11-19 13:19 ` [PATCH v1 2/5] can: kvaser_pciefd: Increase correct stats->{rx,tx}_errors counter Jimmy Assarsson
2021-11-19 13:19 ` [PATCH v1 3/5] can: kvaser_usb: Do not increase stats->rx_{packets,bytes} for error frames Jimmy Assarsson
2021-11-19 13:19 ` [PATCH v1 4/5] can: kvaser_usb: Use CAN_MHZ define in assignment of frequency Jimmy Assarsson
2021-11-19 13:30   ` Jimmy Assarsson
2021-11-19 15:42     ` Vincent MAILHOL
2021-11-19 15:48       ` Vincent MAILHOL
2021-11-19 13:19 ` [PATCH v1 5/5] can: kvaser_usb: Get CAN clock frequency from device Jimmy Assarsson
2021-12-08  9:27 ` [PATCH v1 0/5] kvaser_{pciefd,usb} fixes Marc Kleine-Budde
2021-12-08 11:13   ` Jimmy Assarsson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.