can: CAN network device driver interface and drivers

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	int err;
+
+	/* Synchronize with dev->hard_start_xmit() */
+	netif_tx_lock(dev);
+
+	/* Ensure that no more messages can go out */

Quoted 12 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	int err;
+
+	/* Synchronize with dev->hard_start_xmit() */
+	netif_tx_lock(dev);
+
+	/* Ensure that no more messages can go out */

drivers/net/can/dev.c line 349

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+	if (netif_carrier_ok(dev))
+		netif_carrier_off(dev);
+
+	/* Ensure that no more messages can come in */
+	err = priv->do_set_mode(dev, CAN_MODE_STOP);
+	if (err)
+		return err;
+
+	/*  Now it's save to clean up */
+	del_timer_sync(&priv->restart_timer);

This is deadlockable.

It calls del_timer_sync() while holding netif_tx_lock(). But the timer handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the timer handler to complete.

Quoted 26 lines · On Tue, 12 May 2009 23:30:52 -0700 Andrew Morton <[email protected]>…

Quoted 12 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	int err;
+
+	/* Synchronize with dev->hard_start_xmit() */
+	netif_tx_lock(dev);
+
+	/* Ensure that no more messages can go out */

drivers/net/can/dev.c line 349

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+	if (netif_carrier_ok(dev))
+		netif_carrier_off(dev);
+
+	/* Ensure that no more messages can come in */
+	err = priv->do_set_mode(dev, CAN_MODE_STOP);
+	if (err)
+		return err;
+
+	/*  Now it's save to clean up */
+	del_timer_sync(&priv->restart_timer);

This is deadlockable.

Quoted 5 lines

It calls del_timer_sync() while holding netif_tx_lock(). But the timer handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the timer handler to complete.

Also, I wonder if it's safe to take netif_tx_lock() from a timer handler when other parts of the code might be taking it from process context (I didn't check).

lockdep should be able to detect this, and I trust this code has been fully runtime tested with lockdep enabled?

Quoted 6 lines

Also, I wonder if it's safe to take netif_tx_lock() from a timer handler when other parts of the code might be taking it from process context (I didn't check).

lockdep should be able to detect this, and I trust this code has been fully runtime tested with lockdep enabled?

Well, CONFIG_PROVE_LOCKING would be cool, but I'm unable to enabled it for my MPC5200 test system. Only 64bit PowerPC's seem to support TRACE_IRQFLAGS_SUPPORT. I'm going to test the code on a PC as well.

Quoted 6 lines · On Wed, 13 May 2009 13:37:16 +0200 Wolfgang Grandegger <[email protected]>…

Quoted 6 lines

Also, I wonder if it's safe to take netif_tx_lock() from a timer handler when other parts of the code might be taking it from process context (I didn't check).

lockdep should be able to detect this, and I trust this code has been fully runtime tested with lockdep enabled?

Quoted 3 lines

Well, CONFIG_PROVE_LOCKING would be cool, but I'm unable to enabled it for my MPC5200 test system. Only 64bit PowerPC's seem to support TRACE_IRQFLAGS_SUPPORT. I'm going to test the code on a PC as well.

I discussed this off-list with Peter Zijlstra and Johannes Berg. Apparently lockdep _will_ detect this deadlockable situation - Johannes recently added the capability because he had the same situation in wireless code somewhere.

But of course it does require that the timer handler has executed at least once. Many handlers in the kernel never fire in normal operation.

Quoted 13 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	int err;
+
+	/* Synchronize with dev->hard_start_xmit() */
+	netif_tx_lock(dev);
+
+	/* Ensure that no more messages can go out */

drivers/net/can/dev.c line 349

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+	if (netif_carrier_ok(dev))
+		netif_carrier_off(dev);
+
+	/* Ensure that no more messages can come in */
+	err = priv->do_set_mode(dev, CAN_MODE_STOP);
+	if (err)
+		return err;
+
+	/*  Now it's save to clean up */
+	del_timer_sync(&priv->restart_timer);

This is deadlockable.

It calls del_timer_sync() while holding netif_tx_lock(). But the timer handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the timer handler to complete.

Quoted 30 lines · Andrew Morton

Quoted 13 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+    struct can_priv *priv = netdev_priv(dev);
+    struct net_device_stats *stats = &dev->stats;
+    struct sk_buff *skb;
+    struct can_frame *cf;
+    int err;
+
+    /* Synchronize with dev->hard_start_xmit() */
+    netif_tx_lock(dev);
+
+    /* Ensure that no more messages can go out */

drivers/net/can/dev.c line 349

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+    if (netif_carrier_ok(dev))
+        netif_carrier_off(dev);
+
+    /* Ensure that no more messages can come in */
+    err = priv->do_set_mode(dev, CAN_MODE_STOP);
+    if (err)
+        return err;
+
+    /*  Now it's save to clean up */
+    del_timer_sync(&priv->restart_timer);

This is deadlockable.

Quoted 8 lines

It calls del_timer_sync() while holding netif_tx_lock(). But the timer handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the timer handler to complete.

Hi Wolfgang,

would it be an appropriate solution, just to invoke

netif_stop_queue() in can_bus_off()

and invoke

netif_wake_queue() in can_restart_now()

???

In a BUSOFF condition we're not able to send CAN frames anyway, so we can disable the device queue and the we won't need any netif_tx_lock() right?

AFAIK this was the original implementation before some of the latest improvement with the netif_carrier stuff.

What do you think?

Quoted 58 lines · Oliver Hartkopp

Quoted 31 lines · Andrew Morton

Quoted 14 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+    struct can_priv *priv = netdev_priv(dev);
+    struct net_device_stats *stats = &dev->stats;
+    struct sk_buff *skb;
+    struct can_frame *cf;
+    int err;
+
+    /* Synchronize with dev->hard_start_xmit() */
+    netif_tx_lock(dev);
+
+    /* Ensure that no more messages can go out */

drivers/net/can/dev.c line 349

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+    if (netif_carrier_ok(dev))
+        netif_carrier_off(dev);
+
+    /* Ensure that no more messages can come in */
+    err = priv->do_set_mode(dev, CAN_MODE_STOP);
+    if (err)
+        return err;
+
+    /*  Now it's save to clean up */
+    del_timer_sync(&priv->restart_timer);

This is deadlockable.

Quoted 9 lines

It calls del_timer_sync() while holding netif_tx_lock(). But the timer handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the timer handler to complete.

Hi Wolfgang,

would it be an appropriate solution, just to invoke

netif_stop_queue() in can_bus_off()

and invoke

netif_wake_queue() in can_restart_now()

???

In a BUSOFF condition we're not able to send CAN frames anyway, so we can disable the device queue and the we won't need any netif_tx_lock() right?

Quoted 5 lines

AFAIK this was the original implementation before some of the latest improvement with the netif_carrier stuff.

What do you think?

The problem is the "manual" restart triggered via the netlink interface, which can occur in the middle of ndo_start_xmit().

Quoted 58 lines · Wolfgang Grandegger

Quoted 56 lines · Oliver Hartkopp

Quoted 30 lines · Andrew Morton

