| 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: Tue, 25 Jan 2011 12:36:31 +0800
From: Bhupesh SHARMA <bhupesh.sharma@...com>
To: Wolfgang Grandegger <wg@...ndegger.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Socketcan-core@...ts.berlios.de" <Socketcan-core@...ts.berlios.de>,
David Miller <davem@...emloft.net>,
Marc Kleine-Budde <mkl@...gutronix.de>
Subject: RE: [PATCH net-next-2.6 v5 1/1] can: c_can: Added support for Bosch
C_CAN controller
Hi Wolfgang,
Thanks for your review.
Please see my comments inline:
> Hi Bhupesh,
>
> at a closer look, I realized one more issue, apart from some minor
> ones...
>
> On 01/20/2011 10:20 AM, Bhupesh Sharma wrote:
> > Bosch C_CAN controller is a full-CAN implementation which is
> compliant
> > to CAN protocol version 2.0 part A and B. Bosch C_CAN user manual can
> be
> > obtained from:
> > http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/
> > c_can/users_manual_c_can.pdf
> >
> > This patch adds the support for this controller.
> > The following are the design choices made while writing the
> controller
> > driver:
> > 1. Interface Register set IF1 has be used only in the current design.
> > 2. Out of the 32 Message objects available, 16 are kept aside for RX
> > purposes and the rest for TX purposes.
> > 3. NAPI implementation is such that both the TX and RX paths function
> > in polling mode.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...com>
> > ---
> > Changes since V4:
> > 1. Insured correct ISR implementation to allow shared IRQs.
> > 2. To ensure better understanding of message object numbers
> > and thierusage modified C_CAN_MSG_OBJ_RX_FIRST value from 0
>
> Typo!
Ok :)
> > to 1.
> > 3. Corrected coding style globally.
> > 4. Renamed Interface registers as *ifregs*.
> >
> > drivers/net/can/Kconfig | 2 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/c_can/Kconfig | 15 +
> > drivers/net/can/c_can/Makefile | 8 +
> > drivers/net/can/c_can/c_can.c | 958
> ++++++++++++++++++++++++++++++++
> > drivers/net/can/c_can/c_can.h | 229 ++++++++
> > drivers/net/can/c_can/c_can_platform.c | 207 +++++++
> > 7 files changed, 1420 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/net/can/c_can/Kconfig
> > create mode 100644 drivers/net/can/c_can/Makefile
> > create mode 100644 drivers/net/can/c_can/c_can.c
> > create mode 100644 drivers/net/can/c_can/c_can.h
> > create mode 100644 drivers/net/can/c_can/c_can_platform.c
> >
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index 9d9e453..50549b5 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -86,6 +86,8 @@ source "drivers/net/can/mscan/Kconfig"
> >
> > source "drivers/net/can/sja1000/Kconfig"
> >
> > +source "drivers/net/can/c_can/Kconfig"
> > +
> > source "drivers/net/can/usb/Kconfig"
> >
> > config CAN_DEBUG_DEVICES
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> > index 0057537..c3efeb3 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -11,6 +11,7 @@ obj-y += usb/
> >
> > obj-$(CONFIG_CAN_SJA1000) += sja1000/
> > obj-$(CONFIG_CAN_MSCAN) += mscan/
> > +obj-$(CONFIG_CAN_C_CAN) += c_can/
> > obj-$(CONFIG_CAN_AT91) += at91_can.o
> > obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> > obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
> > diff --git a/drivers/net/can/c_can/Kconfig
> b/drivers/net/can/c_can/Kconfig
> > new file mode 100644
> > index 0000000..ffb9773
> > --- /dev/null
> > +++ b/drivers/net/can/c_can/Kconfig
> > @@ -0,0 +1,15 @@
> > +menuconfig CAN_C_CAN
> > + tristate "Bosch C_CAN devices"
> > + depends on CAN_DEV && HAS_IOMEM
> > +
> > +if CAN_C_CAN
> > +
> > +config CAN_C_CAN_PLATFORM
> > + tristate "Generic Platform Bus based C_CAN driver"
> > + ---help---
> > + This driver adds support for the C_CAN chips connected to
> > + the "platform bus" (Linux abstraction for directly to the
> > + processor attached devices) which can be found on various
> > + boards from ST Microelectronics (http://www.st.com)
> > + like the SPEAr1310 and SPEAr320 evaluation boards.
> > +endif
> > diff --git a/drivers/net/can/c_can/Makefile
> b/drivers/net/can/c_can/Makefile
> > new file mode 100644
> > index 0000000..9273f6d
> > --- /dev/null
> > +++ b/drivers/net/can/c_can/Makefile
> > @@ -0,0 +1,8 @@
> > +#
> > +# Makefile for the Bosch C_CAN controller drivers.
> > +#
> > +
> > +obj-$(CONFIG_CAN_C_CAN) += c_can.o
> > +obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o
> > +
> > +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> > diff --git a/drivers/net/can/c_can/c_can.c
> b/drivers/net/can/c_can/c_can.c
> > new file mode 100644
> > index 0000000..66a400b
> > --- /dev/null
> > +++ b/drivers/net/can/c_can/c_can.c
>
> ...
>
> > +static void c_can_handle_lost_msg_obj(struct net_device *dev,
> > + int iface, int objno)
> > +{
> > + struct c_can_priv *priv = netdev_priv(dev);
> > + struct net_device_stats *stats = &dev->stats;
> > + struct sk_buff *skb;
> > + struct can_frame *frame;
> > +
> > + netdev_err(dev, "msg lost in buffer %d\n", objno);
> > +
> > + c_can_object_get(dev, iface, objno, IF_COMM_ALL &
> > + ~IF_COMM_TXRQST);
>
> Does fit on one line.
OK.
> ...
>
> > +static void c_can_inval_msg_object(struct net_device *dev, int
> iface, int objno)
> > +{
> > + struct c_can_priv *priv = netdev_priv(dev);
> > +
> > + priv->write_reg(priv, &priv->regs->ifregs[iface].arb1, 0);
> > + priv->write_reg(priv, &priv->regs->ifregs[iface].arb2, 0);
> > + priv->write_reg(priv, &priv->regs->ifregs[iface].msg_cntrl, 0);
> > +
> > + c_can_object_put(dev, iface, objno,
> > + IF_COMM_ARB | IF_COMM_CONTROL);
>
> Ditto
Ok.
> ...
>
> > +static inline int c_can_has_and_handle_berr(struct c_can_priv *priv)
> > +{
> > + return (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
> > + (priv->current_status & STATUS_LEC_MASK);
> > +}
> > +
> > +static int c_can_err(struct net_device *dev,
> > + enum c_can_bus_error_types error_type,
> > + enum c_can_lec_type lec_type)
> > +{
> > + unsigned int reg_err_counter;
> > + unsigned int rx_err_passive;
> > + struct c_can_priv *priv = netdev_priv(dev);
> > + struct net_device_stats *stats = &dev->stats;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct can_berr_counter bec;
> > +
> > + /* propogate the error condition to the CAN stack */
> > + skb = alloc_can_err_skb(dev, &cf);
> > + if (unlikely(!skb))
> > + return 0;
> > +
> > + c_can_get_berr_counter(dev, &bec);
> > + reg_err_counter = priv->read_reg(priv, &priv->regs->err_cnt);
> > + rx_err_passive = (reg_err_counter & ERR_CNT_RP_MASK) >>
> > + ERR_CNT_RP_SHIFT;
> > +
> > + if (error_type & C_CAN_ERROR_WARNING) {
> > + /* error warning state */
> > + priv->can.can_stats.error_warning++;
> > + priv->can.state = CAN_STATE_ERROR_WARNING;
> > + cf->can_id |= CAN_ERR_CRTL;
> > + if (bec.rxerr > 96)
> > + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
> > + if (bec.txerr > 96)
> > + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
> > + }
> > + if (error_type & C_CAN_ERROR_PASSIVE) {
> > + /* error passive state */
> > + priv->can.can_stats.error_passive++;
> > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > + cf->can_id |= CAN_ERR_CRTL;
> > + if (rx_err_passive)
> > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> > + if (bec.txerr > 127)
> > + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> > + }
> > + if (error_type & C_CAN_BUS_OFF) {
> > + /* bus-off state */
> > + priv->can.state = CAN_STATE_BUS_OFF;
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > + /*
> > + * disable all interrupts in bus-off mode to ensure that
> > + * the CPU is not hogged down
> > + */
> > + c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
> > + can_bus_off(dev);
> > + }
> > +
> > + /*
> > + * check for 'last error code' which tells us the
> > + * type of the last error to occur on the CAN bus
> > + */
> > +
> > + /* common for all type of bus errors */
> > + priv->can.can_stats.bus_error++;
>
> Does a state change always come together with a bus error?
>
> > + stats->rx_errors++;
> > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > +
> > + switch (lec_type) {
> > + case LEC_STUFF_ERROR:
> > + netdev_dbg(dev, "stuff error\n");
> > + cf->data[2] |= CAN_ERR_PROT_STUFF;
> > + break;
> > +
> > + case LEC_FORM_ERROR:
> > + netdev_dbg(dev, "form error\n");
> > + cf->data[2] |= CAN_ERR_PROT_FORM;
> > + break;
> > +
> > + case LEC_ACK_ERROR:
> > + netdev_dbg(dev, "ack error\n");
> > + cf->data[2] |= (CAN_ERR_PROT_LOC_ACK |
> > + CAN_ERR_PROT_LOC_ACK_DEL);
> > + break;
> > +
> > + case LEC_BIT1_ERROR:
> > + netdev_dbg(dev, "bit1 error\n");
> > + cf->data[2] |= CAN_ERR_PROT_BIT1;
> > + break;
> > +
> > + case LEC_BIT0_ERROR:
> > + netdev_dbg(dev, "bit0 error\n");
> > + cf->data[2] |= CAN_ERR_PROT_BIT0;
> > + break;
> > +
> > + case LEC_CRC_ERROR:
> > + netdev_dbg(dev, "CRC error\n");
> > + cf->data[2] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> > + CAN_ERR_PROT_LOC_CRC_DEL);
> > + break;
> > + }
>
> From the C_CAN manual:
>
> "The LEC field holds a code which indicates the type of the last error
> to occur on the CAN bus. This field will be cleared to '0' when a
> message has been transferred (reception or transmission) without error.
> The unused code '7' may be written by the CPU to check for updates."
>
> Not sure if it's necessary to reset the lec at init and after an error
> to 0x7 and check it. More below...
Hmm.. The default value of Status Register is 0x0 at INIT so I am not
sure if LEC needs to be reset at init. However using the unused code
0x7 seems necessary to check for updates as per specs.
I will modify the same in V6.
> > + netif_receive_skb(skb);
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +
> > + return 1;
> > +}
> > +
> > +static int c_can_poll(struct napi_struct *napi, int quota)
> > +{
> > + u16 irqstatus;
> > + int lec_type = 0;
> > + int work_done = 0;
> > + struct net_device *dev = napi->dev;
> > + struct c_can_priv *priv = netdev_priv(dev);
> > + enum c_can_bus_error_types error_type = C_CAN_NO_ERROR;
> > +
> > + irqstatus = priv->read_reg(priv, &priv->regs->interrupt);
> > + if (!irqstatus)
> > + goto end;
> > +
> > + /* status events have the highest priority */
> > + if (irqstatus == STATUS_INTERRUPT) {
> > + priv->current_status = priv->read_reg(priv,
> > + &priv->regs->status);
> > +
> > + /* handle Tx/Rx events */
> > + if (priv->current_status & STATUS_TXOK)
> > + priv->write_reg(priv, &priv->regs->status,
> > + priv->current_status & ~STATUS_TXOK);
> > +
> > + if (priv->current_status & STATUS_RXOK)
> > + priv->write_reg(priv, &priv->regs->status,
> > + priv->current_status & ~STATUS_RXOK);
> > +
> > + /* handle bus error events */
> > + if (priv->current_status & STATUS_EWARN) {
> > + netdev_dbg(dev,
> > + "entered error warning state\n");
>
> Does fit on one line.
OK.
> > + error_type = C_CAN_ERROR_WARNING;
> > + }
> > + if ((priv->current_status & STATUS_EPASS) &&
> > + (!(priv->last_status & STATUS_EPASS))) {
> > + netdev_dbg(dev,
> > + "entered error passive state\n");
>
> Ditto.
OK.
> > + error_type = C_CAN_ERROR_PASSIVE;
> > + }
> > + if ((priv->current_status & STATUS_BOFF) &&
> > + (!(priv->last_status & STATUS_BOFF))) {
> > + netdev_dbg(dev,
> > + "entered bus off state\n");
>
> Ditto.
OK.
> > + error_type = C_CAN_BUS_OFF;
> > + }
> > +
> > + /* handle bus recovery events */
> > + if ((!(priv->current_status & STATUS_EPASS)) &&
> > + (priv->last_status & STATUS_EPASS)) {
> > + netdev_dbg(dev,
> > + "left error passive state\n");
>
> Ditto.
OK.
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + }
> > + if ((!(priv->current_status & STATUS_BOFF)) &&
> > + (priv->last_status & STATUS_BOFF)) {
> > + netdev_dbg(dev,
> > + "left bus off state\n");
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + }
> > +
> > + priv->last_status = priv->current_status;
> > +
> > + /* handle error on the bus */
> > + lec_type = c_can_has_and_handle_berr(priv);
> > + if (lec_type && (error_type != C_CAN_NO_ERROR))
> > + work_done += c_can_err(dev, error_type, lec_type);
>
> State changes are only reported if berr_reporting is enabled and a bus
> error occured. This needs to be fixed.
As I mentioned earlier in a response to a review comment, the Bus Error
reporting for C_CAN seems different from sja1000 and flexcan approaches.
Do you think it will be useful to drop CAN_CTRLMODE_BERR_REPORTING from
priv->can.ctrlmode_supported as done by *pch* driver? Or do you have
a better idea..
> Feel free to send the output of "candump any,0:0,#FFFFFFFF" when
> sending
> messages without cable connected and with a bus error provocuted.
OK. I will try to cross-compile candump for my arm-v7 architecture
and will send the output.
> Apart form that the patch looks really good.
:)
> Wolfgang.
Regards,
Bhupesh
--
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