| 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, 10 Dec 2009 11:35:55 +0100
From: Wolfgang Grandegger <wg@...ndegger.com>
To: Barry Song <21cnbao@...il.com>
CC: socketcan-core@...ts.berlios.de,
uclinux-dist-devel@...ckfin.uclinux.org, davem@...emloft.net,
"H.J. Oertel" <oe@...t.de>, netdev@...r.kernel.org
Subject: Re: [PATCH v3] add the driver for Analog Devices Blackfin on-chip
CAN controllers
Barry Song wrote:
> On Thu, Dec 10, 2009 at 5:11 PM, Wolfgang Grandegger <wg@...ndegger.com> wrote:
>> Barry Song wrote:
>>> Signed-off-by: Barry Song <21cnbao@...il.com>
>>> Signed-off-by: H.J. Oertel <oe@...t.de>
>>> ---
>>> -v3: use structures to describe the layout of registers
>> Wow, that was quick, thanks for your effort and patience.
>>
>> Please use checkpath.pl to detect the obvious coding style problems,
>> especially the "WARNING: line over 80 characters".
>>
>> I also think the declaration of reg should have the __iomem as well:
>>
>> struct bfin_can_regs __iomem *reg = priv->membase;
>>
>>> drivers/net/can/Kconfig | 9 +
>>> drivers/net/can/Makefile | 1 +
>>> drivers/net/can/bfin_can.c | 836 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 846 insertions(+), 0 deletions(-)
>>> create mode 100644 drivers/net/can/bfin_can.c
>>>
[snip]
>>> diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c
>>> new file mode 100644
>>> index 0000000..b94169d
>>> --- /dev/null
>>> +++ b/drivers/net/can/bfin_can.c
>>> @@ -0,0 +1,836 @@
>>> +/*
>>> + * Blackfin On-Chip CAN Driver
>>> + *
>>> + * Copyright 2004-2009 Analog Devices Inc.
>> Consider adding your copyright here, with a name and address.
>>
>>> + *
>>> + * Enter bugs at http://blackfin.uclinux.org/
>>> + *
>>> + * Licensed under the GPL-2 or later.
>> Please add the usual "GPL-2 or later" bla-bla here.
> Here I am not completely sure. But I am sure I am using the head file
> template of ADI which has been used widely in kernel and should be
> right.
Well, at least it's not the usual bla bla. Maybe somebody else could
clarify that. David?
[snip]
>>> +static void bfin_can_set_reset_mode(struct net_device *dev)
>>> +{
>>> + struct bfin_can_priv *priv = netdev_priv(dev);
>>> + struct bfin_can_regs *reg = priv->membase;
>>> + int timeout = BFIN_CAN_TIMEOUT;
>>> + int i;
>>> +
>>> + /* disable interrupts */
>>> + writew(0, ®->mbim1);
>>> + writew(0, ®->mbim2);
>>> + writew(0, ®->gim);
>>> +
>>> + /* reset can and enter configuration mode */
>>> + writew(SRS | CCR, ®->ctrl);
>>> + SSYNC();
>>> + writew(CCR, ®->ctrl);
>>> + SSYNC();
>>> + while (!(readw(®->ctrl) & CCA)) {
>>> + udelay(10);
>>> + if (--timeout == 0) {
>>> + dev_err(dev->dev.parent, "fail to enter configuration mode\n");
>>> + BUG();
>>> + }
>>> + }
>> Isn't using BUG() to harsh here? Using and handling a proper return code
>> might already be sufficient.
> Due to the hardware design, here timeout will never happen. If it
> happens, that means hardware component has crashed.
OK.
[snip]
>>> +static int bfin_can_err(struct net_device *dev, u16 isrc, u16 status)
>>> +{
>>> + struct bfin_can_priv *priv = netdev_priv(dev);
>>> + struct bfin_can_regs *reg = priv->membase;
>>> + struct net_device_stats *stats = &dev->stats;
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + enum can_state state = priv->can.state;
>>> +
>>> + skb = alloc_can_err_skb(dev, &cf);
>>> + if (skb == NULL)
>>> + return -ENOMEM;
>>> +
>>> + if (isrc & RMLIS) {
>>> + /* data overrun interrupt */
>>> + dev_dbg(dev->dev.parent, "data overrun interrupt\n");
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>>> + stats->rx_over_errors++;
>>> + stats->rx_errors++;
>>> + }
>>> +
>>> + if (isrc & BOIS) {
>>> + dev_dbg(dev->dev.parent, "bus-off mode interrupt\n");
>>> +
>> Empty line?
>>
>>> + state = CAN_STATE_BUS_OFF;
>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>> + can_bus_off(dev);
>>> + }
>>> +
>>> + if (isrc & EPIS) {
>>> + /* error passive interrupt */
>>> + dev_dbg(dev->dev.parent, "error passive interrupt\n");
>>> + state = CAN_STATE_ERROR_PASSIVE;
>>> + }
>>> +
>>> + if ((isrc & EWTIS) || (isrc & EWRIS)) {
>>> + dev_dbg(dev->dev.parent, "Error Warning Transmit/Receive Interrupt\n");
>>> + state = CAN_STATE_ERROR_WARNING;
>>> + }
>>> +
>>> + if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
>>> + state == CAN_STATE_ERROR_PASSIVE)) {
>>> + u16 cec = readw(®->cec);
>>> + u8 rxerr = cec;
>>> + u8 txerr = cec >> 8;
>> Add an empty line here, please.
>>
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + if (state == CAN_STATE_ERROR_WARNING) {
>>> + priv->can.can_stats.error_warning++;
>>> + cf->data[1] = (txerr > rxerr) ?
>>> + CAN_ERR_CRTL_TX_WARNING :
>>> + CAN_ERR_CRTL_RX_WARNING;
>>> + } else {
>>> + priv->can.can_stats.error_passive++;
>>> + cf->data[1] = (txerr > rxerr) ?
>>> + CAN_ERR_CRTL_TX_PASSIVE :
>>> + CAN_ERR_CRTL_RX_PASSIVE;
>>> + }
>>> + }
>>> +
>>> + if (status) {
>>> + priv->can.can_stats.bus_error++;
>>> +
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> +
>>> + if (status & BEF)
>>> + cf->data[2] |= CAN_ERR_PROT_BIT;
>>> + else if (status & FER)
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>> + else if (status & SER)
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> + else
>>> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
>>> + }
>> Add {} here as well.
> {} will cause checkpatch warning if it is given to only one line.
Of course, my fault. Forget it.
>>> + priv->can.state = state;
>>> +
>>> + netif_rx(skb);
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +irqreturn_t bfin_can_interrupt(int irq, void *dev_id)
>>> +{
>>> + struct net_device *dev = dev_id;
>>> + struct bfin_can_priv *priv = netdev_priv(dev);
>>> + struct bfin_can_regs *reg = priv->membase;
>>> + struct net_device_stats *stats = &dev->stats;
>>> + u16 status, isrc;
>>> +
>>> + if ((irq == priv->tx_irq) && readw(®->mbtif2)) {
>>> + /* transmission complete interrupt */
>>> + writew(0xFFFF, ®->mbtif2);
>>> + stats->tx_packets++;
>>> + stats->tx_bytes += bfin_can_read_dlc(priv, TRANSMIT_CHL);
>>> + can_get_echo_skb(dev, 0);
>>> + netif_wake_queue(dev);
>>> + } else if ((irq == priv->rx_irq) && readw(®->mbrif1)) {
>>> + /* receive interrupt */
>>> + isrc = readw(®->mbrif1);
>>> + writew(0xFFFF, ®->mbrif1);
>>> + bfin_can_rx(dev, isrc);
>>> + } else if ((irq == priv->err_irq) && readw(®->gis)) {
>>> + /* error interrupt */
>>> + isrc = readw(®->gis);
>>> + status = readw(®->esr);
>>> + writew(0x7FF, ®->gis);
>>> + bfin_can_err(dev, isrc, status);
>>> + } else
>>> + return IRQ_NONE;
>> Use {} here as well.
> {} will cause checkpatch warning if it is given to only one line.
But here it should be added as explained here:
http://lxr.linux.no/#linux+v2.6.32/Documentation/CodingStyle#L171
Does checkpatch really complain?
[snip]
>>> +#ifdef CONFIG_PM
>>> +static int bfin_can_suspend(struct platform_device *pdev, pm_message_t mesg)
>>> +{
>>> + struct net_device *dev = dev_get_drvdata(&pdev->dev);
>>> + struct bfin_can_priv *priv = netdev_priv(dev);
>>> + struct bfin_can_regs *reg = priv->membase;
>>> + int timeout = BFIN_CAN_TIMEOUT;
>>> +
>>> + if (netif_running(dev)) {
>>> + /* enter sleep mode */
>>> + writew(readw(®->ctrl) | SMR, ®->ctrl);
>>> + SSYNC();
>>> + while (!(readw(®->intr) & SMACK)) {
>>> + udelay(10);
>>> + if (--timeout == 0) {
>>> + dev_err(dev->dev.parent, "fail to enter sleep mode\n");
>>> + BUG();
>>> + }
>>> + }
>> It's already the third time that this timeout check is used. A common
>> function would make sense.
> Every time, the check condition is different and print information
> will change. It is ugly to define only one function.
OK, I obviously missed that. Sorry for the noise.
Wolfgang.
--
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