| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMZ6RqKtn-EuSLCTAppzz0THzr7KUYBUBOTHkwhSCzrDyzSzhw@mail.gmail.com>
Date: Fri, 3 Dec 2021 13:02:25 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Jimmy Assarsson <extja@...ser.com>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org,
Oliver Hartkopp <socketcan@...tkopp.net>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v3 2/5] can: kvaser_usb: do not increase tx statistics
when sending error message frames
On Fri. 3 Dec 2021 at 08:35, Jimmy Assarsson <extja@...ser.com> wrote:
> On 2021-11-28 13:37, Vincent Mailhol wrote:
> > The CAN error message frames (i.e. error skb) are an interface
> > specific to socket CAN. The payload of the CAN error message frames
> > does not correspond to any actual data sent on the wire. Only an error
> > flag and a delimiter are transmitted when an error occurs (c.f. ISO
> > 11898-1 section 10.4.4.2 "Error flag").
> >
> > For this reason, it makes no sense to increment the tx_packets and
> > tx_bytes fields of struct net_device_stats when sending an error
> > message frame because no actual payload will be transmitted on the
> > wire.
> >
> > N.B. Sending error message frames is a very specific feature which, at
> > the moment, is only supported by the Kvaser Hydra hardware. Please
> > refer to [1] for more details on the topic.
> >
> > [1] https://lore.kernel.org/linux-can/CAMZ6RqK0rTNg3u3mBpZOoY51jLZ-et-J01tY6-+mWsM4meVw-A@mail.gmail.com/t/#u
> >
> > CC: Jimmy Assarsson <extja@...ser.com>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
>
> Hi Vincent!
>
> Thanks for the patch.
> There are flags in the TX ACK package, which makes it possible to
> determine if it was an error frame or not. So we don't need to get
> the original CAN frame to determine this.
> I suggest the following change:
This is a great suggestion. I was not a fan of getting the
original CAN frame, this TX ACK solves the issue.
> ---
> .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 25 ++++++++++++-------
> 1 file changed, 16 insertions(+), 9 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 3398da323126..01b076f04e26 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> @@ -295,6 +295,7 @@ struct kvaser_cmd {
> #define KVASER_USB_HYDRA_CF_FLAG_OVERRUN BIT(1)
> #define KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME BIT(4)
> #define KVASER_USB_HYDRA_CF_FLAG_EXTENDED_ID BIT(5)
> +#define KVASER_USB_HYDRA_CF_FLAG_TX_ACK BIT(6)
> /* CAN frame flags. Used in ext_rx_can and ext_tx_can */
> #define KVASER_USB_HYDRA_CF_FLAG_OSM_NACK BIT(12)
> #define KVASER_USB_HYDRA_CF_FLAG_ABL BIT(13)
> @@ -1112,7 +1113,9 @@ static void kvaser_usb_hydra_tx_acknowledge(const
> struct kvaser_usb *dev,
> struct kvaser_usb_tx_urb_context *context;
> struct kvaser_usb_net_priv *priv;
> unsigned long irq_flags;
> + unsigned int len;
> bool one_shot_fail = false;
> + bool is_err_frame = false;
> u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
>
> priv = kvaser_usb_hydra_net_priv_from_cmd(dev, cmd);
> @@ -1131,24 +1134,28 @@ static void
> kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
> kvaser_usb_hydra_one_shot_fail(priv, cmd_ext);
> one_shot_fail = true;
> }
> - }
> -
> - context = &priv->tx_contexts[transid % dev->max_tx_urbs];
> - if (!one_shot_fail) {
> - struct net_device_stats *stats = &priv->netdev->stats;
> -
> - stats->tx_packets++;
> - stats->tx_bytes += can_fd_dlc2len(context->dlc);
> + if (flags & KVASER_USB_HYDRA_CF_FLAG_TX_ACK &&
> + flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME)
> + is_err_frame = true;
Nitpick, but I prefer to write:
+ is_err_frame = flags & KVASER_USB_HYDRA_CF_FLAG_TX_ACK &&
+ flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME;
> }
>
> spin_lock_irqsave(&priv->tx_contexts_lock, irq_flags);
>
> - can_get_echo_skb(priv->netdev, context->echo_index, NULL);
> + context = &priv->tx_contexts[transid % dev->max_tx_urbs];
> + len = can_get_echo_skb(priv->netdev, context->echo_index, NULL);
This line is related to the tx RTR. I will rebase this into
"can: do not increase rx_bytes statistics for RTR frames" (patch 4/5).
> +
> context->echo_index = dev->max_tx_urbs;
> --priv->active_tx_contexts;
> netif_wake_queue(priv->netdev);
>
> spin_unlock_irqrestore(&priv->tx_contexts_lock, irq_flags);
> +
> + if (!one_shot_fail && !is_err_frame) {
> + struct net_device_stats *stats = &priv->netdev->stats;
> +
> + stats->tx_packets++;
> + stats->tx_bytes += len;
> + }
Same here, there is no need anymore to move this block *in this
patch*, will rebase it.
> }
>
> static void kvaser_usb_hydra_rx_msg_std(const struct kvaser_usb *dev,
This patch ("can: kvaser_usb: do not increase tx statistics when
sending error message frames") will become:
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 3398da323126..75009d38f8e3 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -295,6 +295,7 @@ struct kvaser_cmd {
#define KVASER_USB_HYDRA_CF_FLAG_OVERRUN BIT(1)
#define KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME BIT(4)
#define KVASER_USB_HYDRA_CF_FLAG_EXTENDED_ID BIT(5)
+#define KVASER_USB_HYDRA_CF_FLAG_TX_ACK BIT(6)
/* CAN frame flags. Used in ext_rx_can and ext_tx_can */
#define KVASER_USB_HYDRA_CF_FLAG_OSM_NACK BIT(12)
#define KVASER_USB_HYDRA_CF_FLAG_ABL BIT(13)
@@ -1113,6 +1114,7 @@ static void
kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
struct kvaser_usb_net_priv *priv;
unsigned long irq_flags;
bool one_shot_fail = false;
+ bool is_err_frame = false;
u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
priv = kvaser_usb_hydra_net_priv_from_cmd(dev, cmd);
@@ -1131,10 +1133,13 @@ static void
kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
kvaser_usb_hydra_one_shot_fail(priv, cmd_ext);
one_shot_fail = true;
}
+
+ is_err_frame = flags & KVASER_USB_HYDRA_CF_FLAG_TX_ACK &&
+ flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME;
}
context = &priv->tx_contexts[transid % dev->max_tx_urbs];
- if (!one_shot_fail) {
+ if (!one_shot_fail && !is_err_frame) {
struct net_device_stats *stats = &priv->netdev->stats;
stats->tx_packets++;
And patch 5/5 ("can: do not increase tx_bytes
statistics for RTR frames") becomes:
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 75009d38f8e3..2cb35bd162a4 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -1113,6 +1113,7 @@ static void
kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
struct kvaser_usb_tx_urb_context *context;
struct kvaser_usb_net_priv *priv;
unsigned long irq_flags;
+ unsigned int len;
bool one_shot_fail = false;
bool is_err_frame = false;
u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
@@ -1139,21 +1140,23 @@ static void
kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
}
context = &priv->tx_contexts[transid % dev->max_tx_urbs];
- if (!one_shot_fail && !is_err_frame) {
- struct net_device_stats *stats = &priv->netdev->stats;
-
- stats->tx_packets++;
- stats->tx_bytes += can_fd_dlc2len(context->dlc);
- }
spin_lock_irqsave(&priv->tx_contexts_lock, irq_flags);
- can_get_echo_skb(priv->netdev, context->echo_index, NULL);
+ len = can_get_echo_skb(priv->netdev, context->echo_index, NULL);
context->echo_index = dev->max_tx_urbs;
--priv->active_tx_contexts;
netif_wake_queue(priv->netdev);
spin_unlock_irqrestore(&priv->tx_contexts_lock, irq_flags);
+
+ if (!one_shot_fail && !is_err_frame) {
+ struct net_device_stats *stats = &priv->netdev->stats;
+
+ stats->tx_packets++;
+ stats->tx_bytes += len;
+ }
}
Does this look good to you? If so, can I add these tags to patch 2/5?
Co-developed-by: Jimmy Assarsson <extja@...ser.com>
Signed-off-by: Jimmy Assarsson <extja@...ser.com>
Also, can I add your tested-by to patches 1/5, 4/5 and 5/5?
Tested-by: Jimmy Assarsson <extja@...ser.com>
Yours sincerely,
Vincent Mailhol
Powered by blists - more mailing lists