Quoted 13 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+    struct can_priv *priv = netdev_priv(dev);
+    struct net_device_stats *stats = &dev->stats;
+    struct sk_buff *skb;
+    struct can_frame *cf;
+    int err;
+
+    /* Synchronize with dev->hard_start_xmit() */
+    netif_tx_lock(dev);
+
+    /* Ensure that no more messages can go out */

drivers/net/can/dev.c line 349

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+    if (netif_carrier_ok(dev))
+        netif_carrier_off(dev);
+
+    /* Ensure that no more messages can come in */
+    err = priv->do_set_mode(dev, CAN_MODE_STOP);
+    if (err)
+        return err;
+
+    /*  Now it's save to clean up */
+    del_timer_sync(&priv->restart_timer);

This is deadlockable.

Quoted 8 lines

It calls del_timer_sync() while holding netif_tx_lock(). But the timer handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the timer handler to complete.

Hi Wolfgang,

would it be an appropriate solution, just to invoke

netif_stop_queue() in can_bus_off()

and invoke

netif_wake_queue() in can_restart_now()

???

In a BUSOFF condition we're not able to send CAN frames anyway, so we can disable the device queue and the we won't need any netif_tx_lock() right?

Quoted 10 lines

Quoted 5 lines

AFAIK this was the original implementation before some of the latest improvement with the netif_carrier stuff.

What do you think?

The problem is the "manual" restart triggered via the netlink interface, which can occur in the middle of ndo_start_xmit().

Ah, i see.

What if the manual restart via netlink would also stop the queue and start the timer?

Quoted 76 lines · Oliver Hartkopp

Quoted 74 lines · Wolfgang Grandegger

Quoted 56 lines · Oliver Hartkopp

Quoted 32 lines · Andrew Morton

Quoted 14 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

drivers/net/can/dev.c new file +12 -0

+int can_restart_now(struct net_device *dev)
+{
+    struct can_priv *priv = netdev_priv(dev);
+    struct net_device_stats *stats = &dev->stats;
+    struct sk_buff *skb;
+    struct can_frame *cf;
+    int err;
+
+    /* Synchronize with dev->hard_start_xmit() */
+    netif_tx_lock(dev);
+
+    /* Ensure that no more messages can go out */

drivers/net/can/dev.c line 349

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+    if (netif_carrier_ok(dev))
+        netif_carrier_off(dev);
+
+    /* Ensure that no more messages can come in */
+    err = priv->do_set_mode(dev, CAN_MODE_STOP);
+    if (err)
+        return err;
+
+    /*  Now it's save to clean up */
+    del_timer_sync(&priv->restart_timer);

This is deadlockable.

Quoted 9 lines

It calls del_timer_sync() while holding netif_tx_lock(). But the timer handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the timer handler to complete.

Hi Wolfgang,

would it be an appropriate solution, just to invoke

netif_stop_queue() in can_bus_off()

and invoke

netif_wake_queue() in can_restart_now()

???

Quoted 10 lines

In a BUSOFF condition we're not able to send CAN frames anyway, so we can disable the device queue and the we won't need any netif_tx_lock() right?

AFAIK this was the original implementation before some of the latest improvement with the netif_carrier stuff.

What do you think?

The problem is the "manual" restart triggered via the netlink interface, which can occur in the middle of ndo_start_xmit().

Quoted 5 lines

Ah, i see.

What if the manual restart via netlink would also stop the queue and start the timer?

It will not help if the restart is triggered in the middle of ndo_start_xmit().

Quoted 76 lines · Wolfgang Grandegger

Quoted 74 lines · Oliver Hartkopp

Quoted 72 lines · Wolfgang Grandegger

Quoted 59 lines · Oliver Hartkopp

Quoted 30 lines · Andrew Morton

Quoted 13 lines · On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <[email protected]>…

(quoted diff) +12 -0

+int can_restart_now(struct net_device *dev)
+{
+    struct can_priv *priv = netdev_priv(dev);
+    struct net_device_stats *stats = &dev->stats;
+    struct sk_buff *skb;
+    struct can_frame *cf;
+    int err;
+
+    /* Synchronize with dev->hard_start_xmit() */
+    netif_tx_lock(dev);
+
+    /* Ensure that no more messages can go out */

Review

Quoted 10 lines

(quoted diff) +10 -0

+    if (netif_carrier_ok(dev))
+        netif_carrier_off(dev);
+
+    /* Ensure that no more messages can come in */
+    err = priv->do_set_mode(dev, CAN_MODE_STOP);
+    if (err)
+        return err;
+
+    /*  Now it's save to clean up */
+    del_timer_sync(&priv->restart_timer);

This is deadlockable.

Quoted 10 lines

It calls del_timer_sync() while holding netif_tx_lock(). But the timer
handler (can_restart_now()) also takes netif_tx_lock(). So if the timer handler is presently running, it's sitting there spinning in netif_tx_lock(). And del_timer_sync() is sitting there waiting for the
timer handler to complete.

Hi Wolfgang,

would it be an appropriate solution, just to invoke

netif_stop_queue() in can_bus_off()

and invoke

netif_wake_queue() in can_restart_now()

???

In a BUSOFF condition we're not able to send CAN frames anyway, so we can disable the device queue and the we won't need any netif_tx_lock()
right?

Quoted 5 lines

AFAIK this was the original implementation before some of the latest improvement with the netif_carrier stuff.

What do you think?

The problem is the "manual" restart triggered via the netlink interface,
which can occur in the middle of ndo_start_xmit().

Quoted 10 lines

Quoted 5 lines

Ah, i see.

What if the manual restart via netlink would also stop the queue and start the timer?

It will not help if the restart is triggered in the middle of ndo_start_xmit().

Hi Wolfgang,

i think, i found a solution that removes the locking problem completely:

When a bus-off occurs in the controller, the communication on the CAN bus can be treated as unusable for this controller (let's say it is dead). E.g. the SJA1000 set's its reset bit for that reason and waits to be initialized by the CPU again.

So IMO restarting the CAN controller while in operational state is not a valid use case.

When a bus-off (interrupt) occurs, we should

- invoke netif_carrier_off(dev)
- invoke netif_stop_queue(dev)
- set the state to CAN_STATE_BUS_OFF

and of course create the error message, clear the interrupts(!) and then leave the irq service function.

That's it.

When the automatic CAN controller restart is enabled: start the timer.

For the manual netlink function: Test for CAN_STATE_BUS_OFF (!) and invoke the current can_restart_now(dev) or start the timer with e.g. 1ms
...

This approach should make it and fulfills the bus-off intention of the CAN controllers ("disabled and wait for re-initialisation").

And there's no locking of the tx_queue needed anymore as the tx_queue is already stopped, when the restart is performed.

What do you think about this approach?

drivers/net/can/dev.c new file +12 -0

+/*
+ * CAN device restart for bus-off recovery
+ */
+int can_restart_now(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	int err;
+
+	/* Synchronize with dev->hard_start_xmit() */

drivers/net/can/dev.c new file +7 -0

+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_CAN);
+	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
+	memset(cf, 0, sizeof(struct can_frame));
+	cf->can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
+	cf->can_dlc = CAN_ERR_DLC;
+

[Quick review ...]

Quoted 12 lines

drivers/net/can/dev.c new file +12 -0

+/*
+ * CAN device restart for bus-off recovery
+ */
+int can_restart_now(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	int err;
+
+	/* Synchronize with dev->hard_start_xmit() */

drivers/net/can/dev.c line 346

Quoted 10 lines

drivers/net/can/dev.c new file +10 -0

+	netif_tx_lock(dev);
+
+	/* Ensure that no more messages can go out */
+	if (netif_carrier_ok(dev))
+		netif_carrier_off(dev);
+
+	/* Ensure that no more messages can come in */
+	err = priv->do_set_mode(dev, CAN_MODE_STOP);
+	if (err)
+		return err;

This leaves _xmit_lock held and carrier off. Is that really what you want to do?

drivers/net/can/dev.c line 358

Quoted 12 lines

drivers/net/can/dev.c new file +12 -0

+
+	/*  Now it's save to clean up */
+	del_timer_sync(&priv->restart_timer);
+	can_flush_echo_skb(dev);
+
+	dev_dbg(dev->dev.parent, "restarted\n");
+	priv->can_stats.restarts++;
+
+	/* send restart message upstream */
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (skb == NULL)
+		return -ENOMEM;

...here too...

Quoted 12 lines

drivers/net/can/dev.c new file +12 -0

+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_CAN);
+	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
+	memset(cf, 0, sizeof(struct can_frame));
+	cf->can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
+	cf->can_dlc = CAN_ERR_DLC;
+
+	netif_rx(skb);
+
+	dev->last_rx = jiffies;
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;

drivers/net/can/dev.c line 375

Quoted 5 lines

drivers/net/can/dev.c new file +5 -0

+
+	/* Now restart the device */
+	err = priv->do_set_mode(dev, CAN_MODE_START);
+	if (err)
+		return err;

...and here too. Do you maybe want to get rid of the middle-of-function returns and switch to the "goto out" convention?

Quoted 8 lines

drivers/net/can/dev.c new file +8 -0

+	netif_carrier_on(dev);
+
+	netif_tx_unlock(dev);
+
+	return 0;
+}
+
+static void can_restart_after(unsigned long data)

drivers/net/can/dev.c line 389

Quoted 5 lines

drivers/net/can/dev.c new file +5 -0

+{
+	struct net_device *dev = (struct net_device *)data;
+
+	can_restart_now(dev);
+}

can_restart_after what? I find myself confused by this function and wondering why it exists.

drivers/net/can/sja1000/sja1000.c new file +9 -0

+static int set_reset_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	/* disable interrupts */
+	priv->write_reg(dev, REG_IER, IRQ_OFF);
+

drivers/net/can/sja1000/sja1000.c new file +17 -0

+	}
+
+	dev_err(dev->dev.parent, "setting SJA1000 into reset mode failed!\n");
+	return 1;
+
+}
+
+static int set_normal_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if ((status & MOD_RM) == 0) {
+			priv->can.state = CAN_STATE_ERROR_ACTIVE;

drivers/net/can/sja1000/sja1000.c new file +7 -0

+	}
+
+	dev_err(dev->dev.parent, "setting SJA1000 into normal mode failed!\n");
+	return 1;
+
+}
+

drivers/net/can/sja1000/sja1000.c new file +11 -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;

drivers/net/can/sja1000/sja1000.c new file +39 -0

+		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(dev->dev.parent, "%d messages handled in ISR", n);
+
+	return (n) ? IRQ_HANDLED : IRQ_NONE;
+}
+EXPORT_SYMBOL_GPL(sja1000_interrupt);
+
+static int sja1000_open(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	int err;
+
+	/* set chip into reset mode */
+	set_reset_mode(dev);
+
+	/* common open */

drivers/net/can/sja1000/sja1000.c new file +11 -0

+	}
+
+	/* init and start chi */
+	sja1000_start(dev);
+	priv->open_time = jiffies;
+
+	netif_start_queue(dev);
+
+	return 0;
+}
+

+};

[Quick drive-by review continues...]

drivers/net/can/sja1000/sja1000.c line 90

Quoted 4 lines

drivers/net/can/sja1000/sja1000.c new file +4 -0

+
+static int sja1000_probe_chip(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);

Looking down toward the bottom, I see:

drivers/net/can/sja1000/sja1000.h line 153

Quoted 2 lines

drivers/net/can/sja1000/sja1000.h new file +2 -0

+struct sja1000_priv {
+	struct can_priv can;

So you're still using the "put the higher-level structure at the top so we can treat it like either kind of pointer" trick. I'd still recommend against that. Far better to do something like:

	struct can_priv *canpriv = netdev_priv(dev);
	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);

Of course, you can put that dance into a helper function.

drivers/net/can/sja1000/sja1000.c line 98

Quoted 7 lines

drivers/net/can/sja1000/sja1000.c new file +7 -0

+	if (dev->base_addr && (priv->read_reg(dev, 0) == 0xFF)) {
+		printk(KERN_INFO "%s: probing @0x%lX failed\n",
+		       DRV_NAME, dev->base_addr);
+		return 0;
+	}
+	return 1;
+}

So zero is an error return? That's contrary to usual practice.

Quoted 13 lines

drivers/net/can/sja1000/sja1000.c new file +13 -0

+static int set_reset_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	/* disable interrupts */
+	priv->write_reg(dev, REG_IER, IRQ_OFF);
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if (status & MOD_RM) {
+			priv->can.state = CAN_STATE_STOPPED;

drivers/net/can/sja1000/sja1000.c line 118

Quoted 6 lines

drivers/net/can/sja1000/sja1000.c new file +6 -0

+			return 0;
+		}
+
+		priv->write_reg(dev, REG_MOD, MOD_RM);	/* reset chip */
+		status = priv->read_reg(dev, REG_MOD);
+		udelay(10);

Wouldn't you want to read the new state *after* the delay?

drivers/net/can/sja1000/sja1000.c line 122

Quoted 4 lines

drivers/net/can/sja1000/sja1000.c new file +4 -0

+	}
+
+	dev_err(dev->dev.parent, "setting SJA1000 into reset mode failed!\n");
+	return 1;

Quoted 13 lines

drivers/net/can/sja1000/sja1000.c new file +13 -0

+
+}
+
+static int set_normal_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if ((status & MOD_RM) == 0) {
+			priv->can.state = CAN_STATE_ERROR_ACTIVE;

drivers/net/can/sja1000/sja1000.c line 145

Quoted 10 lines

drivers/net/can/sja1000/sja1000.c new file +10 -0

+			/* enable all interrupts */
+			priv->write_reg(dev, REG_IER, IRQ_ALL);
+
+			return 0;
+		}
+
+		/* set chip to normal mode */
+		priv->write_reg(dev, REG_MOD, 0x00);
+		status = priv->read_reg(dev, REG_MOD);
+		udelay(10);

Here too?

Quoted 7 lines

drivers/net/can/sja1000/sja1000.c new file +7 -0

+	}
+
+	dev_err(dev->dev.parent, "setting SJA1000 into normal mode failed!\n");
+	return 1;
+
+}
+

[...]

Quoted 15 lines

drivers/net/can/sja1000/sja1000.c new file +15 -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);
+

drivers/net/can/sja1000/sja1000.c line 495

Quoted 6 lines

