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