| 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
| ||
|
Message-ID: <78bf099d-32ad-9981-6ba9-fe56fbcabc3e@grandegger.com>
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