| 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: <5881cb80-883b-a96b-2939-973150cfc196@pengutronix.de>
Date: Wed, 24 Jul 2019 10:26:02 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Jeroen Hofstee <jhofstee@...tronenergy.com>,
"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>
Cc: Anant Gole <anantgole@...com>, AnilKumar Ch <anilkumar@...com>,
Wolfgang Grandegger <wg@...ndegger.com>,
"David S. Miller" <davem@...emloft.net>,
"open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading
On 4/29/19 2:03 PM, Jeroen Hofstee wrote:
> As already mentioned in [1] and included in [2], there is an off by one
> issue since the high bank is already enabled when the _next_ mailbox to
> be read has index 12, so the mailbox being read was 13. The message can
> therefore go into mailbox 31 and the driver will be repolled until the
> mailbox 12 eventually receives a msg. Or the message might end up in the
> 12th mailbox, but then it would become disabled after reading it and only
> be enabled again in the next "round" after mailbox 13 was read, which can
> cause out of order messages, since the lower priority mailboxes can
> accept messages in the meantime.
>
> As mentioned in [3] there is a hardware race condition when changing the
> CANME register while messages are being received. Even when including a
> busy poll on reception, like in [2] there are still overflows and out of
> order messages at times, but less then without the busy loop polling.
> Unlike what the patch suggests, the polling time is not in the microsecond
> range, but takes as long as a current CAN bus reception needs to finish,
> so typically more in the fraction of millisecond range. Since the timeout
> is in jiffies it won't timeout.
>
> Even with these additional fixes the driver is still not able to provide a
> proper FIFO which doesn't drop packages. So change the driver to use
> rx-offload and base order on timestamp instead of message box numbers. As
> a side affect, this also fixes [4] and [5].
>
> Before this change messages with a single byte counter were dropped /
> received out of order at a bitrate of 250kbit/s on an am3517. With this
> patch that no longer occurs up to and including 1Mbit/s.
>
> [1] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6
> [2] http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2
> [3] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5
> [4] https://patchwork.ozlabs.org/patch/895956/
> [5] https://www.spinics.net/lists/netdev/msg494971.html
>
> Cc: Anant Gole <anantgole@...com>
> Cc: AnilKumar Ch <anilkumar@...com>
> Signed-off-by: Jeroen Hofstee <jhofstee@...tronenergy.com>
> ---
> drivers/net/can/ti_hecc.c | 189 +++++++++++++---------------------------------
> 1 file changed, 53 insertions(+), 136 deletions(-)
>
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> index db6ea93..fe7ffff 100644
> --- a/drivers/net/can/ti_hecc.c
> +++ b/drivers/net/can/ti_hecc.c
> @@ -5,6 +5,7 @@
> * specs for the same is available at <http://www.ti.com>
> *
> * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
> + * Copyright (C) 2019 Jeroen Hofstee <jhofstee@...tronenergy.com>
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License as
> @@ -34,6 +35,7 @@
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> #include <linux/can/led.h>
> +#include <linux/can/rx-offload.h>
>
> #define DRV_NAME "ti_hecc"
> #define HECC_MODULE_VERSION "0.7"
> @@ -63,29 +65,16 @@ MODULE_VERSION(HECC_MODULE_VERSION);
> #define HECC_TX_PRIO_MASK (MAX_TX_PRIO << HECC_MB_TX_SHIFT)
> #define HECC_TX_MB_MASK (HECC_MAX_TX_MBOX - 1)
> #define HECC_TX_MASK ((HECC_MAX_TX_MBOX - 1) | HECC_TX_PRIO_MASK)
> -#define HECC_TX_MBOX_MASK (~(BIT(HECC_MAX_TX_MBOX) - 1))
> -#define HECC_DEF_NAPI_WEIGHT HECC_MAX_RX_MBOX
>
> /*
> - * Important Note: RX mailbox configuration
> - * RX mailboxes are further logically split into two - main and buffer
> - * mailboxes. The goal is to get all packets into main mailboxes as
> - * driven by mailbox number and receive priority (higher to lower) and
> - * buffer mailboxes are used to receive pkts while main mailboxes are being
> - * processed. This ensures in-order packet reception.
> - *
> - * Here are the recommended values for buffer mailbox. Note that RX mailboxes
> - * start after TX mailboxes:
> - *
> - * HECC_MAX_RX_MBOX HECC_RX_BUFFER_MBOX No of buffer mailboxes
> - * 28 12 8
> - * 16 20 4
> + * RX mailbox configuration
> + * The remaining mailboxes are used for reception and are delivered based on
> + * their timestamp, to avoid a hardware race when CANME is changed while
> + * CAN-bus traffix is being received.
> */
>
> #define HECC_MAX_RX_MBOX (HECC_MAX_MAILBOXES - HECC_MAX_TX_MBOX)
> -#define HECC_RX_BUFFER_MBOX 12 /* as per table above */
> #define HECC_RX_FIRST_MBOX (HECC_MAX_MAILBOXES - 1)
> -#define HECC_RX_HIGH_MBOX_MASK (~(BIT(HECC_RX_BUFFER_MBOX) - 1))
>
> /* TI HECC module registers */
> #define HECC_CANME 0x0 /* Mailbox enable */
> @@ -123,6 +112,8 @@ MODULE_VERSION(HECC_MODULE_VERSION);
> #define HECC_CANMDL 0x8
> #define HECC_CANMDH 0xC
>
> +#define HECC_CANMOTS 0x100
It's actually 0x80
> +
> #define HECC_SET_REG 0xFFFFFFFF
> #define HECC_CANID_MASK 0x3FF /* 18 bits mask for extended id's */
> #define HECC_CCE_WAIT_COUNT 100 /* Wait for ~1 sec for CCE bit */
> @@ -193,7 +184,7 @@ static const struct can_bittiming_const ti_hecc_bittiming_const = {
>
> struct ti_hecc_priv {
> struct can_priv can; /* MUST be first member/field */
> - struct napi_struct napi;
> + struct can_rx_offload offload;
> struct net_device *ndev;
> struct clk *clk;
> void __iomem *base;
> @@ -203,7 +194,6 @@ struct ti_hecc_priv {
> spinlock_t mbx_lock; /* CANME register needs protection */
> u32 tx_head;
> u32 tx_tail;
> - u32 rx_next;
> struct regulator *reg_xceiver;
> };
>
> @@ -265,6 +255,11 @@ static inline u32 hecc_get_bit(struct ti_hecc_priv *priv, int reg, u32 bit_mask)
> return (hecc_read(priv, reg) & bit_mask) ? 1 : 0;
> }
>
> +static inline u32 hecc_read_stamp(struct ti_hecc_priv *priv, u32 mbxno)
> +{
> + return __raw_readl(priv->hecc_ram + 0x80 + 4 * mbxno);
I've changed this function to use HECC_CANMOTS.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists