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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 24 Jul 2019 09:13:30 +0000
From:   Jeroen Hofstee <jhofstee@...tronenergy.com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>,
        "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

Hello Marc,

On 7/24/19 10:26 AM, Marc Kleine-Budde wrote:
> 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.
>

That is correct. For completeness the HECC_CANMOTS wasn't
even used in the original patch, so there is no functional difference.

Thanks,

Jeroen


Powered by blists - more mailing lists