drivers/net/can/sja1000/sja1000.c new file +6 -0

+	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
+		n++;
+		status = priv->read_reg(dev, REG_SR);
+
+		if (isrc & IRQ_WUI)
+			dev_warn(dev->dev.parent, "wakeup interrupt\n");

How many of these might you get? Should this be rate limited?

Quoted 45 lines

drivers/net/can/sja1000/sja1000.c new file +45 -0

+		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(dev->dev.parent, "%d messages handled in ISR", n);
+
+	return (n) ? IRQ_HANDLED : IRQ_NONE;
+}
+EXPORT_SYMBOL_GPL(sja1000_interrupt);
+
+static int sja1000_open(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	int err;
+
+	/* set chip into reset mode */
+	set_reset_mode(dev);
+
+	/* common open */
+	err = open_candev(dev);
+	if (err)
+		return err;
+
+	/* register interrupt handler, if not done by the device driver */
+	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER)) {

drivers/net/can/sja1000/sja1000.c line 545

Quoted 4 lines

drivers/net/can/sja1000/sja1000.c new file +4 -0

+		err = request_irq(dev->irq, &sja1000_interrupt, IRQF_SHARED,
+				  dev->name, (void *)dev);
+		if (err)
+			return -EAGAIN;

If you return here you fail, but you've not undone open_candev(). Looking there, it seems no harm will be done - until somebody changes open_candev() someday.

Quoted 11 lines

drivers/net/can/sja1000/sja1000.c new file +11 -0

+	}
+
+	/* init and start chi */
+	sja1000_start(dev);
+	priv->open_time = jiffies;
+
+	netif_start_queue(dev);
+
+	return 0;
+}
+

[...]

drivers/net/can/sja1000/sja1000.h line 160

Quoted 12 lines

drivers/net/can/sja1000/sja1000.h new file +12 -0

+/*
+ * SJA1000 private data structure
+ */
+struct sja1000_priv {
+	struct can_priv can;
+	long open_time;
+	struct sk_buff *echo_skb;
+
+	u8 (*read_reg) (const struct net_device *dev, int reg);
+	void (*write_reg) (const struct net_device *dev, int reg, u8 val);
+	void (*pre_irq) (const struct net_device *dev);
+	void (*post_irq) (const struct net_device *dev);

What are the locking rules for functions like ->read_reg() now? Entirely up to the lower level? Would be good to document that near the structure definition.

drivers/net/can/sja1000/sja1000.h line 167

Quoted 7 lines

drivers/net/can/sja1000/sja1000.h new file +7 -0

+
+	void *priv;		/* for board-specific data */
+	struct net_device *dev;
+
+	u8 ocr;
+	u8 cdr;
+	u32 flags;

The meaning of these fields is not exactly clear.

Quoted 7 lines

So you're still using the "put the higher-level structure at the top so we can treat it like either kind of pointer" trick. I'd still recommend against that. Far better to do something like:

	struct can_priv *canpriv = netdev_priv(dev);
	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);

Quoted 1 line

Of course, you can put that dance into a helper function.

There is no way to initialize the value returned by netdev_priv() as it does not point to a member of struct net_device. I already commented here:

http://marc.info/?l=linux-netdev&m=124120212106891&w=2

Have I missed something?

Quoted 7 lines · On Thu, 14 May 2009 11:03:53 +0200 Wolfgang Grandegger <[email protected]>…

Quoted 7 lines

So you're still using the "put the higher-level structure at the top so we can treat it like either kind of pointer" trick. I'd still recommend against that. Far better to do something like:

	struct can_priv *canpriv = netdev_priv(dev);
	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);

Quoted 7 lines

Quoted 1 line

Of course, you can put that dance into a helper function.

There is no way to initialize the value returned by netdev_priv() as it does not point to a member of struct net_device. I already commented here:

http://marc.info/?l=linux-netdev&m=124120212106891&w=2

Have I missed something?

I'm confused. It points to the struct can_priv that you registered at the beginning. Since that structure is contained within struct sja1000_priv, you can use container_of(), as described above, to get it.

I would probably just write something like:

static inline struct sja1000_priv *to_sja1000_priv(struct net_device *dev)

{
	return container_of(netdev_priv(dev), struct sja1000_priv, can);
}

So have *I* missed something?

Quoted 22 lines · Jonathan Corbet

Quoted 7 lines · On Thu, 14 May 2009 11:03:53 +0200 Wolfgang Grandegger <[email protected]>…

Quoted 7 lines

So you're still using the "put the higher-level structure at the top so we can treat it like either kind of pointer" trick. I'd still recommend against that. Far better to do something like:

	struct can_priv *canpriv = netdev_priv(dev);
	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);

Quoted 7 lines

Quoted 1 line

Of course, you can put that dance into a helper function.

There is no way to initialize the value returned by netdev_priv() as it does not point to a member of struct net_device. I already commented here:

http://marc.info/?l=linux-netdev&m=124120212106891&w=2

Have I missed something?

I'm confused. It points to the struct can_priv that you registered at the beginning. Since that structure is contained within struct sja1000_priv, you can use container_of(), as described above, to get it.

Quoted 8 lines

I would probably just write something like:

static inline struct sja1000_priv *to_sja1000_priv(struct net_device *dev)

{
	return container_of(netdev_priv(dev), struct sja1000_priv, can);
}

So have *I* missed something?

Furthermore, the higher layer needs to known the location of the member "struct sja1000_priv can", e.g. by defining:

  dev->priv = &dev_specific_priv->can;

But "struct net_device" does not have a "priv" member. netdev_priv(dev) always points to the beginning of the private data area. See:

   http://lxr.linux.no/linux+v2.6.29/include/linux/netdevice.h#L953

Quoted 4 lines

But "struct net_device" does not have a "priv" member. netdev_priv(dev) always points to the beginning of the private data area. See:

   http://lxr.linux.no/linux+v2.6.29/include/linux/netdevice.h#L953  

I could use container_of() as you suggested, of course, if the "struct can_priv" remains the first member of "struct sja1000_priv". Would that already be an improvement?

This driver adds support for the SJA1000 chips connected to the "platform bus", which can be found on various embedded systems.

Quoted 2 lines · On Tue, 12 May 2009 11:28:02 +0200 Wolfgang Grandegger <[email protected]>…

This driver adds support for the SJA1000 chips connected to the "platform bus", which can be found on various embedded systems.

[...]

drivers/net/can/sja1000/sja1000_platform.c line 48

Quoted 10 lines

drivers/net/can/sja1000/sja1000_platform.c new file +10 -0

+
+static u8 sp_read_reg(const struct net_device *dev, int reg)
+{
+	return ioread8((void __iomem *)(dev->base_addr + reg));
+}
+
+static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
+{
+	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
+}

So there's no locking around accesses to the hardware at all. How do you protect against concurrent access?

[...]

drivers/net/can/sja1000/sja1000_platform.c line 142

Quoted 12 lines

drivers/net/can/sja1000/sja1000_platform.c new file +12 -0

+static int sp_remove(struct platform_device *pdev)
+{
+	struct net_device *dev = dev_get_drvdata(&pdev->dev);
+	struct resource *res;
+
+	unregister_sja1000dev(dev);
+	dev_set_drvdata(&pdev->dev, NULL);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, res->end - res->start + 1);
+
+	if (dev->base_addr)
+		iounmap((void __iomem *)dev->base_addr);

