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: Sun, 19 Mar 2017 17:17:37 +0100 From: Wolfgang Grandegger <wg@...ndegger.com> To: Akshay Bhat <akshay.bhat@...esys.com>, mkl@...gutronix.de Cc: linux-can@...r.kernel.org, netdev@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Akshay Bhat <nodeax@...il.com> Subject: Re: [PATCH v4 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver Hello Akshay, I still see some improvements... Am 17.03.2017 um 22:10 schrieb Akshay Bhat: > This patch adds support for the Holt HI-311x CAN controller. The HI311x > CAN controller is capable of transmitting and receiving standard data > frames, extended data frames and remote frames. The HI311x interfaces > with the host over SPI. > > Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do > > Signed-off-by: Akshay Bhat <nodeax@...il.com> > --- > > v3 -> v4: > Address comments from Wolfgang Grandegger: > - Add support for CAN warning state reporting > - Add support for reporting tx/rx error counts in bus error messages > - Keep bus error interrupts enabled to detect CAN state changes > - Fix bug in restart code after BUSOFF state > - Clean up error handling code > > drivers/net/can/spi/Kconfig | 6 + > drivers/net/can/spi/Makefile | 1 + > drivers/net/can/spi/hi311x.c | 1072 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 1079 insertions(+) > create mode 100644 drivers/net/can/spi/hi311x.c > > diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig > index 148cae5..8f2e0dd 100644 > --- a/drivers/net/can/spi/Kconfig > +++ b/drivers/net/can/spi/Kconfig > @@ -1,6 +1,12 @@ > menu "CAN SPI interfaces" > depends on SPI > > +config CAN_HI311X > + tristate "Holt HI311x SPI CAN controllers" > + depends on CAN_DEV && SPI && HAS_DMA > + ---help--- > + Driver for the Holt HI311x SPI CAN controllers. > + > config CAN_MCP251X > tristate "Microchip MCP251x SPI CAN controllers" > depends on HAS_DMA > diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile > index 0e86040..f59fa37 100644 > --- a/drivers/net/can/spi/Makefile > +++ b/drivers/net/can/spi/Makefile > @@ -3,4 +3,5 @@ > # > > > +obj-$(CONFIG_CAN_HI311X) += hi311x.o > obj-$(CONFIG_CAN_MCP251X) += mcp251x.o > diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c > new file mode 100644 > index 0000000..2a3d794 > --- /dev/null > +++ b/drivers/net/can/spi/hi311x.c > @@ -0,0 +1,1072 @@ > +/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface > + * > + * Copyright(C) Timesys Corporation 2016 > + * > + * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver > + * Copyright 2009 Christian Pellegrin EVOL S.r.l. > + * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved. > + * Copyright 2006 Arcom Control Systems Ltd. > + * > + * Based on CAN bus driver for the CCAN controller written by > + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix > + * - Simon Kallweit, intefo AG > + * Copyright 2007 > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ ... snip... > + > +static irqreturn_t hi3110_can_ist(int irq, void *dev_id) > +{ > + struct hi3110_priv *priv = dev_id; > + struct spi_device *spi = priv->spi; > + struct net_device *net = priv->net; > + > + mutex_lock(&priv->hi3110_lock); > + > + while (!priv->force_quit) { > + enum can_state new_state; > + u8 intf, eflag, statf; > + > + while (!(HI3110_STAT_RXFMTY & > + (statf = hi3110_read(spi, HI3110_READ_STATF)))) { > + hi3110_hw_rx(spi); > + } > + > + intf = hi3110_read(spi, HI3110_READ_INTF); I think HI3110_READ_ERR is only valid if HI3110_INT_BUSERR is set! Therefore: if ((intf & HI3110_INT_BUSERR) { It saves reading one SPI register in the message processing path. Please check if back-to-error-active still comes. > + eflag = hi3110_read(spi, HI3110_READ_ERR); > + /* Update can state */ > + if (eflag & HI3110_ERR_BUSOFF) > + new_state = CAN_STATE_BUS_OFF; > + else if (eflag & HI3110_ERR_PASSIVE_MASK) > + new_state = CAN_STATE_ERROR_PASSIVE; > + else if (statf & HI3110_STAT_ERRW) > + new_state = CAN_STATE_ERROR_WARNING; > + else > + new_state = CAN_STATE_ERROR_ACTIVE; > + > + if (new_state != priv->can.state) { > + struct can_frame *cf; > + struct sk_buff *skb; > + enum can_state rx_state, tx_state; > + u8 rxerr, txerr; > + > + skb = alloc_can_err_skb(net, &cf); > + if (!skb) > + break; > + > + txerr = hi3110_read(spi, HI3110_READ_TEC); > + rxerr = hi3110_read(spi, HI3110_READ_REC); > + cf->data[6] = txerr; > + cf->data[7] = rxerr; > + tx_state = txerr >= rxerr ? new_state : 0; > + rx_state = txerr <= rxerr ? new_state : 0; > + can_change_state(net, cf, tx_state, rx_state); > + netif_rx_ni(skb); > + > + if (new_state == CAN_STATE_BUS_OFF) { > + can_bus_off(net); > + if (priv->can.restart_ms == 0) { > + priv->force_quit = 1; > + hi3110_hw_sleep(spi); > + break; > + } > + } > + } > + > + /* Update bus errors */ > + if ((intf & HI3110_INT_BUSERR) && > + (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) { > + struct can_frame *cf; > + struct sk_buff *skb; > + > + /* Check for protocol errors */ > + if (eflag & HI3110_ERR_PROTOCOL_MASK) { > + skb = alloc_can_err_skb(net, &cf); > + if (!skb) > + break; > + > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > + priv->can.can_stats.bus_error++; > + priv->net->stats.rx_errors++; > + if (eflag & HI3110_ERR_BITERR) > + cf->data[2] |= CAN_ERR_PROT_BIT; > + else if (eflag & HI3110_ERR_FRMERR) > + cf->data[2] |= CAN_ERR_PROT_FORM; > + else if (eflag & HI3110_ERR_STUFERR) > + cf->data[2] |= CAN_ERR_PROT_STUFF; > + else if (eflag & HI3110_ERR_CRCERR) > + cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ; > + else if (eflag & HI3110_ERR_ACKERR) > + cf->data[3] |= CAN_ERR_PROT_LOC_ACK; > + > + cf->data[6] = hi3110_read(spi, HI3110_READ_TEC); > + cf->data[7] = hi3110_read(spi, HI3110_READ_REC); > + netdev_dbg(priv->net, "Bus Error\n"); > + netif_rx_ni(skb); > + } > + } > + > + if (intf == 0) > + break; I do not see a real benefit of the check above. Just more code. > + if (intf & HI3110_INT_TXCPLT) { > + net->stats.tx_packets++; > + net->stats.tx_bytes += priv->tx_len - 1; > + can_led_event(net, CAN_LED_EVENT_TX); > + if (priv->tx_len) { > + can_get_echo_skb(net, 0); > + priv->tx_len = 0; > + } > + netif_wake_queue(net); > + } > + } > + mutex_unlock(&priv->hi3110_lock); > + return IRQ_HANDLED; > +} Apart from that, it looks good now and you can add my: Acked-by: Wolfgang Grandegger <wg@...ndegger.com> Thanks, Wolfgang.
Powered by blists - more mailing lists