| 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: <5aed4f5a-cc80-7ad9-2834-a31da53412a5@kvaser.com>
Date: Fri, 3 Dec 2021 09:45:28 +0100
From: Jimmy Assarsson <extja@...ser.com>
To: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
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 2021-12-03 05:02, Vincent MAILHOL wrote:
> 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;
>
Agree, I also prefer this.
>> }
>>
>> 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.
Agree.
>> }
>>
>> 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>
Yes.
> Also, can I add your tested-by to patches 1/5, 4/5 and 5/5?
> Tested-by: Jimmy Assarsson <extja@...ser.com>
Yes.
> Yours sincerely,
> Vincent Mailhol
Thanks!
Best regards,
jimmy
Powered by blists - more mailing lists