Seems like you should unmap it before releasing it back to the kernel. Nobody else is ever going to jump in and try to map it, but still...

This driver adds support for the SJA1000 chips connected to the "platform bus", which can be found on various embedded systems.

Signed-off-by Sascha Hauer <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/Kconfig                    |   10 +
 drivers/net/can/sja1000/Makefile           |    1 
 drivers/net/can/sja1000/sja1000_platform.c |  169 +++++++++++++++++++++++++++++
 include/linux/can/platform/sja1000.h       |   32 +++++
 4 files changed, 212 insertions(+)
 create mode 100644 drivers/net/can/sja1000/sja1000_platform.c
 create mode 100644 include/linux/can/platform/sja1000.h

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================

drivers/net/can/Kconfig +10 -0

@@ -41,6 +41,16 @@
 	---help---
 	  Driver for the SJA1000 CAN controllers from Philips or NXP
 
+config CAN_SJA1000_PLATFORM
+	depends on CAN_SJA1000
+	tristate "Generic Platform Bus based SJA1000 driver"
+	---help---
+	  This driver adds support for the SJA1000 chips connected to
+	  the "platform bus" (Linux abstraction for directly to the
+	  processor attached devices).  Which can be found on various
+	  boards from Phytec (http://www.phytec.de) like the PCM027,
+	  PCM038.
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN

Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================

drivers/net/can/sja1000/Makefile +1 -0

@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
+obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG

Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
===================================================================

drivers/net/can/sja1000/sja1000_platform.c new file +65 -0

@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) 2005 Sascha Hauer, Pengutronix
+ * Copyright (C) 2007 Wolfgang Grandegger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/platform/sja1000.h>
+#include <linux/io.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME "sja1000_platform"
+
+MODULE_AUTHOR("Sascha Hauer <[email protected]>");
+MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
+MODULE_LICENSE("GPL v2");
+
+static u8 sp_read_reg(const struct net_device *dev, int reg)
+{
+	return ioread8((void __iomem *)(dev->base_addr + reg));
+}
+
+static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
+{
+	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
+}
+
+static int sp_probe(struct platform_device *pdev)
+{
+	int err, irq;
+	void __iomem *addr;
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+	struct resource *res_mem, *res_irq;
+	struct sja1000_platform_data *pdata;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data provided!\n");
+		err = -ENODEV;
+		goto exit;
+	}
+

drivers/net/can/sja1000/sja1000_platform.c new file +4 -0

+		err = -EBUSY;
+		goto exit;
+	}
+

drivers/net/can/sja1000/sja1000_platform.c new file +19 -0

+
+	dev = alloc_sja1000dev(0);
+	if (!dev) {
+		err = -ENOMEM;
+		goto exit_iounmap;
+	}
+	priv = netdev_priv(dev);
+
+	priv->read_reg = sp_read_reg;
+	priv->write_reg = sp_write_reg;
+	priv->can.clock.freq = pdata->clock;
+	priv->ocr = pdata->ocr;
+	priv->cdr = pdata->cdr;
+
+	dev->irq = irq;
+	dev->base_addr = (unsigned long)addr;
+
+	dev_set_drvdata(&pdev->dev, dev);
+	SET_NETDEV_DEV(dev, &pdev->dev);

drivers/net/can/sja1000/sja1000_platform.c new file +51 -0

+	return 0;
+
+ exit_free:
+	free_sja1000dev(dev);
+ exit_iounmap:
+	iounmap(addr);
+ exit_release:
+	release_mem_region(res_mem->start, res_mem->end - res_mem->start + 1);
+ exit:
+	return err;
+}
+
+static int sp_remove(struct platform_device *pdev)
+{
+	struct net_device *dev = dev_get_drvdata(&pdev->dev);
+	struct resource *res;
+
+	unregister_sja1000dev(dev);
+	dev_set_drvdata(&pdev->dev, NULL);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, res->end - res->start + 1);
+
+	if (dev->base_addr)
+		iounmap((void __iomem *)dev->base_addr);
+
+	free_sja1000dev(dev);
+
+	return 0;
+}
+
+static struct platform_driver sp_driver = {
+	.probe = sp_probe,
+	.remove = sp_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init sp_init(void)
+{
+	return platform_driver_register(&sp_driver);
+}
+
+static void __exit sp_exit(void)
+{
+	platform_driver_unregister(&sp_driver);
+}
+
+module_init(sp_init);
+module_exit(sp_exit);

Index: net-next-2.6/include/linux/can/platform/sja1000.h
===================================================================

include/linux/can/platform/sja1000.h new file +32 -0

@@ -0,0 +1,32 @@
+#ifndef _CAN_PLATFORM_SJA1000_H_
+#define _CAN_PLATFORM_SJA1000_H_
+
+/* clock divider register */
+#define CDR_CLKOUT_MASK 0x07
+#define CDR_CLK_OFF	0x08 /* Clock off (CLKOUT pin) */
+#define CDR_RXINPEN	0x20 /* TX1 output is RX irq output */
+#define CDR_CBP		0x40 /* CAN input comparator bypass */
+#define CDR_PELICAN	0x80 /* PeliCAN mode */
+
+/* output control register */
+#define OCR_MODE_BIPHASE  0x00
+#define OCR_MODE_TEST     0x01
+#define OCR_MODE_NORMAL   0x02
+#define OCR_MODE_CLOCK    0x03
+#define OCR_TX0_INVERT    0x04
+#define OCR_TX0_PULLDOWN  0x08
+#define OCR_TX0_PULLUP    0x10
+#define OCR_TX0_PUSHPULL  0x18
+#define OCR_TX1_INVERT    0x20
+#define OCR_TX1_PULLDOWN  0x40
+#define OCR_TX1_PULLUP    0x80
+#define OCR_TX1_PUSHPULL  0xc0
+
+struct sja1000_platform_data {
+	u32 clock;	/* CAN bus oscillator frequency in Hz */
+
+	u8 ocr;		/* output control register */
+	u8 cdr;		/* clock divider register */
+};
+
+#endif	/* !_CAN_PLATFORM_SJA1000_H_ */

Hi Wolfgang,

some comments inline.

Sascha

Quoted 123 lines · Wolfgang Grandegger

This driver adds support for the SJA1000 chips connected to the "platform bus", which can be found on various embedded systems.

Signed-off-by Sascha Hauer <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/Kconfig                    |   10 +
 drivers/net/can/sja1000/Makefile           |    1 
 drivers/net/can/sja1000/sja1000_platform.c |  169 +++++++++++++++++++++++++++++
 include/linux/can/platform/sja1000.h       |   32 +++++
 4 files changed, 212 insertions(+)
 create mode 100644 drivers/net/can/sja1000/sja1000_platform.c
 create mode 100644 include/linux/can/platform/sja1000.h

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================

drivers/net/can/Kconfig +10 -0

@@ -41,6 +41,16 @@
 	---help---
 	  Driver for the SJA1000 CAN controllers from Philips or NXP
 
+config CAN_SJA1000_PLATFORM
+	depends on CAN_SJA1000
+	tristate "Generic Platform Bus based SJA1000 driver"
+	---help---
+	  This driver adds support for the SJA1000 chips connected to
+	  the "platform bus" (Linux abstraction for directly to the
+	  processor attached devices).  Which can be found on various
+	  boards from Phytec (http://www.phytec.de) like the PCM027,
+	  PCM038.
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN

Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================

drivers/net/can/sja1000/Makefile +1 -0

@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
+obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG

Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
===================================================================

drivers/net/can/sja1000/sja1000_platform.c new file +70 -0

@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) 2005 Sascha Hauer, Pengutronix
+ * Copyright (C) 2007 Wolfgang Grandegger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/platform/sja1000.h>
+#include <linux/io.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME "sja1000_platform"
+
+MODULE_AUTHOR("Sascha Hauer <[email protected]>");
+MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
+MODULE_LICENSE("GPL v2");
+
+static u8 sp_read_reg(const struct net_device *dev, int reg)
+{
+	return ioread8((void __iomem *)(dev->base_addr + reg));
+}
+
+static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
+{
+	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
+}
+
+static int sp_probe(struct platform_device *pdev)
+{
+	int err, irq;
+	void __iomem *addr;
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+	struct resource *res_mem, *res_irq;
+	struct sja1000_platform_data *pdata;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data provided!\n");
+		err = -ENODEV;
+		goto exit;
+	}
+
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res_mem || !res_irq) {
+		err = -ENODEV;
+		goto exit;

drivers/net/can/sja1000/sja1000_platform.c line 75

Quoted 5 lines

drivers/net/can/sja1000/sja1000_platform.c new file +5 -0

+	}
+
+	if (!request_mem_region(res_mem->start,
+				res_mem->end - res_mem->start + 1,
+				DRV_NAME)) {

resource_size(res_mem) please, also for the other occurrences in this file

Quoted 9 lines

drivers/net/can/sja1000/sja1000_platform.c new file +9 -0

+		err = -EBUSY;
+		goto exit;
+	}
+
+	addr = ioremap_nocache(res_mem->start,
+			       res_mem->end - res_mem->start + 1);
+	if (!addr) {
+		err = -ENOMEM;
+		goto exit_release;

drivers/net/can/sja1000/sja1000_platform.c line 89

Quoted 5 lines

drivers/net/can/sja1000/sja1000_platform.c new file +5 -0

+	}
+
+	irq = res_irq->start;
+	if (res_irq->flags & IRQF_TRIGGER_MASK)
+		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);

