netdev - Re: [PATCH V3] 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]
Date:	Thu, 30 Jul 2009 11:27:30 +0200
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Wolfgang Grandegger <wg@...ndegger.com>
Cc:	Socketcan-core@...ts.berlios.de,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [PATCH V3] CAN: Add Flexcan CAN controller driver

On Thu, Jul 30, 2009 at 10:51:26AM +0200, Wolfgang Grandegger wrote:
> 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.

Read what I've written above. We get an interrupt on entering
error warning, but the bits reflecting the actual state only give error
active, error passive or bus off, but *not* error warning.

We could read the error counter and do something like

	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;
	}

	errcnt = readl(&regs->errcnt);
	rxerr = (errcnt >> 8) & 0xff;
	txerr = errcnt & 0xff;
	if ((rxerr >= 96 || txerr >= 96) && state == CAN_STATE_ERROR_ACTIVE)
		state = CAN_STATE_ERROR_WARNING;

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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