lkml.org 
[lkml]   [2009]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/8] can: Driver for the SJA1000 CAN controller
I won't be able to look at all of these...

> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index d609895..78a412b 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -35,6 +35,17 @@ config CAN_CALC_BITTIMING
> files "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
> If unsure, say Y.
>
> +config CAN_SJA1000
> + depends on CAN_DEV
> + tristate "Philips SJA1000"
> + ---help---
> + The SJA1000 is one of the top CAN controllers out there. As it
> + has a multiplexed interface it fits directly to 8051
> + microcontrollers or into the PC I/O port space. The SJA1000
> + is a full CAN controller, with shadow registers for RX and TX.
> + It can send and receive any kinds of CAN frames (SFF/EFF/RTR)
> + with a single (simple) filter setup.

This sounds more like advertising text. But what people need to know is
whether they should enable it or not.

[...]

> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> new file mode 100644
> index 0000000..6fe516d
> --- /dev/null
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -0,0 +1,681 @@

[...]

> +static void sja1000_start(struct net_device *dev)
> +{
> + struct sja1000_priv *priv = netdev_priv(dev);
> +
> + /* leave reset mode */
> + if (priv->can.state != CAN_STATE_STOPPED)
> + set_reset_mode(dev);
> +
> + /* Clear error counters and error code capture */
> + priv->write_reg(dev, REG_TXERR, 0x0);
> + priv->write_reg(dev, REG_RXERR, 0x0);
> + priv->read_reg(dev, REG_ECC);
> +
> + /* leave reset mode */
> + set_normal_mode(dev);
> +}

It's about here that I begin to wonder about locking again. What is
preventing concurrent access to the device?

[...]

> +static int sja1000_get_state(struct net_device *dev, enum can_state *state)
> +{
> + struct sja1000_priv *priv = netdev_priv(dev);
> + u8 status;
> +
> + /* FIXME: inspecting the status register to get the current state
> + * is not really necessary, because state changes are handled by
> + * in the ISR and the variable priv->can.state gets updated. The
> + * CAN devicde interface needs fixing!
> + */
> +
> + spin_lock_irq(&priv->can.irq_lock);

Interesting, here we do have a lock. What is it protecting? *state?? It
can't be the device registers, since they are accessed without locks in
many other places.

> + if (priv->can.state == CAN_STATE_STOPPED) {
> + *state = CAN_STATE_STOPPED;
> + } else {
> + status = priv->read_reg(dev, REG_SR);
> + if (status & SR_BS)
> + *state = CAN_STATE_BUS_OFF;
> + else if (status & SR_ES) {
> + if (priv->read_reg(dev, REG_TXERR) > 127 ||
> + priv->read_reg(dev, REG_RXERR) > 127)
> + *state = CAN_STATE_BUS_PASSIVE;
> + else
> + *state = CAN_STATE_BUS_WARNING;
> + } else
> + *state = CAN_STATE_ACTIVE;
> + }
> + /* Check state */
> + if (*state != priv->can.state)
> + dev_err(ND2D(dev),
> + "Oops, state mismatch: hard %d != soft %d\n",
> + *state, priv->can.state);
> + spin_unlock_irq(&priv->can.irq_lock);
> + return 0;
> +}

[...]

> +/*
> + * transmit a CAN message
> + * message layout in the sk_buff should be like this:
> + * xx xx xx xx ff ll 00 11 22 33 44 55 66 77
> + * [ can-id ] [flags] [len] [can data (up to 8 bytes]
> + */
> +static int sja1000_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct sja1000_priv *priv = netdev_priv(dev);
> + struct net_device_stats *stats = &dev->stats;
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + uint8_t fi;
> + uint8_t dlc;
> + canid_t id;
> + uint8_t dreg;
> + int i;
> +
> + netif_stop_queue(dev);
> +
> + fi = dlc = cf->can_dlc;
> + id = cf->can_id;
> +
> + if (id & CAN_RTR_FLAG)
> + fi |= FI_RTR;
> +
> + if (id & CAN_EFF_FLAG) {
> + fi |= FI_FF;
> + dreg = EFF_BUF;
> + priv->write_reg(dev, REG_FI, fi);
> + priv->write_reg(dev, REG_ID1, (id & 0x1fe00000) >> (5 + 16));
> + priv->write_reg(dev, REG_ID2, (id & 0x001fe000) >> (5 + 8));
> + priv->write_reg(dev, REG_ID3, (id & 0x00001fe0) >> 5);
> + priv->write_reg(dev, REG_ID4, (id & 0x0000001f) << 3);
> + } else {
> + dreg = SFF_BUF;
> + priv->write_reg(dev, REG_FI, fi);
> + priv->write_reg(dev, REG_ID1, (id & 0x000007f8) >> 3);
> + priv->write_reg(dev, REG_ID2, (id & 0x00000007) << 5);
> + }
> +
> + for (i = 0; i < dlc; i++)
> + priv->write_reg(dev, dreg++, cf->data[i]);
> +
> + stats->tx_bytes += dlc;
> + dev->trans_start = jiffies;
> +
> + can_put_echo_skb(skb, dev, 0);