You shouldn't use set_irq_type on not yet requested irqs but instead pass the flags to the real driver and pass them in request_irq.

Hi Sascha,

Quoted 130 lines · Sascha Hauer

Hi Wolfgang,

some comments inline.

Sascha

Quoted 123 lines · Wolfgang Grandegger

This driver adds support for the SJA1000 chips connected to the "platform bus", which can be found on various embedded systems.

Signed-off-by Sascha Hauer <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/Kconfig                    |   10 +
 drivers/net/can/sja1000/Makefile           |    1 
 drivers/net/can/sja1000/sja1000_platform.c |  169 +++++++++++++++++++++++++++++
 include/linux/can/platform/sja1000.h       |   32 +++++
 4 files changed, 212 insertions(+)
 create mode 100644 drivers/net/can/sja1000/sja1000_platform.c
 create mode 100644 include/linux/can/platform/sja1000.h

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================

drivers/net/can/Kconfig +10 -0

@@ -41,6 +41,16 @@
 	---help---
 	  Driver for the SJA1000 CAN controllers from Philips or NXP
 
+config CAN_SJA1000_PLATFORM
+	depends on CAN_SJA1000
+	tristate "Generic Platform Bus based SJA1000 driver"
+	---help---
+	  This driver adds support for the SJA1000 chips connected to
+	  the "platform bus" (Linux abstraction for directly to the
+	  processor attached devices).  Which can be found on various
+	  boards from Phytec (http://www.phytec.de) like the PCM027,
+	  PCM038.
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN

Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================

drivers/net/can/sja1000/Makefile +1 -0

@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
+obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG

Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
===================================================================

drivers/net/can/sja1000/sja1000_platform.c new file +70 -0

@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) 2005 Sascha Hauer, Pengutronix
+ * Copyright (C) 2007 Wolfgang Grandegger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/platform/sja1000.h>
+#include <linux/io.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME "sja1000_platform"
+
+MODULE_AUTHOR("Sascha Hauer <[email protected]>");
+MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
+MODULE_LICENSE("GPL v2");
+
+static u8 sp_read_reg(const struct net_device *dev, int reg)
+{
+	return ioread8((void __iomem *)(dev->base_addr + reg));
+}
+
+static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
+{
+	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
+}
+
+static int sp_probe(struct platform_device *pdev)
+{
+	int err, irq;
+	void __iomem *addr;
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+	struct resource *res_mem, *res_irq;
+	struct sja1000_platform_data *pdata;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data provided!\n");
+		err = -ENODEV;
+		goto exit;
+	}
+
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res_mem || !res_irq) {
+		err = -ENODEV;
+		goto exit;

Quoted 8 lines

drivers/net/can/sja1000/sja1000_platform.c line 75

Quoted 5 lines

drivers/net/can/sja1000/sja1000_platform.c new file +5 -0

+	}
+
+	if (!request_mem_region(res_mem->start,
+				res_mem->end - res_mem->start + 1,
+				DRV_NAME)) {

resource_size(res_mem) please, also for the other occurrences in this file

OK.

Quoted 9 lines

Quoted 9 lines

drivers/net/can/sja1000/sja1000_platform.c new file +9 -0

+		err = -EBUSY;
+		goto exit;
+	}
+
+	addr = ioremap_nocache(res_mem->start,
+			       res_mem->end - res_mem->start + 1);
+	if (!addr) {
+		err = -ENOMEM;
+		goto exit_release;

Quoted 8 lines

drivers/net/can/sja1000/sja1000_platform.c line 89

Quoted 5 lines

drivers/net/can/sja1000/sja1000_platform.c new file +5 -0

+	}
+
+	irq = res_irq->start;
+	if (res_irq->flags & IRQF_TRIGGER_MASK)
+		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);

You shouldn't use set_irq_type on not yet requested irqs but instead pass the flags to the real driver and pass them in request_irq.

Why? I would require an extra member in the struct sja1000_priv.

Quoted 4 lines · Wolfgang Grandegger

Hi Sascha,

Quoted 1 line · Sascha Hauer

Hi Wolfgang,

Quoted 6 lines

Quoted 5 lines

drivers/net/can/sja1000/sja1000_platform.c line 89

Quoted 3 lines

drivers/net/can/sja1000/sja1000_platform.c new file +3 -0

+	irq = res_irq->start;
+	if (res_irq->flags & IRQF_TRIGGER_MASK)
+		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);

You shouldn't use set_irq_type on not yet requested irqs but instead pass the flags to the real driver and pass them in request_irq.

Why? I would require an extra member in the struct sja1000_priv.

You change a resource you do not own. What if request_irq returns with -EBUSY? You already screwed up any other potential user.

drivers/net/can/sja1000/kvaser_pci.c new file +5 -0

