| 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:37:29 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Wolfgang Grandegger <wg@...ndegger.com>
Cc: Linux Netdev List <netdev@...r.kernel.org>,
Socketcan-core@...ts.berlios.de
Subject: Re: [PATCH V3] CAN: Add Flexcan CAN controller driver
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++;
}
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
Powered by blists - more mailing lists