netdev - Re: [PATCH] CAN: Add Flexcan CAN controller driver

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4C3EE12A.1030307@grandegger.com>
Date:	Thu, 15 Jul 2010 12:21:30 +0200
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>
CC:	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org
Subject: Re: [PATCH] CAN: Add Flexcan CAN controller driver

On 07/15/2010 12:06 PM, Marc Kleine-Budde wrote:
> Hey Wolfgang,
> 
> Wolfgang Grandegger wrote:
>>> This core is found on some Freescale SoCs and also some Coldfire
>>> SoCs. Support for Coldfire is missing though at the moment as
>>> they have an older revision of the core which does not have RX FIFO
>>> support.
>>>
>>> Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
>>> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
>>> ---
>>>
>>> Changes to prev version:
>>> * The is now GPLv2 (only) as no one complained.
>>>
>>> The patch applies to current net-next-2.6/master.
>>> If there aren't any objections please consider applying this patch.
>>> Wolfgang, can I an Acked-by?
>>
>> I already had a look to the previous version and I realized that
>> accessing reg_esr is racy. I will dig out my notes and come up with a
>> full review later today or tomorrow.
> 
> Let me have a look....
> 
> I think I should remove the read reg_esr in "flexcan_irq_err()" and use
> the value read in "flexcan_irq()"
> 
> Without the fix, there's a race window that we loose some error bits
> from the interrupt handler to the napi function, because the error bits
> are cleared on read.

Right, that is my concern. reg_esr should only be read and handled
*once*. Currently it's read *3* times! I would read it in the ISR and
handle it in the NAPI poll function, including state change
notification. But I need a closer look...

> Regarding the upcoming improvement of the can error frames upon state
> changes, I'd first like to get this driver merged and then discuss a
> solution for the error frames.

Fine for me. I'm currently preparing a RFC patch for SJA1000 and MSCAN.

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html