+	/* Disable interrupts from card */
+	tmp = ioread32(board->conf_addr + S5920_INTCSR);
+	tmp &= ~INTCSR_ADDON_INTENABLE_M;
+	iowrite32(tmp, board->conf_addr + S5920_INTCSR);
+}

drivers/net/can/sja1000/kvaser_pci.c new file +5 -0

+static int number_of_sja1000_chip(void __iomem *base_addr)
+{
+	u8 status;
+	int i;
+

drivers/net/can/sja1000/kvaser_pci.c new file +6 -0

+	}
+
+	return i;
+}
+
+static void kvaser_pci_del_chan(struct net_device *dev)

drivers/net/can/sja1000/kvaser_pci.c new file +9 -0

+	board = priv->priv;
+	if (!board)
+		return;
+
+	dev_info(&board->pci_dev->dev, "Removing device %s\n",
+		 dev->name);
+
+	for (i = 0; i < board->no_channels - 1; i++) {
+		if (board->slave_dev[i]) {

drivers/net/can/sja1000/kvaser_pci.c new file +13 -0

+	pci_iounmap(board->pci_dev, (void __iomem *)dev->base_addr);
+	pci_iounmap(board->pci_dev, board->conf_addr);
+	pci_iounmap(board->pci_dev, board->res_addr);
+
+	free_sja1000dev(dev);
+}
+
+static int kvaser_pci_add_chan(struct pci_dev *pdev, int channel,
+			       struct net_device **master_dev,
+			       void __iomem *conf_addr,
+			       void __iomem *res_addr,
+			       unsigned long base_addr)
+{

drivers/net/can/sja1000/kvaser_pci.c new file +14 -0

+	board = priv->priv;
+
+	board->pci_dev = pdev;
+	board->channel = channel;
+
+	/*S5920*/
+	board->conf_addr = conf_addr;
+
+	/*XILINX board wide address*/
+	board->res_addr = res_addr;
+
+	if (channel == 0) {
+		board->xilinx_ver =
+			ioread8(board->res_addr + XILINX_VERINT) >> 4;

drivers/net/can/sja1000/kvaser_pci.c new file +64 -0

+	} else {
+		struct sja1000_priv *master_priv = netdev_priv(*master_dev);
+		struct kvaser_pci *master_board = master_priv->priv;
+		master_board->slave_dev[channel - 1] = dev;
+		master_board->no_channels = channel + 1;
+		board->xilinx_ver = master_board->xilinx_ver;
+	}
+
+	dev->base_addr = base_addr + channel * KVASER_PCI_PORT_BYTES;
+
+	priv->read_reg = kvaser_pci_read_reg;
+	priv->write_reg = kvaser_pci_write_reg;
+
+	priv->can.clock.freq = KVASER_PCI_CAN_CLOCK;
+
+	priv->ocr = KVASER_PCI_OCR;
+	priv->cdr = KVASER_PCI_CDR;
+
+	/* Register and setup interrupt handling */
+	dev->irq = pdev->irq;
+	init_step = 4;
+
+	dev_info(&pdev->dev, "base_addr=%#lx conf_addr=%p irq=%d\n",
+		 dev->base_addr, board->conf_addr, dev->irq);
+
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	/* Register SJA1000 device */
+	err = register_sja1000dev(dev);
+	if (err) {
+		dev_err(&pdev->dev, "Registering device failed (err=%d)\n",
+			err);
+		goto failure;
+	}
+
+	if (channel == 0)
+		*master_dev = dev;
+
+	return 0;
+
+failure:
+	kvaser_pci_del_chan(dev);
+	return err;
+}
+
+static int __devinit kvaser_pci_init_one(struct pci_dev *pdev,
+					 const struct pci_device_id *ent)
+{
+	int err;
+	struct net_device *master_dev = NULL;
+	struct sja1000_priv *priv;
+	struct kvaser_pci *board;
+	int no_channels;
+	void __iomem *base_addr = NULL;
+	void __iomem *conf_addr = NULL;
+	void __iomem *res_addr = NULL;
+	int i;
+
+	dev_info(&pdev->dev, "initializing device %04x:%04x\n",
+		 pdev->vendor, pdev->device);
+
+	err = pci_enable_device(pdev);
+	if (err)
+		goto failure;

drivers/net/can/sja1000/kvaser_pci.c new file +48 -0

+	}
+
+	/*XILINX board wide address*/
+	res_addr = pci_iomap(pdev, 2, PCI_PORT_XILINX_SIZE);
+	if (res_addr == NULL) {
+		err = -ENOMEM;
+		goto failure_iounmap;
+	}
+
+	base_addr = pci_iomap(pdev, 1, PCI_PORT_SIZE);
+	if (base_addr == NULL) {
+		err = -ENOMEM;
+		goto failure_iounmap;
+	}
+
+	no_channels = number_of_sja1000_chip(base_addr);
+	if (no_channels == 0) {
+		err = -ENOMEM;
+		goto failure_iounmap;
+	}
+
+	for (i = 0; i < no_channels; i++) {
+		err = kvaser_pci_add_chan(pdev, i, &master_dev,
+					  conf_addr, res_addr,
+					  (unsigned long)base_addr);
+		if (err)
+			goto failure_cleanup;
+	}
+
+	priv = netdev_priv(master_dev);
+	board = priv->priv;
+
+	dev_info(&pdev->dev, "xilinx version=%d number of channels=%d\n",
+		 board->xilinx_ver, board->no_channels);
+
+	pci_set_drvdata(pdev, master_dev);
+	return 0;
+
+failure_cleanup:
+	kvaser_pci_del_chan(master_dev);
+
+failure_iounmap:
+	if (conf_addr == NULL)
+		pci_iounmap(pdev, conf_addr);
+	if (res_addr == NULL)
+		pci_iounmap(pdev, res_addr);
+	if (base_addr == NULL)
+		pci_iounmap(pdev, base_addr);

Quoted 2 lines · On Tue, 12 May 2009 11:28:04 +0200 Wolfgang Grandegger <[email protected]>…

The patch adds support for the PCI cards: PCIcan and PCIcanx (1, 2 or 4 channel) from Kvaser (http://www.kvaser.com).

drivers/net/can/sja1000/kvaser_pci.c line 135

Quoted 5 lines

drivers/net/can/sja1000/kvaser_pci.c new file +5 -0

+static void kvaser_pci_disable_irq(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct kvaser_pci *board = priv->priv;
+	u32 tmp;

I've seen certain reviewers getting grumpy about variables called "tmp" recently.

Quoted 5 lines

drivers/net/can/sja1000/kvaser_pci.c new file +5 -0

+	/* Disable interrupts from card */
+	tmp = ioread32(board->conf_addr + S5920_INTCSR);
+	tmp &= ~INTCSR_ADDON_INTENABLE_M;
+	iowrite32(tmp, board->conf_addr + S5920_INTCSR);
+}

[...]

Quoted 9 lines

drivers/net/can/sja1000/kvaser_pci.c new file +9 -0

+static int number_of_sja1000_chip(void __iomem *base_addr)
+{
+	u8 status;
+	int i;
+
+	for (i = 0; i < MAX_NO_OF_CHANNELS; i++) {
+		/* reset chip */
+		iowrite8(MOD_RM, base_addr +
+			 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);

drivers/net/can/sja1000/kvaser_pci.c line 169

Quoted 6 lines

drivers/net/can/sja1000/kvaser_pci.c new file +6 -0

+		status = ioread8(base_addr +
+				 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);
+		udelay(10);
+		/* check reset bit */
+		if (!(status & MOD_RM))
+			break;

Why would you delay before checking status? It ain't gonna change.

Quoted 10 lines

drivers/net/can/sja1000/kvaser_pci.c new file +10 -0

+	}
+
+	return i;
+}
+
+static void kvaser_pci_del_chan(struct net_device *dev)
+{
+	struct sja1000_priv *priv;
+	struct kvaser_pci *board;
+	int i;

drivers/net/can/sja1000/kvaser_pci.c line 185

Quoted 6 lines

drivers/net/can/sja1000/kvaser_pci.c new file +6 -0

+
+	if (!dev)
+		return;
+	priv = netdev_priv(dev);
+	if (!priv)
+		return;

Can this happen?

Quoted 15 lines

drivers/net/can/sja1000/kvaser_pci.c new file +15 -0

+	board = priv->priv;
+	if (!board)
+		return;
+
+	dev_info(&board->pci_dev->dev, "Removing device %s\n",
+		 dev->name);
+
+	for (i = 0; i < board->no_channels - 1; i++) {
+		if (board->slave_dev[i]) {
+			dev_info(&board->pci_dev->dev, "Removing device %s\n",
+				 board->slave_dev[i]->name);
+			unregister_sja1000dev(board->slave_dev[i]);
+			free_sja1000dev(board->slave_dev[i]);
+		}
+	}

drivers/net/can/sja1000/kvaser_pci.c line 204

Quoted 4 lines

drivers/net/can/sja1000/kvaser_pci.c new file +4 -0

+	unregister_sja1000dev(dev);
+
+	/* Disable PCI interrupts */
+	kvaser_pci_disable_irq(dev);

It seems to me like you might want to disable interrupts *before* tearing down all that structure? What happens if it interrupts after the sja1000 layer has forgotten about it?

Quoted 13 lines

drivers/net/can/sja1000/kvaser_pci.c new file +13 -0

+	pci_iounmap(board->pci_dev, (void __iomem *)dev->base_addr);
+	pci_iounmap(board->pci_dev, board->conf_addr);
+	pci_iounmap(board->pci_dev, board->res_addr);
+
+	free_sja1000dev(dev);
+}
+
+static int kvaser_pci_add_chan(struct pci_dev *pdev, int channel,
+			       struct net_device **master_dev,
+			       void __iomem *conf_addr,
+			       void __iomem *res_addr,
+			       unsigned long base_addr)
+{

drivers/net/can/sja1000/kvaser_pci.c line 228

Quoted 10 lines

drivers/net/can/sja1000/kvaser_pci.c new file +10 -0

+	struct net_device *dev;
+	struct sja1000_priv *priv;
+	struct kvaser_pci *board;
+	int err, init_step;
+
+	dev = alloc_sja1000dev(sizeof(struct kvaser_pci));
+	if (dev == NULL)
+		return -ENOMEM;
+
+	priv = netdev_priv(dev);

Here you don't have the !priv check.

Quoted 19 lines

drivers/net/can/sja1000/kvaser_pci.c new file +19 -0

+	board = priv->priv;
+
+	board->pci_dev = pdev;
+	board->channel = channel;
+
+	/*S5920*/
+	board->conf_addr = conf_addr;
+
+	/*XILINX board wide address*/
+	board->res_addr = res_addr;
+
+	if (channel == 0) {
+		board->xilinx_ver =
+			ioread8(board->res_addr + XILINX_VERINT) >> 4;
+		init_step = 2;
+
+		/* Assert PTADR# - we're in passive mode so the other bits are
+		   not important */
+		iowrite32(0x80808080UL, board->conf_addr + S5920_PTCR);

drivers/net/can/sja1000/kvaser_pci.c line 252

Quoted 5 lines

drivers/net/can/sja1000/kvaser_pci.c new file +5 -0

+
+		/* Disable interrupts from card */
+		kvaser_pci_disable_irq(dev);
+		/* Enable interrupts from card */
+		kvaser_pci_enable_irq(dev);

I'm sure there's a perfectly good reason for the disable/enable dance. Presumably the hardware needs it or it misbehaves? Worth commenting.

Quoted 68 lines

drivers/net/can/sja1000/kvaser_pci.c new file +68 -0

+	} else {
+		struct sja1000_priv *master_priv = netdev_priv(*master_dev);
+		struct kvaser_pci *master_board = master_priv->priv;
+		master_board->slave_dev[channel - 1] = dev;
+		master_board->no_channels = channel + 1;
+		board->xilinx_ver = master_board->xilinx_ver;
+	}
+
+	dev->base_addr = base_addr + channel * KVASER_PCI_PORT_BYTES;
+
+	priv->read_reg = kvaser_pci_read_reg;
+	priv->write_reg = kvaser_pci_write_reg;
+
+	priv->can.clock.freq = KVASER_PCI_CAN_CLOCK;
+
+	priv->ocr = KVASER_PCI_OCR;
+	priv->cdr = KVASER_PCI_CDR;
+
+	/* Register and setup interrupt handling */
+	dev->irq = pdev->irq;
+	init_step = 4;
+
+	dev_info(&pdev->dev, "base_addr=%#lx conf_addr=%p irq=%d\n",
+		 dev->base_addr, board->conf_addr, dev->irq);
+
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	/* Register SJA1000 device */
+	err = register_sja1000dev(dev);
+	if (err) {
+		dev_err(&pdev->dev, "Registering device failed (err=%d)\n",
+			err);
+		goto failure;
+	}
+
+	if (channel == 0)
+		*master_dev = dev;
+
+	return 0;
+
+failure:
+	kvaser_pci_del_chan(dev);
+	return err;
+}
+
+static int __devinit kvaser_pci_init_one(struct pci_dev *pdev,
+					 const struct pci_device_id *ent)
+{
+	int err;
+	struct net_device *master_dev = NULL;
+	struct sja1000_priv *priv;
+	struct kvaser_pci *board;
+	int no_channels;
+	void __iomem *base_addr = NULL;
+	void __iomem *conf_addr = NULL;
+	void __iomem *res_addr = NULL;
+	int i;
+
+	dev_info(&pdev->dev, "initializing device %04x:%04x\n",
+		 pdev->vendor, pdev->device);
+
+	err = pci_enable_device(pdev);
+	if (err)
+		goto failure;
+
+	err = pci_request_regions(pdev, DRV_NAME);
+	if (err)
+		goto failure_release_pci;

drivers/net/can/sja1000/kvaser_pci.c line 326

Quoted 6 lines

drivers/net/can/sja1000/kvaser_pci.c new file +6 -0

+
+	/*S5920*/
+	conf_addr = pci_iomap(pdev, 0, PCI_CONFIG_PORT_SIZE);
+	if (conf_addr == NULL) {
+		err = -ENODEV;
+		goto failure_iounmap;

Why go there? You know there's nothing to unmap.