| 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
| ||
|
Date: Thu, 30 Jul 2009 10:51:26 +0200
From: Wolfgang Grandegger <wg@...ndegger.com>
To: Sascha Hauer <s.hauer@...gutronix.de>
CC: Socketcan-core@...ts.berlios.de,
Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [PATCH V3] CAN: Add Flexcan CAN controller driver
Sascha Hauer wrote:
> On Wed, Jul 29, 2009 at 11:09:14AM +0200, Wolfgang Grandegger wrote:
>> Sascha Hauer wrote:
>>> And another go...
>>>
>>> Sascha
>>>
>>>
>>> >From 9f6f6fb67d57d082e056dec8121f1423c1b4fa0b Mon Sep 17 00:00:00 2001
>>> From: Sascha Hauer <s.hauer@...gutronix.de>
>>> Date: Tue, 21 Jul 2009 10:47:19 +0200
>>> Subject: [PATCH] CAN: Add Flexcan CAN driver
>>>
>>> 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.
>>>
>>> V3: integrated comments from Oliver Hartkopp
>>> V2: integrated comments from Wolfgang Grandegger
>>>
>>> Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
>>> ---
>>> drivers/net/can/Kconfig | 6 +
>>> drivers/net/can/Makefile | 1 +
>>> drivers/net/can/flexcan.c | 719 +++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 726 insertions(+), 0 deletions(-)
>>> create mode 100644 drivers/net/can/flexcan.c
>>>
>>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>>> index 33821a8..99c6da4 100644
>>> --- a/drivers/net/can/Kconfig
>>> +++ b/drivers/net/can/Kconfig
>>> @@ -74,6 +74,12 @@ config CAN_KVASER_PCI
>>> This driver is for the the PCIcanx and PCIcan cards (1, 2 or
>>> 4 channel) from Kvaser (http://www.kvaser.com).
>>>
>>> +config CAN_FLEXCAN
>>> + tristate "Support for Freescale FLEXCAN based chips"
>>> + depends on CAN_DEV
>>> + ---help---
>>> + Say Y here if you want to support for Freescale FlexCAN.
>>> +
>>> config CAN_DEBUG_DEVICES
>>> bool "CAN devices debugging messages"
>>> depends on CAN
>>> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
>>> index 523a941..25f2032 100644
>>> --- a/drivers/net/can/Makefile
>>> +++ b/drivers/net/can/Makefile
>>> @@ -8,5 +8,6 @@ obj-$(CONFIG_CAN_DEV) += can-dev.o
>>> can-dev-y := dev.o
>>>
>>> obj-$(CONFIG_CAN_SJA1000) += sja1000/
>>> +obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o
>>>
>>> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> new file mode 100644
>>> index 0000000..77661b3
>>> --- /dev/null
>>> +++ b/drivers/net/can/flexcan.c
>> [...]
>>> +static void flexcan_error(struct net_device *ndev, u32 stat)
>>> +{
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + struct flexcan_priv *priv = netdev_priv(ndev);
>>> + struct net_device_stats *stats = &ndev->stats;
>>> + enum can_state state = priv->can.state;
>>> + int error_warning = 0, rx_errors = 0, tx_errors = 0;
>>> +
>>> + skb = dev_alloc_skb(sizeof(struct can_frame));
>>> + if (!skb)
>>> + return;
>>> +
>>> + skb->dev = ndev;
>>> + skb->protocol = __constant_htons(ETH_P_CAN);
>>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> +
>>> + cf = (struct can_frame *)skb_put(skb, sizeof(*cf));
>>> + memset(cf, 0, sizeof(*cf));
>>> +
>>> + cf->can_id = CAN_ERR_FLAG;
>>> + cf->can_dlc = CAN_ERR_DLC;
>>> +
>>> + if (stat & ERRSTAT_RWRNINT) {
>>> + error_warning = 1;
>>> + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
>>> + }
>>> +
>>> + if (stat & ERRSTAT_TWRNINT) {
>>> + error_warning = 1;
>>> + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
>>> + }
>> What is the meaning of this error warning interrupt? It does *not*
>> change the state.
>
> This TWRNINT means that the Tx error counter has transitioned from < 96
> to >= 96 (analog the RWRNINT for the Rx error counter). Unfortunately
> the state bits handled below do not reflect this error warning state.
>
> So this interrupt signals us that we have been in error warning state
> once but we have no possibility to detect if we are still in error
> warning state. I don't know how to handle this correctly. I could
> disable this interrupt and ignore it completely.
>
>>> + switch ((stat >> 4) & 0x3) {
>>> + case 0:
>>> + state = CAN_STATE_ERROR_ACTIVE;
>>> + break;
>>> + case 1:
>>> + state = CAN_STATE_ERROR_PASSIVE;
>>> + break;
>>> + default:
>>> + state = CAN_STATE_BUS_OFF;
>>> + break;
>>> + }
>> I'm still not happy with the error message generation. If a state change
>> to error passive happens, it should be signaled to the user. Here my ideas:
>>
>> if (state != priv->can.state) {
>> if (state == CAN_STATE_ERROR_WARNING) {
>> cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
>> priv->can.can_stats.error_warning++;
>> } else if (state == CAN_STATE_ERROR_PASSIVE) {
>> cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
>> priv->can.can_stats.error_passive++;
>> }
>>
>> It might have missed something, though.
>
> Sorry, I missed that from your previous mail. As we can't properly
> detect ERROR_WARNING state this would reduce to:
>
> if (state != priv->can.state && state == CAN_STATE_ERROR_WARNING) {
> cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
> priv->can.can_stats.error_warning++;
> }
Why you can't detect error warning? When the [RT]WRNINT comes, the error
waning state has entered. By looking to the manual, the state changes to
error warning and bus-off are directly signaled via interrupt. A change
to error passive might be visible when bus error ints occurs. You could
add some dev_dbg's to the "if (stat & ERRSTAT_[RT]WRNINT)" and "switch"
blocks above and check with sending a message with no cable connected.
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
Powered by blists - more mailing lists