Hmm...looking back at can_put_echo_skb(), I see that it expects dev->priv
to point to a struct can_priv. Here, though, we see it pointing to a
struct sja1000_prive instead. I begin to suspect dangerous trickery going
on behind our backs...

> +
> + priv->write_reg(dev, REG_CMR, CMD_TR);
> +
> + return 0;
> +}

[...]

> +irqreturn_t sja1000_interrupt(int irq, void *dev_id)
> +{
> + struct net_device *dev = (struct net_device *)dev_id;
> + struct sja1000_priv *priv = netdev_priv(dev);
> + struct net_device_stats *stats = &dev->stats;
> + uint8_t isrc, status;
> + int n = 0;
> +
> + /* Shared interrupts and IRQ off? */
> + if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
> + return IRQ_NONE;
> +
> + if (priv->pre_irq)
> + priv->pre_irq(dev);
> +
> + while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
> + n++;
> + status = priv->read_reg(dev, REG_SR);
> +
> + if (isrc & IRQ_WUI) {
> + /* wake-up interrupt */
> + priv->can.can_stats.wakeup++;
> + }
> + if (isrc & IRQ_TI) {
> + /* transmission complete interrupt */
> + stats->tx_packets++;
> + can_get_echo_skb(dev, 0);
> + netif_wake_queue(dev);
> + }
> + if (isrc & IRQ_RI) {
> + /* receive interrupt */
> + while (status & SR_RBS) {
> + sja1000_rx(dev);
> + status = priv->read_reg(dev, REG_SR);
> + }
> + }
> + if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
> + /* error interrupt */
> + if (sja1000_err(dev, isrc, status))
> + break;
> + }
> + }
> +
> + if (priv->post_irq)
> + priv->post_irq(dev);
> +
> + if (n >= SJA1000_MAX_IRQ)
> + dev_dbg(ND2D(dev), "%d messages handled in ISR", n);
> +
> + return (n) ? IRQ_HANDLED : IRQ_NONE;
> +}
> +EXPORT_SYMBOL_GPL(sja1000_interrupt);

You used spin_lock_irq(&irq_lock) above, but the interrupt handler doesn't
take that lock? So (above) you could acquire the lock while the interrupt
handler is running? I hate to keep asking this question, but...what does
that lock protect?

[...]

> +static int sja1000_close(struct net_device *dev)
> +{
> + struct sja1000_priv *priv = netdev_priv(dev);
> +
> + set_reset_mode(dev);
> + netif_stop_queue(dev);
> + priv->open_time = 0;
> + can_close_cleanup(dev);

What happens if your device interrupts right here? Maybe you should
disconnect the handler earlier?

> + if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER))
> + free_irq(dev->irq, (void *)dev);
> +
> + return 0;
> +}

[...]

> +int register_sja1000dev(struct net_device *dev)
> +{
> + struct sja1000_priv *priv = netdev_priv(dev);
> + int err;
> +
> + if (!sja1000_probe_chip(dev))
> + return -ENODEV;
> +
> + dev->flags |= IFF_ECHO; /* we support local echo */
> +
> + dev->netdev_ops = &sja1000_netdev_ops;
> +
> + priv->can.bittiming_const = &sja1000_bittiming_const;
> + priv->can.do_set_bittiming = sja1000_set_bittiming;
> + priv->can.do_get_state = sja1000_get_state;
> + priv->can.do_set_mode = sja1000_set_mode;
> + priv->dev = dev;
> +
> + err = register_candev(dev);

Here we've registered our device with the CAN and networking core...

> + if (err) {
> + printk(KERN_INFO
> + "%s: registering netdev failed\n", DRV_NAME);
> + free_netdev(dev);
> + return err;
> + }
> +
> + set_reset_mode(dev);
> + chipset_init(dev);

...but only here have we gotten it ready to operate. If the higher levels
decide to do something with your device in the mean time, will the right
thing happen?

> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_sja1000dev);

[...]

> diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
> new file mode 100644
> index 0000000..60d4cd6
> --- /dev/null
> +++ b/drivers/net/can/sja1000/sja1000.h

[...]

> +/*
> + * SJA1000 private data structure
> + */
> +struct sja1000_priv {
> + struct can_priv can; /* must be the first member! */

AHA! I knew it!

This kind of pointer trickery is fragile and dangerous, please don't do
it. Much better would be something like:

dev->priv = &dev_specific_priv->can;

Then the higher layers know they have a proper struct can_priv pointer.
Then you can use container_of() at this level to get the outer structure
pointer. Much more robust and in line with normal kernel coding idiom.

> + long open_time;
> + struct sk_buff *echo_skb;
> +
> + u8 (*read_reg) (struct net_device *dev, int reg);
> + void (*write_reg) (struct net_device *dev, int reg, u8 val);
> + void (*pre_irq) (struct net_device *dev);
> + void (*post_irq) (struct net_device *dev);
> +
> + void *priv; /* for board-specific data */
> + struct net_device *dev;
> +
> + u8 ocr;
> + u8 cdr;
> + u32 flags;


\
 
 \ /
  Last update: 2009-02-20 01:17    [W:0.195 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site