[PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean]

This patch adds the support of the PCAN-PC Card (PCMCIA) card from
PEAK-System Technik (http://www.peak-system.com). The PCAN-PC Card is
sja1000 based and exists in 1 or 2 channels.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
v3 changes:
- fix an uninitialized  warning issue on 'err'

 drivers/net/can/sja1000/Kconfig       |    9 +
 drivers/net/can/sja1000/Makefile      |    1 +
 drivers/net/can/sja1000/peak_pcmcia.c |  721 +++++++++++++++++++++++++++++++++
 3 files changed, 731 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/sja1000/peak_pcmcia.c

diff --git a/drivers/net/can/sja1000/Kconfig b/drivers/net/can/sja1000/Kconfig
index 8116336..e6b0d6a 100644
--- a/drivers/net/can/sja1000/Kconfig
+++ b/drivers/net/can/sja1000/Kconfig
@@ -43,6 +43,15 @@ config CAN_EMS_PCI
 	  CPC-PCIe and CPC-104P cards from EMS Dr. Thomas Wuensche
 	  (http://www.ems-wuensche.de).
 
+config CAN_PEAK_PCMCIA
+	tristate "PEAK PCAN-PC Card"
+	depends on PCMCIA
+	---help---
+	  This driver is for the PCAN-PC Card PCMCIA adapter (1 or 2 channels)
+	  from PEAK-System Technik (http://www.peak-system.com).
+	  To compile this driver as a module, choose M here: the module will
+	  be called peak_pcmcia.
+
 config CAN_PEAK_PCI
 	tristate "PEAK PCAN-PCI/PCIe/miniPCI Cards"
 	depends on PCI
diff --git a/drivers/net/can/sja1000/Makefile b/drivers/net/can/sja1000/Makefile
index 0604f24..b3d05cb 100644
--- a/drivers/net/can/sja1000/Makefile
+++ b/drivers/net/can/sja1000/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += sja1000_of_platform.o
 obj-$(CONFIG_CAN_EMS_PCMCIA) += ems_pcmcia.o
 obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o
 obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o
+obj-$(CONFIG_CAN_PEAK_PCMCIA) += peak_pcmcia.o
 obj-$(CONFIG_CAN_PEAK_PCI) += peak_pci.o
 obj-$(CONFIG_CAN_PLX_PCI) += plx_pci.o
 obj-$(CONFIG_CAN_TSCAN1) += tscan1.o
diff --git a/drivers/net/can/sja1000/peak_pcmcia.c b/drivers/net/can/sja1000/peak_pcmcia.c
new file mode 100644
index 0000000..c832848
--- /dev/null
+++ b/drivers/net/can/sja1000/peak_pcmcia.c
@@ -0,0 +1,721 @@
+/*
+ * Copyright (C) 2010-2012 Stephane Grosjean <s.grosjean@peak-system.com>
+ *
+ * CAN driver for PEAK-System PCAN-PC Card
+ * Derived from the PCAN project file driver/src/pcan_pccard.c
+ * Copyright (C) 2006-2010 PEAK System-Technik GmbH
+ *
+ * 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.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/timer.h>
+#include <linux/io.h>
+#include <pcmcia/cistpl.h>
+#include <pcmcia/ds.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include "sja1000.h"
+
+MODULE_AUTHOR("Stephane Grosjean <s.grosjean@peak-system.com>");
+MODULE_DESCRIPTION("CAN driver for PEAK-System PCAN-PC Cards");
+MODULE_LICENSE("GPL v2");
+MODULE_SUPPORTED_DEVICE("PEAK PCAN-PC Card");
+
+/* PEAK-System PCMCIA driver name */
+#define PCC_NAME		"peak_pcmcia"
+
+#define PCC_CHAN_MAX		2
+
+#define PCC_CAN_CLOCK		(16000000 / 2)
+
+#define PCC_MANF_ID		0x0377
+#define PCC_CARD_ID		0x0001
+
+#define PCC_CHAN_SIZE		0x20
+#define PCC_CHAN_OFF(c)		((c) * PCC_CHAN_SIZE)
+#define PCC_COMN_OFF		(PCC_CHAN_OFF(PCC_CHAN_MAX))
+#define PCC_COMN_SIZE		0x40
+
+/* common area registers */
+#define PCC_CCR			0x00
+#define PCC_CSR			0x02
+#define PCC_CPR			0x04
+#define PCC_SPI_DIR		0x06
+#define PCC_SPI_DOR		0x08
+#define PCC_SPI_ADR		0x0a
+#define PCC_SPI_IR		0x0c
+#define PCC_FW_MAJOR		0x10
+#define PCC_FW_MINOR		0x12
+
+/* CCR bits */
+#define PCC_CCR_CLK_16		0x00
+#define PCC_CCR_CLK_10		0x01
+#define PCC_CCR_CLK_21		0x02
+#define PCC_CCR_CLK_8		0x03
+#define PCC_CCR_CLK_MASK	PCC_CCR_CLK_8
+
+#define PCC_CCR_RST_CHAN(c)	(0x01 << ((c) + 2))
+#define PCC_CCR_RST_ALL		(PCC_CCR_RST_CHAN(0) | PCC_CCR_RST_CHAN(1))
+#define PCC_CCR_RST_MASK	PCC_CCR_RST_ALL
+
+/* led selection bits */
+#define PCC_LED(c)		(1 << (c))
+#define PCC_LED_ALL		(PCC_LED(0) | PCC_LED(1))
+
+/* led state value */
+#define PCC_LED_ON		0x00
+#define PCC_LED_FAST		0x01
+#define PCC_LED_SLOW		0x02
+#define PCC_LED_OFF		0x03
+
+#define PCC_CCR_LED_CHAN(s, c)	((s) << (((c) + 2) << 1))
+
+#define PCC_CCR_LED_ON_CHAN(c)		PCC_CCR_LED_CHAN(PCC_LED_ON, c)
+#define PCC_CCR_LED_FAST_CHAN(c)	PCC_CCR_LED_CHAN(PCC_LED_FAST, c)
+#define PCC_CCR_LED_SLOW_CHAN(c)	PCC_CCR_LED_CHAN(PCC_LED_SLOW, c)
+#define PCC_CCR_LED_OFF_CHAN(c)		PCC_CCR_LED_CHAN(PCC_LED_OFF, c)
+#define PCC_CCR_LED_MASK_CHAN(c)	PCC_CCR_LED_OFF_CHAN(c)
+#define PCC_CCR_LED_OFF_ALL		(PCC_CCR_LED_OFF_CHAN(0) | \
+					 PCC_CCR_LED_OFF_CHAN(1))
+#define PCC_CCR_LED_MASK		PCC_CCR_LED_OFF_ALL
+
+#define PCC_CCR_INIT	(PCC_CCR_CLK_16 | PCC_CCR_RST_ALL | PCC_CCR_LED_OFF_ALL)
+
+/* CSR bits */
+#define PCC_CSR_SPI_BUSY		0x04
+
+#define PCC_SPI_MAX_BUSY_WAIT_MS	3	/* time waiting for spi !busy */
+
+/* max count of reading the SPI status register waiting for a change */
+#define PCC_WRITE_MAX_LOOP		1000
+
+/* max nb of int handled by that isr in one shot (prevent from infinite loop) */
+#define PCC_ISR_MAX_LOOP		10
+
+/* EEPROM chip instruction set */
+/* note: EEPROM Read/Write instructions include A8 bit */
+#define PCC_EEP_WRITE(a)	(0x02 | (((a) & 0x100) >> 5))
+#define PCC_EEP_READ(a)		(0x03 | (((a) & 0x100) >> 5))
+#define PCC_EEP_WRDI		0x04	/* EEPROM Write Disable */
+#define PCC_EEP_RDSR		0x05	/* EEPROM Read Status Register */
+#define PCC_EEP_WREN		0x06	/* EEPROM Write Enable */
+
+/* EEPROM Status Register bits */
+#define PCC_EEP_SR_WEN		0x02	/* EEPROM SR Write Enable bit */
+#define PCC_EEP_SR_WIP		0x01	/* EEPROM SR Write In Progress bit */
+
+/*
+ * The board configuration is probably following:
+ * RX1 is connected to ground.
+ * TX1 is not connected.
+ * CLKO is not connected.
+ * Setting the OCR register to 0xDA is a good idea.
+ * This means normal output mode, push-pull and the correct polarity.
+ */
+#define PCC_OCR			(OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL)
+
+/*
+ * In the CDR register, you should set CBP to 1.
+ * You will probably also want to set the clock divider value to 7
+ * (meaning direct oscillator output) because the second SJA1000 chip
+ * is driven by the first one CLKOUT output.
+ */
+#define PCC_CDR			(CDR_CBP | CDR_CLKOUT_MASK)
+
+struct pcan_channel {
+	struct net_device *netdev;
+	unsigned long prev_rx_bytes;
+	unsigned long prev_tx_bytes;
+};
+
+/* PCAN-PC Card private structure */
+struct pcan_pccard {
+	struct pcmcia_device *pdev;
+	int chan_count;
+	struct pcan_channel channel[PCC_CHAN_MAX];
+	u8 ccr;
+	void __iomem *ioport_addr;
+	struct timer_list led_timer;
+};
+
+static struct pcmcia_device_id pcan_table[] = {
+	PCMCIA_DEVICE_MANF_CARD(PCC_MANF_ID, PCC_CARD_ID),
+	PCMCIA_DEVICE_NULL,
+};
+
+MODULE_DEVICE_TABLE(pcmcia, pcan_table);
+
+static void pcan_set_leds(struct pcan_pccard *card, u8 mask, u8 state);
+
+/*
+ * start timer which controls leds state
+ */
+static void pcan_start_led_timer(struct pcan_pccard *card)
+{
+	if (!timer_pending(&card->led_timer))
+		mod_timer(&card->led_timer, jiffies + HZ);
+}
+
+/*
+ * stop the timer which controls leds state
+ */
+static void pcan_stop_led_timer(struct pcan_pccard *card)
+{
+	del_timer_sync(&card->led_timer);
+}
+
+/*
+ * read a sja1000 register
+ */
+static u8 pcan_read_canreg(const struct sja1000_priv *priv, int port)
+{
+	return ioread8(priv->reg_base + port);
+}
+
+/*
+ * write a sja1000 register
+ */
+static void pcan_write_canreg(const struct sja1000_priv *priv, int port, u8 v)
+{
+	struct pcan_pccard *card = priv->priv;
+	int c = (priv->reg_base - card->ioport_addr) / PCC_CHAN_SIZE;
+
+	/* sja1000 register changes control the leds state */
+	if (port == REG_MOD)
+		switch (v) {
+		case MOD_RM:
+			/* Reset Mode: set led on */
+			pcan_set_leds(card, PCC_LED(c), PCC_LED_ON);
+			break;
+		case 0x00:
+			/* Normal Mode: led slow blinking and start led timer */
+			pcan_set_leds(card, PCC_LED(c), PCC_LED_SLOW);
+			pcan_start_led_timer(card);
+			break;
+		default:
+			break;
+		}
+
+	iowrite8(v, priv->reg_base + port);
+}
+
+/*
+ * read a register from the common area
+ */
+static u8 pcan_read_reg(struct pcan_pccard *card, int port)
+{
+	return ioread8(card->ioport_addr + PCC_COMN_OFF + port);
+}
+
+/*
+ * write a register into the common area
+ */
+static void pcan_write_reg(struct pcan_pccard *card, int port, u8 v)
+{
+	/* cache ccr value */
+	if (port == PCC_CCR) {
+		if (card->ccr == v)
+			return;
+		card->ccr = v;
+	}
+
+	iowrite8(v, card->ioport_addr + PCC_COMN_OFF + port);
+}
+
+/*
+ * wait for SPI engine while it is busy
+ */
+static int pcan_wait_spi_busy(struct pcan_pccard *card)
+{
+	unsigned long timeout = jiffies +
+				msecs_to_jiffies(PCC_SPI_MAX_BUSY_WAIT_MS) + 1;
+
+	/* be sure to read status at least once after sleeping */
+	while (pcan_read_reg(card, PCC_CSR) & PCC_CSR_SPI_BUSY) {
+		if (jiffies >= timeout)
+			return -EBUSY;
+		schedule();
+	}
+
+	return 0;
+}
+
+/*
+ * write data in device eeprom
+ */
+static int pcan_write_eeprom(struct pcan_pccard *card, u16 addr, u8 v)
+{
+	u8 status;
+	int err, i;
+
+	/* write instruction enabling write */
+	pcan_write_reg(card, PCC_SPI_IR, PCC_EEP_WREN);
+	err = pcan_wait_spi_busy(card);
+	if (err)
+		goto we_spi_err;
+
+	/* wait until write enabled */
+	for (i = 0; i < PCC_WRITE_MAX_LOOP; i++) {
+		/* write instruction reading the status register */
+		pcan_write_reg(card, PCC_SPI_IR, PCC_EEP_RDSR);
+		err = pcan_wait_spi_busy(card);
+		if (err)
+			goto we_spi_err;
+
+		/* get status register value and check write enable bit */
+		status = pcan_read_reg(card, PCC_SPI_DIR);
+		if (status & PCC_EEP_SR_WEN)
+			break;
+	}
+
+	if (i >= PCC_WRITE_MAX_LOOP) {
+		dev_err(&card->pdev->dev,
+			"stop waiting to be allowed to write in eeprom\n");
+		return -EIO;
+	}
+
+	/* set address and data */
+	pcan_write_reg(card, PCC_SPI_ADR, addr & 0xff);
+	pcan_write_reg(card, PCC_SPI_DOR, v);
+
+	/*
+	 * write instruction with bit[3] set according to address value:
+	 * if addr refers to upper half of the memory array: bit[3] = 1
+	 */
+	pcan_write_reg(card, PCC_SPI_IR, PCC_EEP_WRITE(addr));
+	err = pcan_wait_spi_busy(card);
+	if (err)
+		goto we_spi_err;
+
+	/* wait while write in progress */
+	for (i = 0; i < PCC_WRITE_MAX_LOOP; i++) {
+		/* write instruction reading the status register */
+		pcan_write_reg(card, PCC_SPI_IR, PCC_EEP_RDSR);
+		err = pcan_wait_spi_busy(card);
+		if (err)
+			goto we_spi_err;
+
+		/* get status register value and check write in progress bit */
+		status = pcan_read_reg(card, PCC_SPI_DIR);
+		if (!(status & PCC_EEP_SR_WIP))
+			break;
+	}
+
+	if (i >= PCC_WRITE_MAX_LOOP) {
+		dev_err(&card->pdev->dev,
+			"stop waiting for write in eeprom to complete\n");
+		return -EIO;
+	}
+
+	/* write instruction disabling write */
+	pcan_write_reg(card, PCC_SPI_IR, PCC_EEP_WRDI);
+	err = pcan_wait_spi_busy(card);
+	if (err)
+		goto we_spi_err;
+
+	return 0;
+
+we_spi_err:
+	dev_err(&card->pdev->dev,
+		"stop waiting (spi engine always busy) err %d\n", err);
+
+	return err;
+}
+
+static void pcan_set_leds(struct pcan_pccard *card, u8 led_mask, u8 state)
+{
+	u8 ccr = card->ccr;
+	int i;
+
+	for (i = 0; i < card->chan_count; i++)
+		if (led_mask & PCC_LED(i)) {
+			/* clear corresponding led bits in ccr */
+			ccr &= ~PCC_CCR_LED_MASK_CHAN(i);
+			/* then set new bits */
+			ccr |= PCC_CCR_LED_CHAN(state, i);
+		}
+
+	/* real write only if something has changed in ccr */
+	pcan_write_reg(card, PCC_CCR, ccr);
+}
+
+/*
+ * enable/disable CAN connectors power
+ */
+static inline void pcan_set_can_power(struct pcan_pccard *card, int onoff)
+{
+	int err = pcan_write_eeprom(card, 0, (onoff) ? 1 : 0);
+	if (err)
+		dev_err(&card->pdev->dev,
+			"failed setting power %s to can connectors (err %d)\n",
+			(onoff) ? "on" : "off", err);
+}
+
+/*
+ * set leds state according to channel activity
+ */
+static void pcan_led_timer(unsigned long arg)
+{
+	struct pcan_pccard *card = (struct pcan_pccard *)arg;
+	struct net_device *netdev;
+	u8 ccr = card->ccr;
+	int i, up_count = 0;
+
+	for (i = 0; i < card->chan_count; i++) {
+		/* default is: not configured */
+		ccr &= ~PCC_CCR_LED_MASK_CHAN(i);
+		ccr |= PCC_CCR_LED_ON_CHAN(i);
+
+		netdev = card->channel[i].netdev;
+		if (!netdev || !(netdev->flags & IFF_UP))
+			continue;
+
+		up_count++;
+
+		/* no activity (but configured) */
+		ccr &= ~PCC_CCR_LED_MASK_CHAN(i);
+		ccr |= PCC_CCR_LED_SLOW_CHAN(i);
+
+		/* if bytes counters changed, set fast blinking led */
+		if (netdev->stats.rx_bytes != card->channel[i].prev_rx_bytes) {
+			card->channel[i].prev_rx_bytes = netdev->stats.rx_bytes;
+			ccr &= ~PCC_CCR_LED_MASK_CHAN(i);
+			ccr |= PCC_CCR_LED_FAST_CHAN(i);
+		}
+		if (netdev->stats.tx_bytes != card->channel[i].prev_tx_bytes) {
+			card->channel[i].prev_tx_bytes = netdev->stats.tx_bytes;
+			ccr &= ~PCC_CCR_LED_MASK_CHAN(i);
+			ccr |= PCC_CCR_LED_FAST_CHAN(i);
+		}
+	}
+
+	/* write the new leds state */
+	pcan_write_reg(card, PCC_CCR, ccr);
+
+	/* restart timer (except if no more configured channels) */
+	if (up_count)
+		mod_timer(&card->led_timer, jiffies + HZ);
+}
+
+/*
+ * interrupt service routine
+ */
+static irqreturn_t pcan_isr(int irq, void *dev_id)
+{
+	struct pcan_pccard *card = dev_id;
+	int irq_handled;
+
+	/* prevent from infinite loop */
+	for (irq_handled = 0; irq_handled < PCC_ISR_MAX_LOOP; irq_handled++) {
+		/* handle shared interrupt and next pass */
+		int nothing_to_handle = 1;
+		int i;
+
+		/* check interrupt for each channel */
+		for (i = 0; i < card->chan_count; i++) {
+			/* be sure the netdevice always exists */
+			struct net_device *netdev = card->channel[i].netdev;
+			if (!netdev)
+				continue;
+
+			/*
+			 * if some interrupt from the controller handled,
+			 * should check whether all have been handled or if have
+			 * been stopped because of SJA1000_MAX_IRQ
+			 */
+			if (sja1000_interrupt(irq, netdev) == IRQ_HANDLED)
+				nothing_to_handle = 0;
+		}
+
+		if (nothing_to_handle)
+			break;
+	}
+
+	return (irq_handled) ? IRQ_HANDLED : IRQ_NONE;
+}
+
+/*
+ * free all resources used by the channels and switch off leds and can power
+ */
+static void pcan_free_channels(struct pcan_pccard *card)
+{
+	int i;
+	u8 led_mask = 0;
+
+	for (i = 0; i < card->chan_count; i++) {
+		struct net_device *netdev;
+		char name[IFNAMSIZ];
+
+		led_mask |= PCC_LED(i);
+
+		netdev = card->channel[i].netdev;
+		if (!netdev)
+			continue;
+
+		strncpy(name, netdev->name, IFNAMSIZ);
+		unregister_sja1000dev(netdev);
+		free_sja1000dev(netdev);
+
+		dev_info(&card->pdev->dev, "%s removed\n", name);
+	}
+
+	if (pcmcia_dev_present(card->pdev)) {
+		pcan_set_leds(card, led_mask, PCC_LED_OFF);
+		pcan_set_can_power(card, 0);
+	}
+
+	ioport_unmap(card->ioport_addr);
+}
+
+/*
+ * check if a CAN controller is present at the specified location
+ */
+static inline int pcan_check_channel(struct sja1000_priv *priv)
+{
+	/* make sure SJA1000 is in reset mode */
+	pcan_write_canreg(priv, REG_MOD, 1);
+	pcan_write_canreg(priv, REG_CDR, CDR_PELICAN);
+
+	/* read reset-values */
+	if (pcan_read_canreg(priv, REG_CDR) == CDR_PELICAN)
+		return 1;
+
+	return 0;
+}
+
+static int pcan_add_channels(struct pcan_pccard *card)
+{
+	struct pcmcia_device *pdev = card->pdev;
+	int i, err = 0;
+	u8 ccr = PCC_CCR_INIT;
+
+	/* init common registers (reset channels and leds off) */
+	card->ccr = ~ccr;
+	pcan_write_reg(card, PCC_CCR, ccr);
+
+	/* wait 2ms before unresetting channels */
+	mdelay(2);
+
+	ccr &= ~PCC_CCR_RST_ALL;
+	pcan_write_reg(card, PCC_CCR, ccr);
+
+	/* create one network device per channel detected */
+	for (i = 0; i < ARRAY_SIZE(card->channel); i++) {
+		struct net_device *netdev;
+		struct sja1000_priv *priv;
+
+		netdev = alloc_sja1000dev(0);
+		if (!netdev) {
+			err = -ENOMEM;
+			break;
+		}
+
+		/* update linkages */
+		priv = netdev_priv(netdev);
+		priv->priv = card;
+		SET_NETDEV_DEV(netdev, &pdev->dev);
+
+		priv->irq_flags = IRQF_SHARED;
+		netdev->irq = pdev->irq;
+		priv->reg_base = card->ioport_addr + PCC_CHAN_OFF(i);
+
+		/* check if channel is present */
+		if (!pcan_check_channel(priv)) {
+			dev_err(&pdev->dev, "channel %d not present\n", i);
+			free_sja1000dev(netdev);
+			continue;
+		}
+
+		priv->read_reg  = pcan_read_canreg;
+		priv->write_reg = pcan_write_canreg;
+		priv->can.clock.freq = PCC_CAN_CLOCK;
+		priv->ocr = PCC_OCR;
+		priv->cdr = PCC_CDR;
+
+		/* Neither a slave device distributes the clock */
+		if (i > 0)
+			priv->cdr |= CDR_CLK_OFF;
+
+		priv->flags |= SJA1000_CUSTOM_IRQ_HANDLER;
+
+		/* register SJA1000 device */
+		err = register_sja1000dev(netdev);
+		if (err) {
+			free_sja1000dev(netdev);
+			continue;
+		}
+
+		card->channel[i].netdev = netdev;
+		card->chan_count++;
+
+		/* set corresponding led on in the new ccr */
+		ccr &= ~PCC_CCR_LED_OFF_CHAN(i);
+
+		dev_info(&pdev->dev,
+			"registered %s on channel %d at 0x%p irq %d\n",
+			netdev->name, i, priv->reg_base, pdev->irq);
+	}
+
+	/* write new ccr (change leds state) */
+	pcan_write_reg(card, PCC_CCR, ccr);
+
+	return err;
+}
+
+static int pcan_conf_check(struct pcmcia_device *pdev, void *priv_data)
+{
+	pdev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
+	pdev->resource[0]->flags |= IO_DATA_PATH_WIDTH_8; /* only */
+	pdev->io_lines = 10;
+
+	/* This reserves IO space but doesn't actually enable it */
+	return pcmcia_request_io(pdev);
+}
+
+/*
+ * free all resources used by the device
+ */
+static void pcan_free(struct pcmcia_device *pdev)
+{
+	if (!pdev->priv)
+		return;
+
+	pcan_stop_led_timer(pdev->priv);
+	free_irq(pdev->irq, pdev->priv);
+	pcan_free_channels(pdev->priv);
+
+	kfree(pdev->priv);
+	pdev->priv = NULL;
+}
+
+/*
+ * setup PCMCIA socket and probe for PEAK-System PC-CARD
+ */
+static int __devinit pcan_probe(struct pcmcia_device *pdev)
+{
+	struct pcan_pccard *card;
+	int err;
+
+	pdev->config_flags |= CONF_ENABLE_IRQ | CONF_AUTO_SET_IO;
+
+	err = pcmcia_loop_config(pdev, pcan_conf_check, NULL);
+	if (err) {
+		dev_err(&pdev->dev, "pcmcia_loop_config() error %d\n", err);
+		goto probe_err_1;
+	}
+
+	dev_info(&pdev->dev, "new PEAK-System pcmcia card detected: %s %s:\n",
+		pdev->prod_id[0] ? pdev->prod_id[0] : "PEAK-System",
+		pdev->prod_id[1] ? pdev->prod_id[1] : "PCAN-PC Card");
+
+	if (!pdev->irq) {
+		dev_err(&pdev->dev, "no irq assigned\n");
+		err = -ENODEV;
+		goto probe_err_1;
+	}
+
+	err = pcmcia_enable_device(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "pcmcia_enable_device failed err=%d\n",
+			err);
+		goto probe_err_1;
+	}
+
+	card = kzalloc(sizeof(struct pcan_pccard), GFP_KERNEL);
+	if (!card) {
+		dev_err(&pdev->dev, "couldn't allocated card memory\n");
+		err = -ENOMEM;
+		goto probe_err_2;
+	}
+
+	card->pdev = pdev;
+	pdev->priv = card;
+
+	/* sja1000 api uses iomem */
+	card->ioport_addr = ioport_map(pdev->resource[0]->start,
+					resource_size(pdev->resource[0]));
+	if (!card->ioport_addr) {
+		err = -ENOMEM;
+		goto probe_err_3;
+	}
+
+	/* display firware version */
+	dev_info(&pdev->dev, "firmware %d.%d\n",
+		pcan_read_reg(card, PCC_FW_MAJOR),
+		pcan_read_reg(card, PCC_FW_MINOR));
+
+	/* detect available channels */
+	pcan_add_channels(card);
+	if (!card->chan_count) {
+		ioport_unmap(card->ioport_addr);
+		goto probe_err_3;
+	}
+
+	/* init the timer which controls the leds */
+	init_timer(&card->led_timer);
+	card->led_timer.function = pcan_led_timer;
+	card->led_timer.data = (unsigned long)card;
+
+	/* request the given irq */
+	err = request_irq(pdev->irq, &pcan_isr, IRQF_SHARED, PCC_NAME, card);
+	if (err)
+		goto probe_err_4;
+
+	/* power on the connectors */
+	pcan_set_can_power(card, 1);
+
+	return 0;
+
+probe_err_4:
+	/* unregister can devices from network */
+	pcan_free_channels(card);
+
+probe_err_3:
+	kfree(card);
+	pdev->priv = NULL;
+
+probe_err_2:
+	pcmcia_disable_device(pdev);
+
+probe_err_1:
+	return err;
+}
+
+/*
+ * release claimed resources
+ */
+static void pcan_remove(struct pcmcia_device *pdev)
+{
+	pcan_free(pdev);
+	pcmcia_disable_device(pdev);
+}
+
+static struct pcmcia_driver pcan_driver = {
+	.name = PCC_NAME,
+	.probe = pcan_probe,
+	.remove = pcan_remove,
+	.id_table = pcan_table,
+};
+
+static int __init pcan_init(void)
+{
+	return pcmcia_register_driver(&pcan_driver);
+}
+module_init(pcan_init);
+
+static void __exit pcan_exit(void)
+{
+	pcmcia_unregister_driver(&pcan_driver);
+}
+module_exit(pcan_exit);
-- 
1.7.5.4

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On 02/06/2012 04:56 PM, Stephane Grosjean wrote:
> This patch adds the support of the PCAN-PC Card (PCMCIA) card from
> PEAK-System Technik (http://www.peak-system.com). The PCAN-PC Card is
> sja1000 based and exists in 1 or 2 channels.
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>

What's the status of this patch?

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean]

Hello all,

I copied&pasted everything in this single mail to simplify...

Le 13/02/2012 10:14, Marc Kleine-Budde a écrit :
> On 02/06/2012 04:56 PM, Stephane Grosjean wrote:
>> This patch adds the support of the PCAN-PC Card (PCMCIA) card from
>> PEAK-System Technik (http://www.peak-system.com). The PCAN-PC Card is
>> sja1000 based and exists in 1 or 2 channels.
>>
>> Signed-off-by: Stephane Grosjean<s.grosjean@peak-system.com>
> What's the status of this patch?
>

See below please...

> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index ebbcfca..f7526a7 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>                  n++;
>                  status = priv->read_reg(priv, REG_SR);
>
> +               /* check for absent controller due to hw unplug */
> +               if (status == 0xFF)
> +                       break;
> +
>                  if (isrc&  IRQ_WUI)
>                          netdev_warn(dev, "wakeup interrupt\n");
>
> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>                          netif_wake_queue(dev);
>                  }
>                  if (isrc&  IRQ_RI) {
> -                       /* receive interrupt */
> -                       while (status&  SR_RBS) {
> +                       /* receive interrupt / check for absent controller */
> +                       while (status&  SR_RBS&&  status != 0xFF) {
>                                  sja1000_rx(dev);
>                                  status = priv->read_reg(priv, REG_SR);
>                          }
>
> @Stephane: Can you check that patch? I'm out of hw right now.

I confirm that this patch works too...
So I think I should be able to post a new version of the peak_pcmcia 
during that day (the previous should work but some calls to 
pcmcia_dev_present() are no more useful...)

Stéphane

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

On 02/13/2012 11:01 AM, Stephane Grosjean wrote:
> Hello all,
> 
> I copied&pasted everything in this single mail to simplify...
> 
> Le 13/02/2012 10:14, Marc Kleine-Budde a écrit :
>> On 02/06/2012 04:56 PM, Stephane Grosjean wrote:
>>> This patch adds the support of the PCAN-PC Card (PCMCIA) card from
>>> PEAK-System Technik (http://www.peak-system.com). The PCAN-PC Card is
>>> sja1000 based and exists in 1 or 2 channels.
>>>
>>> Signed-off-by: Stephane Grosjean<s.grosjean@peak-system.com>
>> What's the status of this patch?
>>
> 
> See below please...
> 
>> diff --git a/drivers/net/can/sja1000/sja1000.c
>> b/drivers/net/can/sja1000/sja1000.c
>> index ebbcfca..f7526a7 100644
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>                  n++;
>>                  status = priv->read_reg(priv, REG_SR);
>>
>> +               /* check for absent controller due to hw unplug */
>> +               if (status == 0xFF)
>> +                       break;
>> +
>>                  if (isrc&  IRQ_WUI)
>>                          netdev_warn(dev, "wakeup interrupt\n");
>>
>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>                          netif_wake_queue(dev);
>>                  }
>>                  if (isrc&  IRQ_RI) {
>> -                       /* receive interrupt */
>> -                       while (status&  SR_RBS) {
>> +                       /* receive interrupt / check for absent
>> controller */
>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>                                  sja1000_rx(dev);
>>                                  status = priv->read_reg(priv, REG_SR);
>>                          }
>>
>> @Stephane: Can you check that patch? I'm out of hw right now.
> 
> I confirm that this patch works too...
> So I think I should be able to post a new version of the peak_pcmcia
> during that day (the previous should work but some calls to
> pcmcia_dev_present() are no more useful...)

But that fix should not go to the common interrupt handler, if possible.

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp]

On 13.02.2012 11:14, Wolfgang Grandegger wrote:


>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>> b/drivers/net/can/sja1000/sja1000.c
>>> index ebbcfca..f7526a7 100644
>>> --- a/drivers/net/can/sja1000/sja1000.c
>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>                  n++;
>>>                  status = priv->read_reg(priv, REG_SR);
>>>
>>> +               /* check for absent controller due to hw unplug */
>>> +               if (status == 0xFF)
>>> +                       break;
>>> +
>>>                  if (isrc&  IRQ_WUI)
>>>                          netdev_warn(dev, "wakeup interrupt\n");
>>>
>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>                          netif_wake_queue(dev);
>>>                  }
>>>                  if (isrc&  IRQ_RI) {
>>> -                       /* receive interrupt */
>>> -                       while (status&  SR_RBS) {
>>> +                       /* receive interrupt / check for absent controller */
>>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>>                                  sja1000_rx(dev);
>>>                                  status = priv->read_reg(priv, REG_SR);
>>>                          }
>>>
>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>
>> I confirm that this patch works too...
>> So I think I should be able to post a new version of the peak_pcmcia
>> during that day (the previous should work but some calls to
>> pcmcia_dev_present() are no more useful...)
> 
> But that fix should not go to the common interrupt handler, if possible.


Do you have an idea how to handle the while() statement without copying the
entire interrupt handler code for the devices that might be unplugged?

IMHO this patch is pretty cheap.

Regards,
Oliver

ps. @Stephane: I just made a screencopy of my suggested patch - so there may
be whitepace issues then, sorry.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean]

Le 13/02/2012 11:14, Wolfgang Grandegger a écrit :
>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>> b/drivers/net/can/sja1000/sja1000.c
>>> index ebbcfca..f7526a7 100644
>>> --- a/drivers/net/can/sja1000/sja1000.c
>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>                   n++;
>>>                   status = priv->read_reg(priv, REG_SR);
>>>
>>> +               /* check for absent controller due to hw unplug */
>>> +               if (status == 0xFF)
>>> +                       break;
>>> +
>>>                   if (isrc&   IRQ_WUI)
>>>                           netdev_warn(dev, "wakeup interrupt\n");
>>>
>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>                           netif_wake_queue(dev);
>>>                   }
>>>                   if (isrc&   IRQ_RI) {
>>> -                       /* receive interrupt */
>>> -                       while (status&   SR_RBS) {
>>> +                       /* receive interrupt / check for absent
>>> controller */
>>> +                       while (status&   SR_RBS&&   status != 0xFF) {
>>>                                   sja1000_rx(dev);
>>>                                   status = priv->read_reg(priv, REG_SR);
>>>                           }
>>>
>>> @Stephane: Can you check that patch? I'm out of hw right now.
>> I confirm that this patch works too...
>> So I think I should be able to post a new version of the peak_pcmcia
>> during that day (the previous should work but some calls to
>> pcmcia_dev_present() are no more useful...)
> But that fix should not go to the common interrupt handler, if possible.

I suppose that the reason why you say that is because the 0xff value 
doesn't have got the special meaning of "potential unplug" (is it?), so 
I could understand why it's not appropriate to check it, in that common 
handler (perhaps the returned value is different in some other archs too).

But I don't agree with that potential infinite loop, testing the SR_RBS 
bit.

So, since we already are looping until SJA1000_MAX_IRQ, why not changing 
this:

                 if (isrc & IRQ_RI) {
                        /* receive interrupt */
                        while (status & SR_RBS) {
                                 sja1000_rx(dev);
                                 status = priv->read_reg(priv, REG_SR);
                         }

into something like that:

                 if (isrc & IRQ_RI) {
                        /* receive interrupt */
                        if (status & SR_RBS)
                                 sja1000_rx(dev);


In that case, even the 0xff value would not loop infinitely... Testing 
the device presence will be of the driver responsibility and could speed 
up unplug detection.

Stéphane


--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

On 02/13/2012 11:46 AM, Stephane Grosjean wrote:
> Le 13/02/2012 11:14, Wolfgang Grandegger a écrit :
>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>> b/drivers/net/can/sja1000/sja1000.c
>>>> index ebbcfca..f7526a7 100644
>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void
>>>> *dev_id)
>>>>                   n++;
>>>>                   status = priv->read_reg(priv, REG_SR);
>>>>
>>>> +               /* check for absent controller due to hw unplug */
>>>> +               if (status == 0xFF)
>>>> +                       break;
>>>> +
>>>>                   if (isrc&   IRQ_WUI)
>>>>                           netdev_warn(dev, "wakeup interrupt\n");
>>>>
>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void
>>>> *dev_id)
>>>>                           netif_wake_queue(dev);
>>>>                   }
>>>>                   if (isrc&   IRQ_RI) {
>>>> -                       /* receive interrupt */
>>>> -                       while (status&   SR_RBS) {
>>>> +                       /* receive interrupt / check for absent
>>>> controller */
>>>> +                       while (status&   SR_RBS&&   status != 0xFF) {
>>>>                                   sja1000_rx(dev);
>>>>                                   status = priv->read_reg(priv,
>>>> REG_SR);
>>>>                           }
>>>>
>>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>> I confirm that this patch works too...
>>> So I think I should be able to post a new version of the peak_pcmcia
>>> during that day (the previous should work but some calls to
>>> pcmcia_dev_present() are no more useful...)
>> But that fix should not go to the common interrupt handler, if possible.
> 
> I suppose that the reason why you say that is because the 0xff value
> doesn't have got the special meaning of "potential unplug" (is it?), so
> I could understand why it's not appropriate to check it, in that common
> handler (perhaps the returned value is different in some other archs too).

That's one reason. The other is that it's related to PCMCIA hotplug. The
check is not needed for other devices ==> overhead.

> But I don't agree with that potential infinite loop, testing the SR_RBS
> bit.
> 
> So, since we already are looping until SJA1000_MAX_IRQ, why not changing
> this:
> 
>                 if (isrc & IRQ_RI) {
>                        /* receive interrupt */
>                        while (status & SR_RBS) {
>                                 sja1000_rx(dev);
>                                 status = priv->read_reg(priv, REG_SR);
>                         }

We should add that patch, definetely. And the "check for absent
controller due to hw unplug" should go to the custum isr.

> into something like that:
> 
>                 if (isrc & IRQ_RI) {
>                        /* receive interrupt */
>                        if (status & SR_RBS)
>                                 sja1000_rx(dev);
> 
> 
> In that case, even the 0xff value would not loop infinitely... Testing
> the device presence will be of the driver responsibility and could speed
> up unplug detection.

But the SJA1000_MAX_IRQ is for another register (not for status). I
think we need two separated loops.

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

On 02/13/2012 11:41 AM, Oliver Hartkopp wrote:
> On 13.02.2012 11:14, Wolfgang Grandegger wrote:
> 
> 
>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>> b/drivers/net/can/sja1000/sja1000.c
>>>> index ebbcfca..f7526a7 100644
>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>                  n++;
>>>>                  status = priv->read_reg(priv, REG_SR);
>>>>
>>>> +               /* check for absent controller due to hw unplug */
>>>> +               if (status == 0xFF)
>>>> +                       break;
>>>> +
>>>>                  if (isrc&  IRQ_WUI)
>>>>                          netdev_warn(dev, "wakeup interrupt\n");
>>>>
>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>                          netif_wake_queue(dev);
>>>>                  }
>>>>                  if (isrc&  IRQ_RI) {
>>>> -                       /* receive interrupt */
>>>> -                       while (status&  SR_RBS) {
>>>> +                       /* receive interrupt / check for absent controller */
>>>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>>>                                  sja1000_rx(dev);
>>>>                                  status = priv->read_reg(priv, REG_SR);
>>>>                          }
>>>>
>>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>>
>>> I confirm that this patch works too...
>>> So I think I should be able to post a new version of the peak_pcmcia
>>> during that day (the previous should work but some calls to
>>> pcmcia_dev_present() are no more useful...)
>>
>> But that fix should not go to the common interrupt handler, if possible.
> 
> 
> Do you have an idea how to handle the while() statement without copying the
> entire interrupt handler code for the devices that might be unplugged?
> 
> IMHO this patch is pretty cheap.

I think it's enough to add a read in the custom pcmcia isr before
calling sja1000_interrupt(). Maybe the race window is a bit lower but we
are not able to react when the unplug happens anyway.

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2759 bytes --]

On 02/13/2012 12:02 PM, Wolfgang Grandegger wrote:
> On 02/13/2012 11:41 AM, Oliver Hartkopp wrote:
>> On 13.02.2012 11:14, Wolfgang Grandegger wrote:
>>
>>
>>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>>> b/drivers/net/can/sja1000/sja1000.c
>>>>> index ebbcfca..f7526a7 100644
>>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>                  n++;
>>>>>                  status = priv->read_reg(priv, REG_SR);
>>>>>
>>>>> +               /* check for absent controller due to hw unplug */
>>>>> +               if (status == 0xFF)
>>>>> +                       break;
>>>>> +
>>>>>                  if (isrc&  IRQ_WUI)
>>>>>                          netdev_warn(dev, "wakeup interrupt\n");
>>>>>
>>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>                          netif_wake_queue(dev);
>>>>>                  }
>>>>>                  if (isrc&  IRQ_RI) {
>>>>> -                       /* receive interrupt */
>>>>> -                       while (status&  SR_RBS) {
>>>>> +                       /* receive interrupt / check for absent controller */
>>>>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>>>>                                  sja1000_rx(dev);
>>>>>                                  status = priv->read_reg(priv, REG_SR);
>>>>>                          }
>>>>>
>>>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>>>
>>>> I confirm that this patch works too...
>>>> So I think I should be able to post a new version of the peak_pcmcia
>>>> during that day (the previous should work but some calls to
>>>> pcmcia_dev_present() are no more useful...)
>>>
>>> But that fix should not go to the common interrupt handler, if possible.
>>
>>
>> Do you have an idea how to handle the while() statement without copying the
>> entire interrupt handler code for the devices that might be unplugged?
>>
>> IMHO this patch is pretty cheap.
> 
> I think it's enough to add a read in the custom pcmcia isr before
> calling sja1000_interrupt(). Maybe the race window is a bit lower but we
> are not able to react when the unplug happens anyway.

It's not about reacting when an unlug happens, it's about not having the
interrupt handler to loop forever. At least we must limit the inner
while loop.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

On 02/13/2012 12:06 PM, Marc Kleine-Budde wrote:
> On 02/13/2012 12:02 PM, Wolfgang Grandegger wrote:
>> On 02/13/2012 11:41 AM, Oliver Hartkopp wrote:
>>> On 13.02.2012 11:14, Wolfgang Grandegger wrote:
>>>
>>>
>>>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>>>> b/drivers/net/can/sja1000/sja1000.c
>>>>>> index ebbcfca..f7526a7 100644
>>>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>                  n++;
>>>>>>                  status = priv->read_reg(priv, REG_SR);
>>>>>>
>>>>>> +               /* check for absent controller due to hw unplug */
>>>>>> +               if (status == 0xFF)
>>>>>> +                       break;
>>>>>> +
>>>>>>                  if (isrc&  IRQ_WUI)
>>>>>>                          netdev_warn(dev, "wakeup interrupt\n");
>>>>>>
>>>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>                          netif_wake_queue(dev);
>>>>>>                  }
>>>>>>                  if (isrc&  IRQ_RI) {
>>>>>> -                       /* receive interrupt */
>>>>>> -                       while (status&  SR_RBS) {
>>>>>> +                       /* receive interrupt / check for absent controller */
>>>>>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>>>>>                                  sja1000_rx(dev);
>>>>>>                                  status = priv->read_reg(priv, REG_SR);
>>>>>>                          }
>>>>>>
>>>>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>>>>
>>>>> I confirm that this patch works too...
>>>>> So I think I should be able to post a new version of the peak_pcmcia
>>>>> during that day (the previous should work but some calls to
>>>>> pcmcia_dev_present() are no more useful...)
>>>>
>>>> But that fix should not go to the common interrupt handler, if possible.
>>>
>>>
>>> Do you have an idea how to handle the while() statement without copying the
>>> entire interrupt handler code for the devices that might be unplugged?
>>>
>>> IMHO this patch is pretty cheap.
>>
>> I think it's enough to add a read in the custom pcmcia isr before
>> calling sja1000_interrupt(). Maybe the race window is a bit lower but we
>> are not able to react when the unplug happens anyway.
> 
> It's not about reacting when an unlug happens, it's about not having the
> interrupt handler to loop forever. At least we must limit the inner
> while loop.

Of course, the loop should be limited in sja1000_interrupt anyway.

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp]

On 13.02.2012 12:08, Wolfgang Grandegger wrote:

> On 02/13/2012 12:06 PM, Marc Kleine-Budde wrote:
>> On 02/13/2012 12:02 PM, Wolfgang Grandegger wrote:
>>> On 02/13/2012 11:41 AM, Oliver Hartkopp wrote:
>>>> On 13.02.2012 11:14, Wolfgang Grandegger wrote:
>>>>
>>>>
>>>>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>>>>> b/drivers/net/can/sja1000/sja1000.c
>>>>>>> index ebbcfca..f7526a7 100644
>>>>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>>                  n++;
>>>>>>>                  status = priv->read_reg(priv, REG_SR);
>>>>>>>
>>>>>>> +               /* check for absent controller due to hw unplug */
>>>>>>> +               if (status == 0xFF)
>>>>>>> +                       break;
>>>>>>> +
>>>>>>>                  if (isrc&  IRQ_WUI)
>>>>>>>                          netdev_warn(dev, "wakeup interrupt\n");
>>>>>>>
>>>>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>>                          netif_wake_queue(dev);
>>>>>>>                  }
>>>>>>>                  if (isrc&  IRQ_RI) {
>>>>>>> -                       /* receive interrupt */
>>>>>>> -                       while (status&  SR_RBS) {
>>>>>>> +                       /* receive interrupt / check for absent controller */
>>>>>>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>>>>>>                                  sja1000_rx(dev);
>>>>>>>                                  status = priv->read_reg(priv, REG_SR);
>>>>>>>                          }
>>>>>>>
>>>>>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>>>>>
>>>>>> I confirm that this patch works too...
>>>>>> So I think I should be able to post a new version of the peak_pcmcia
>>>>>> during that day (the previous should work but some calls to
>>>>>> pcmcia_dev_present() are no more useful...)
>>>>>
>>>>> But that fix should not go to the common interrupt handler, if possible.
>>>>
>>>>
>>>> Do you have an idea how to handle the while() statement without copying the
>>>> entire interrupt handler code for the devices that might be unplugged?
>>>>
>>>> IMHO this patch is pretty cheap.
>>>
>>> I think it's enough to add a read in the custom pcmcia isr before
>>> calling sja1000_interrupt(). Maybe the race window is a bit lower but we
>>> are not able to react when the unplug happens anyway.


Does this mean, it is 'less bad'?

>> It's not about reacting when an unlug happens, it's about not having the
>> interrupt handler to loop forever. At least we must limit the inner
>> while loop.


What happens, if inside this inner while() statement the unplug happens?

Do we get two correct CAN frames and then we get eight CAN frames like this:

12345677 4 AA BB CC DD
12345678 6 AA BB CC DD EE FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF
1FFFFFFF 8 FF FF FF FF FF FF FF FF

???

Sending nothing is preferred to sending wrong data :-)

> Of course, the loop should be limited in sja1000_interrupt anyway.


IMO checking for absent hardware in the custom isr is fine and should fix most
of the problems (as we can already see in the ems_pcmcia driver).

But adding the patch which is checking two times for status != 0xFF reduces
the race conditions to almost zero. Having a while() statement that
potentially produces wrong data doesn't heal the problem IMO.

Regards,
Oliver

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

On 02/13/2012 08:55 PM, Oliver Hartkopp wrote:
> On 13.02.2012 12:08, Wolfgang Grandegger wrote:
> 
>> On 02/13/2012 12:06 PM, Marc Kleine-Budde wrote:
>>> On 02/13/2012 12:02 PM, Wolfgang Grandegger wrote:
>>>> On 02/13/2012 11:41 AM, Oliver Hartkopp wrote:
>>>>> On 13.02.2012 11:14, Wolfgang Grandegger wrote:
>>>>>
>>>>>
>>>>>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>>>>>> b/drivers/net/can/sja1000/sja1000.c
>>>>>>>> index ebbcfca..f7526a7 100644
>>>>>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>>>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>>>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>>>                  n++;
>>>>>>>>                  status = priv->read_reg(priv, REG_SR);
>>>>>>>>
>>>>>>>> +               /* check for absent controller due to hw unplug */
>>>>>>>> +               if (status == 0xFF)
>>>>>>>> +                       break;
>>>>>>>> +
>>>>>>>>                  if (isrc&  IRQ_WUI)
>>>>>>>>                          netdev_warn(dev, "wakeup interrupt\n");
>>>>>>>>
>>>>>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>>>>                          netif_wake_queue(dev);
>>>>>>>>                  }
>>>>>>>>                  if (isrc&  IRQ_RI) {
>>>>>>>> -                       /* receive interrupt */
>>>>>>>> -                       while (status&  SR_RBS) {
>>>>>>>> +                       /* receive interrupt / check for absent controller */
>>>>>>>> +                       while (status&  SR_RBS&&  status != 0xFF) {
>>>>>>>>                                  sja1000_rx(dev);
>>>>>>>>                                  status = priv->read_reg(priv, REG_SR);
>>>>>>>>                          }
>>>>>>>>
>>>>>>>> @Stephane: Can you check that patch? I'm out of hw right now.
>>>>>>>
>>>>>>> I confirm that this patch works too...
>>>>>>> So I think I should be able to post a new version of the peak_pcmcia
>>>>>>> during that day (the previous should work but some calls to
>>>>>>> pcmcia_dev_present() are no more useful...)
>>>>>>
>>>>>> But that fix should not go to the common interrupt handler, if possible.
>>>>>
>>>>>
>>>>> Do you have an idea how to handle the while() statement without copying the
>>>>> entire interrupt handler code for the devices that might be unplugged?
>>>>>
>>>>> IMHO this patch is pretty cheap.
>>>>
>>>> I think it's enough to add a read in the custom pcmcia isr before
>>>> calling sja1000_interrupt(). Maybe the race window is a bit lower but we
>>>> are not able to react when the unplug happens anyway.
> 
> 
> Does this mean, it is 'less bad'?

No.

>>> It's not about reacting when an unlug happens, it's about not having the
>>> interrupt handler to loop forever. At least we must limit the inner
>>> while loop.
> 
> 
> What happens, if inside this inner while() statement the unplug happens?
> 
> Do we get two correct CAN frames and then we get eight CAN frames like this:
> 
> 12345677 4 AA BB CC DD
> 12345678 6 AA BB CC DD EE FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
> 
> ???

Do you have seen such traces with the "device present" check in the
custom handler? I doubt.

First it is unlikely, that the unplug will happen when data from the
device gets processed. And if, the check in the while loop will not
change a lot, in contrast to the check in the custom handler. If it
happens while processing the data, we will get grap... but well...

> Sending nothing is preferred to sending wrong data :-)

You cannot avoid sending crap under any circumstances. Also be aware
that the while loop will normally process just *one* message, and rarely
more.

>> Of course, the loop should be limited in sja1000_interrupt anyway.
> 
> 
> IMO checking for absent hardware in the custom isr is fine and should fix most
> of the problems (as we can already see in the ems_pcmcia driver).
> 
> But adding the patch which is checking two times for status != 0xFF reduces
> the race conditions to almost zero. Having a while() statement that
> potentially produces wrong data doesn't heal the problem IMO.

I disagree. It will not change a lot. Measurements may proof that I'm
wrong, though.

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean]

Le 13/02/2012 21:23, Wolfgang Grandegger a écrit :
> On 02/13/2012 08:55 PM, Oliver Hartkopp wrote:
>> What happens, if inside this inner while() statement the unplug happens?
>>
>> Do we get two correct CAN frames and then we get eight CAN frames like this:
>>
>> 12345677 4 AA BB CC DD
>> 12345678 6 AA BB CC DD EE FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>
>> ???
> Do you have seen such traces with the "device present" check in the
> custom handler? I doubt.

Well if I may interfere: since the driver "remove()" entry point is 
called and the netdev interface is destroyed next, nobody can truly says 
if these frames have time to be processed until the client application. 
The problem is not here.

> First it is unlikely, that the unplug will happen when data from the
> device gets processed. And if, the check in the while loop will not
> change a lot, in contrast to the check in the custom handler. If it
> happens while processing the data, we will get grap... but well...
>
>> Sending nothing is preferred to sending wrong data :-)
> You cannot avoid sending crap under any circumstances. Also be aware
> that the while loop will normally process just *one* message, and rarely
> more.

cannot say that, this is arch dependent.

>>> Of course, the loop should be limited in sja1000_interrupt anyway.
>>
>> IMO checking for absent hardware in the custom isr is fine and should fix most
>> of the problems (as we can already see in the ems_pcmcia driver).

cannot say that: all the tests I made @1xMbps bitrate show that the 
issue at least occurs 3/4 of time in the sja1000_interrupt, not in the 
custom isr. This also is obviously arch dependent.

>> But adding the patch which is checking two times for status != 0xFF reduces
>> the race conditions to almost zero. Having a while() statement that
>> potentially produces wrong data doesn't heal the problem IMO.
> I disagree. It will not change a lot. Measurements may proof that I'm
> wrong, though.

What about the fix Oliver talked about in one his first e-mail:

> The used status register has a bit for bus-off which is "1" in the bus-off
> case. Assuming RX interrupts can only occur in a 'bus-on' state, this bit can
> be assumed to be '0'.

IMHO, this is the best proposal: while the sja1000_interrupt() requests 
for the SR, it seems normal that it does some semantic checks about what 
it got.

So the fix could be something like this instead:

         while ((isrc = priv->read_reg(priv, REG_IR)) && (n < 
SJA1000_MAX_IRQ)) {
                 n++;
                 status = priv->read_reg(priv, REG_SR);

+               if (status & (SR_RBS|SR_BS) == (SR_RBS|SR_BS)) {
+                       /* what? something is rotten in my kingdom... */
+                       n = 0;
+                       break; /* or goto post_irq; as you want... */
+               }

                 if (isrc & IRQ_RI) {
                         /* receive interrupt */
                         while (status & SR_RBS) {
                                 sja1000_rx(dev);
                                 status = priv->read_reg(priv, REG_SR);
+                               if (status & (SR_RBS|SR_BS) == 
(SR_RBS|SR_BS)) {
+                                       /* what? something is rotten in 
my kingdom, here also... */
+                                       n = 0;
+                                       goto post_irq;
                                 }
                         }
                 }
         }

+post_irq:
         if (priv->post_irq)
                 priv->post_irq(priv);


Of course, in order to definitely say asta lavista to the potential 
infinite loop, we can use a for(;;) loop instead of the while()...

What are your opinion?

Stéphane
--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

Hi Stephane,

On 02/14/2012 10:14 AM, Stephane Grosjean wrote:
> 
> Le 13/02/2012 21:23, Wolfgang Grandegger a écrit :
>> On 02/13/2012 08:55 PM, Oliver Hartkopp wrote:
>>> What happens, if inside this inner while() statement the unplug happens?
>>>
>>> Do we get two correct CAN frames and then we get eight CAN frames
>>> like this:
>>>
>>> 12345677 4 AA BB CC DD
>>> 12345678 6 AA BB CC DD EE FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>>
>>> ???
>> Do you have seen such traces with the "device present" check in the
>> custom handler? I doubt.
> 
> Well if I may interfere: since the driver "remove()" entry point is
> called and the netdev interface is destroyed next, nobody can truly says
> if these frames have time to be processed until the client application.
> The problem is not here.
> 
>> First it is unlikely, that the unplug will happen when data from the
>> device gets processed. And if, the check in the while loop will not
>> change a lot, in contrast to the check in the custom handler. If it
>> happens while processing the data, we will get grap... but well...
>>
>>> Sending nothing is preferred to sending wrong data :-)
>> You cannot avoid sending crap under any circumstances. Also be aware
>> that the while loop will normally process just *one* message, and rarely
>> more.
> 
> cannot say that, this is arch dependent.
> 
>>>> Of course, the loop should be limited in sja1000_interrupt anyway.
>>>
>>> IMO checking for absent hardware in the custom isr is fine and should
>>> fix most
>>> of the problems (as we can already see in the ems_pcmcia driver).
> 
> cannot say that: all the tests I made @1xMbps bitrate show that the
> issue at least occurs 3/4 of time in the sja1000_interrupt, not in the
> custom isr. This also is obviously arch dependent.

What check did you use in the custom isr? A dummy register read != 0xff?
If that is really the case, I would prefer Oliver's simple fix in
sja1000_interrupt().

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp]

On 13.02.2012 21:23, Wolfgang Grandegger wrote:

> On 02/13/2012 08:55 PM, Oliver Hartkopp wrote:
>> On 13.02.2012 12:08, Wolfgang Grandegger wrote:
>>

>>>> It's not about reacting when an unlug happens, it's about not having the
>>>> interrupt handler to loop forever. At least we must limit the inner
>>>> while loop.
>>
>>
>> What happens, if inside this inner while() statement the unplug happens?
>>
>> Do we get two correct CAN frames and then we get eight CAN frames like this:
>>
>> 12345677 4 AA BB CC DD
>> 12345678 6 AA BB CC DD EE FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>> 1FFFFFFF 8 FF FF FF FF FF FF FF FF
>>
>> ???
> 
> Do you have seen such traces with the "device present" check in the
> custom handler? I doubt.


No, indeed i haven't. But plugging out PCMCIA cards 100 times a day under
traffic load is not my usual job ;-)

> First it is unlikely, that the unplug will happen when data from the
> device gets processed. And if, the check in the while loop will not
> change a lot, in contrast to the check in the custom handler. If it
> happens while processing the data, we will get grap... but well...


May be acceptable under these circumstances.

>> Sending nothing is preferred to sending wrong data :-)
> 
> You cannot avoid sending crap under any circumstances. Also be aware
> that the while loop will normally process just *one* message, and rarely
> more.


Under full load the SJA1000 internal FIFO is processed. I assume that this is
not that rare when having 6 SJA1000 based interfaces using one shared
interrupt but i did not make any measurements about this specific behavior.

>>> Of course, the loop should be limited in sja1000_interrupt anyway.
>>


ack.

>>
>> IMO checking for absent hardware in the custom isr is fine and should fix most
>> of the problems (as we can already see in the ems_pcmcia driver).
>>
>> But adding the patch which is checking two times for status != 0xFF reduces
>> the race conditions to almost zero. Having a while() statement that
>> potentially produces wrong data doesn't heal the problem IMO.
> 
> I disagree. It will not change a lot. Measurements may proof that I'm
> wrong, though.


Ok, i'll try it the other way ...

If we agree that the inner while statement needs some kind of check not to
loop forever i would like to suggest the following patch in favor to create
an additional variable for the inner while statement:


diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index ebbcfca..3bec3d9 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -99,7 +99,7 @@ static int sja1000_probe_chip(struct net_device *dev)
 {
 	struct sja1000_priv *priv = netdev_priv(dev);
 
-	if (priv->reg_base && (priv->read_reg(priv, 0) == 0xFF)) {
+	if (priv->reg_base && (priv->read_reg(priv, REG_MOD) == 0xFF)) {
 		printk(KERN_INFO "%s: probing @0x%lX failed\n",
 		       DRV_NAME, dev->base_addr);
 		return 0;
@@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	while ((isrc = priv->read_reg(priv, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
 		n++;
 		status = priv->read_reg(priv, REG_SR);
+		/* check for absent controller due to hw unplug */
+		if (status == 0xFF && !sja1000_probe_chip(dev))
+			break;
 
 		if (isrc & IRQ_WUI)
 			netdev_warn(dev, "wakeup interrupt\n");
@@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 			while (status & SR_RBS) {
 				sja1000_rx(dev);
 				status = priv->read_reg(priv, REG_SR);
+				/* check for absent controller */
+				if (status == 0xFF && !sja1000_probe_chip(dev))
+					break;
 			}
 		}
 		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {

The big advantage is, that we do not need another expensive register read nor
another loop counter variable in the case everything is working fine.

Only when the _already_ read status variable becomes 0xFF we check in the
mode register if the device is still present.

And finally there's a very little chance left to create rubbish data. 

Regards,
Oliver

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean]

Le 14/02/2012 10:59, Oliver Hartkopp a écrit :

> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>   	while ((isrc = priv->read_reg(priv, REG_IR))&&  (n<  SJA1000_MAX_IRQ)) {
>   		n++;
>   		status = priv->read_reg(priv, REG_SR);
> +		/* check for absent controller due to hw unplug */
> +		if (status == 0xFF&&  !sja1000_probe_chip(dev))
> +			break;
>
>   		if (isrc&  IRQ_WUI)
>   			netdev_warn(dev, "wakeup interrupt\n");
> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>   			while (status&  SR_RBS) {
>   				sja1000_rx(dev);
>   				status = priv->read_reg(priv, REG_SR);
> +				/* check for absent controller */
> +				if (status == 0xFF&&  !sja1000_probe_chip(dev))
> +					break;
>   			}
>   		}
>   		if (isrc&  (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
>
> The big advantage is, that we do not need another expensive register read nor
> another loop counter variable in the case everything is working fine.
>
> Only when the _already_ read status variable becomes 0xFF we check in the
> mode register if the device is still present.
>
> And finally there's a very little chance left to create rubbish data.


Quite perfect, except that we will push a "rubbish" error frame before 
exiting, and we will return IRQ_HANDLED while I would prefer IRQ_NONE...
But does it matter?

Moreover, regarding to this patch, should the peak_pcmcia custom isr 
always test for the card presence? I mean, this costs at least 2xtwo 
ioread8 for each hw irq!


Stéphane

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp]

On 14.02.2012 11:16, Stephane Grosjean wrote:

> 
> Le 14/02/2012 10:59, Oliver Hartkopp a écrit :
> 
>> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>       while ((isrc = priv->read_reg(priv, REG_IR))&&  (n<  SJA1000_MAX_IRQ)) {
>>           n++;
>>           status = priv->read_reg(priv, REG_SR);
>> +        /* check for absent controller due to hw unplug */
>> +        if (status == 0xFF&&  !sja1000_probe_chip(dev))
>> +            break;
>>
>>           if (isrc&  IRQ_WUI)
>>               netdev_warn(dev, "wakeup interrupt\n");
>> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>               while (status&  SR_RBS) {
>>                   sja1000_rx(dev);
>>                   status = priv->read_reg(priv, REG_SR);
>> +                /* check for absent controller */
>> +                if (status == 0xFF&&  !sja1000_probe_chip(dev))
>> +                    break;
>>               }
>>           }
>>           if (isrc&  (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
>>
>> The big advantage is, that we do not need another expensive register read nor
>> another loop counter variable in the case everything is working fine.
>>
>> Only when the _already_ read status variable becomes 0xFF we check in the
>> mode register if the device is still present.
>>
>> And finally there's a very little chance left to create rubbish data.
> 
> 
> Quite perfect, except that we will push a "rubbish" error frame before
> exiting, and we will return IRQ_HANDLED while I would prefer IRQ_NONE...
> But does it matter?


An alternative would be to replace the 'break' statements above with

	return IRQ_NONE;

like

		/* check for absent controller due to hw unplug */
		if (status == 0xFF && !sja1000_probe_chip(dev))
			return IRQ_NONE;

Don't know if this is 'good style' - but the while statement allows it ;-)

> 
> Moreover, regarding to this patch, should the peak_pcmcia custom isr always
> test for the card presence? I mean, this costs at least 2xtwo ioread8 for each
> hw irq!


I think the earlier we know the better.

But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...

So if we add the 'return IRQ_NONE' to the above patch the private check for
present hardware becomes obsolete as the private ISRs terminate properly when
they get an IRQ_NONE.

Right?

Regards,
Oliver

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

Hi Oliver,

we start paving common code to partially cure pcmcia related problems,
that's exactly what I do not like.

On 02/14/2012 05:41 PM, Oliver Hartkopp wrote:
> On 14.02.2012 11:16, Stephane Grosjean wrote:
> 
>>
>> Le 14/02/2012 10:59, Oliver Hartkopp a écrit :
>>
>>> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>       while ((isrc = priv->read_reg(priv, REG_IR))&&  (n<  SJA1000_MAX_IRQ)) {

In case of unplug, already the above read may return 0xff.

>>>           n++;
>>>           status = priv->read_reg(priv, REG_SR);
>>> +        /* check for absent controller due to hw unplug */
>>> +        if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>> +            break;

Once...

>>>           if (isrc&  IRQ_WUI)
>>>               netdev_warn(dev, "wakeup interrupt\n");
>>> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>               while (status&  SR_RBS) {
>>>                   sja1000_rx(dev);
>>>                   status = priv->read_reg(priv, REG_SR);
>>> +                /* check for absent controller */
>>> +                if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>> +                    break;

... twice... What about the other accesses?

>>>               }
>>>           }
>>>           if (isrc&  (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
>>>
>>> The big advantage is, that we do not need another expensive register read nor
>>> another loop counter variable in the case everything is working fine.
>>>
>>> Only when the _already_ read status variable becomes 0xFF we check in the
>>> mode register if the device is still present.
>>>
>>> And finally there's a very little chance left to create rubbish data.
>>
>>
>> Quite perfect, except that we will push a "rubbish" error frame before
>> exiting, and we will return IRQ_HANDLED while I would prefer IRQ_NONE...
>> But does it matter?

Well, it's far away from being perfect?

> 
> An alternative would be to replace the 'break' statements above with
> 
> 	return IRQ_NONE;
> 
> like
> 
> 		/* check for absent controller due to hw unplug */
> 		if (status == 0xFF && !sja1000_probe_chip(dev))
> 			return IRQ_NONE;
> 
> Don't know if this is 'good style' - but the while statement allows it ;-)
> 
>>
>> Moreover, regarding to this patch, should the peak_pcmcia custom isr always
>> test for the card presence? I mean, this costs at least 2xtwo ioread8 for each
>> hw irq!
> 
> 
> I think the earlier we know the better.
> 
> But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...
> 
> So if we add the 'return IRQ_NONE' to the above patch the private check for
> present hardware becomes obsolete as the private ISRs terminate properly when
> they get an IRQ_NONE.
> 
> Right?

If it's clear that the interrupt came from that device, we should return
IRQ_HANDLED otherwise it's treated as spurious. I would accept *one* "if
(status == 0xFF ..." check in a good place.

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp]

On 15.02.2012 08:03, Wolfgang Grandegger wrote:

> Hi Oliver,
> 
> we start paving common code to partially cure pcmcia related problems,
> that's exactly what I do not like.


It's PCMCIA and PCIeC and Parallel port, etc. Any pluggable sja1000 hardware.

> 
> On 02/14/2012 05:41 PM, Oliver Hartkopp wrote:
>> On 14.02.2012 11:16, Stephane Grosjean wrote:
>>
>>>
>>> Le 14/02/2012 10:59, Oliver Hartkopp a écrit :
>>>
>>>> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>       while ((isrc = priv->read_reg(priv, REG_IR))&&  (n<  SJA1000_MAX_IRQ)) {
> 
> In case of unplug, already the above read may return 0xff.


Right. But it does not get a bad effect as we quit from this bad condition
three lines later. No previously retrieved isrc variable is referenced then.
Please re-check the patched sja1000_interrupt() as a whole.

> 
>>>>           n++;
>>>>           status = priv->read_reg(priv, REG_SR);
>>>> +        /* check for absent controller due to hw unplug */
>>>> +        if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>>> +            break;
> 
> Once...
> 
>>>>           if (isrc&  IRQ_WUI)
>>>>               netdev_warn(dev, "wakeup interrupt\n");
>>>> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>               while (status&  SR_RBS) {
>>>>                   sja1000_rx(dev);
>>>>                   status = priv->read_reg(priv, REG_SR);
>>>> +                /* check for absent controller */
>>>> +                if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>>> +                    break;
> 
> ... twice... What about the other accesses?


Which ones do you have in mind?

The really only thing that could happen, that exactly ONE CAN frame can be
damaged on read while accessing the SJA1000 registers. This can not be solved
unless you check for sja1000_probe_chip() at the end of reading the frame and
before pushing the skbuff to the rx queue.

But this would add indeed a new ioport access each time we read a CAN frame.

I don't think that we should do this additional effort - even if it's probably
the right(TM) thing to do 8-)

My patch doesn't do any additional ioport accesses in a 'good' condition.

(..)

>> But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...
>>

>> So if we add the 'return IRQ_NONE' to the above patch the private check for
>> present hardware becomes obsolete as the private ISRs terminate properly when
>> they get an IRQ_NONE.
>>
>> Right?
> 
> If it's clear that the interrupt came from that device, we should return
> IRQ_HANDLED otherwise it's treated as spurious.


Can this usually happen in the case of unplugging devices?

Or we need some kind of wrapper for the sja1000 isr that returns

IRQ_NONE
IRQ_HANDLED
IRQ_PLUGOUT

while HANDLED & PLUGOUT would lead to IRQ_HANDLED on the top level.

> I would accept *one* "if
> (status == 0xFF ..." check in a good place.


Sorry but you need an additional check within the while statement.
There are two potential occurrences where we read the status. Why checking
only one time? As i wrote above: Please re-check the entire patched function.

Regards,
Oliver

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

On 02/15/2012 09:05 AM, Oliver Hartkopp wrote:
> On 15.02.2012 08:03, Wolfgang Grandegger wrote:
> 
>> Hi Oliver,
>>
>> we start paving common code to partially cure pcmcia related problems,
>> that's exactly what I do not like.
> 
> 
> It's PCMCIA and PCIeC and Parallel port, etc. Any pluggable sja1000 hardware.
> 
>>
>> On 02/14/2012 05:41 PM, Oliver Hartkopp wrote:
>>> On 14.02.2012 11:16, Stephane Grosjean wrote:
>>>
>>>>
>>>> Le 14/02/2012 10:59, Oliver Hartkopp a écrit :
>>>>
>>>>> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>       while ((isrc = priv->read_reg(priv, REG_IR))&&  (n<  SJA1000_MAX_IRQ)) {
>>
>> In case of unplug, already the above read may return 0xff.
> 
> 
> Right. But it does not get a bad effect as we quit from this bad condition
> three lines later. No previously retrieved isrc variable is referenced then.
> Please re-check the patched sja1000_interrupt() as a whole.
> 
>>
>>>>>           n++;
>>>>>           status = priv->read_reg(priv, REG_SR);
>>>>> +        /* check for absent controller due to hw unplug */
>>>>> +        if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>>>> +            break;
>>
>> Once...
>>
>>>>>           if (isrc&  IRQ_WUI)
>>>>>               netdev_warn(dev, "wakeup interrupt\n");
>>>>> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>>               while (status&  SR_RBS) {
>>>>>                   sja1000_rx(dev);
>>>>>                   status = priv->read_reg(priv, REG_SR);
>>>>> +                /* check for absent controller */
>>>>> +                if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>>>> +                    break;
>>
>> ... twice... What about the other accesses?
> 
> 
> Which ones do you have in mind?
> 
> The really only thing that could happen, that exactly ONE CAN frame can be
> damaged on read while accessing the SJA1000 registers. This can not be solved
> unless you check for sja1000_probe_chip() at the end of reading the frame and
> before pushing the skbuff to the rx queue.
> 
> But this would add indeed a new ioport access each time we read a CAN frame.
> 
> I don't think that we should do this additional effort - even if it's probably
> the right(TM) thing to do 8-)

Well, see my comment below.

> 
> My patch doesn't do any additional ioport accesses in a 'good' condition.
> 
> (..)
> 
>>> But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...
>>>
> 
>>> So if we add the 'return IRQ_NONE' to the above patch the private check for
>>> present hardware becomes obsolete as the private ISRs terminate properly when
>>> they get an IRQ_NONE.
>>>
>>> Right?
>>
>> If it's clear that the interrupt came from that device, we should return
>> IRQ_HANDLED otherwise it's treated as spurious.
> 
> 
> Can this usually happen in the case of unplugging devices?
> 
> Or we need some kind of wrapper for the sja1000 isr that returns
> 
> IRQ_NONE
> IRQ_HANDLED
> IRQ_PLUGOUT
> 
> while HANDLED & PLUGOUT would lead to IRQ_HANDLED on the top level.
> 
>> I would accept *one* "if
>> (status == 0xFF ..." check in a good place.
> 
> 
> Sorry but you need an additional check within the while statement.
> There are two potential occurrences where we read the status. Why checking
> only one time? As i wrote above: Please re-check the entire patched function.

Why do we care so much about improper handling of the hardware. It is
*illegal* to unplug the device while it's under operation and if it
happens we cannot guarantee valid behavior and data anyway. The system
should not hang, of course, everything else is not that dramatic, I
think. Mark, what is your opinion?

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean]

Le 15/02/2012 08:03, Wolfgang Grandegger a écrit :
> Hi Oliver,
>
> we start paving common code to partially cure pcmcia related problems,
> that's exactly what I do not like.


I think it's much more than a pcmcia related problem: IMHO, I think it's 
right in some common code to do some 'basic' checks on returned values, 
especially when these are returned from custom callbacks and these 
values are so critical. For me, it's like one has to check the kmalloc() 
return value, for example. But I admit this could not be the right(TM) 
way of doing things in the Kernel.


>> An alternative would be to replace the 'break' statements above with
>>
>> 	return IRQ_NONE;
>>
>> like
>>
>> 		/* check for absent controller due to hw unplug */
>> 		if (status == 0xFF&&  !sja1000_probe_chip(dev))
>> 			return IRQ_NONE;
>>
>> Don't know if this is 'good style' - but the while statement allows it ;-)
>>
>>> Moreover, regarding to this patch, should the peak_pcmcia custom isr always
>>> test for the card presence? I mean, this costs at least 2xtwo ioread8 for each
>>> hw irq!
>>
>> I think the earlier we know the better.


Ok, so I keep testing the presence of the card in the peak_pcmcia driver.

If I may ask something else: didn't find any "linux-pcmcia" mailing list 
(or something like that) in vger majordomo lists. Does someone know if 
there is such a list? By default, which ml should be posted the 
peak_pcmcia.c too?


>> But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...


Hmmm... I don't know what is the real goal of these pre_irq() and 
post_iqr() callbacks but I imagine that, for example, the "pre_irq()" 
could acquire a lock while the post_irq() would release it... So, I 
would prefer to call the post_irq() anytime (that is, no direct "return 
IRQ_xxx" but a 'break'  or a 'goto'). Does it make sense?


>> So if we add the 'return IRQ_NONE' to the above patch the private check for
>> present hardware becomes obsolete as the private ISRs terminate properly when
>> they get an IRQ_NONE.
>>
>> Right?


I would also have say that before, but I checked that IRQ_NONE may be 
processed as spurious, so I don't even know, for the moment. LDD3 
chapter 10 also says:

> Interrupt handlers should return a value indicating whether there was 
> actually an
> interrupt to handle. If the handler found that its device did, indeed, 
> need attention, it
> should return IRQ_HANDLED; otherwise the return value should be IRQ_NONE.

So, what does this "need attention" stands for? Since the device is 
unplugged, we could say that it didn't need any attention anymore... 
But, on the other hand, the ems_pcmcia driver does return IRQ_HANDLED 
when its private check fails to detect the card and it has been 
approved, so, where is the truth? Near the perfection? ;-)


Stéphane
--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Wolfgang Grandegger]

On 02/15/2012 12:52 PM, Stephane Grosjean wrote:
> Le 15/02/2012 08:03, Wolfgang Grandegger a écrit :
>> Hi Oliver,
>>
>> we start paving common code to partially cure pcmcia related problems,
>> that's exactly what I do not like.
> 
> 
> I think it's much more than a pcmcia related problem: IMHO, I think it's
> right in some common code to do some 'basic' checks on returned values,
> especially when these are returned from custom callbacks and these
> values are so critical. For me, it's like one has to check the kmalloc()
> return value, for example. But I admit this could not be the right(TM)
> way of doing things in the Kernel.

Well, but it's unusual (and slow) to check any hardware access.

>>> An alternative would be to replace the 'break' statements above with
>>>
>>>     return IRQ_NONE;
>>>
>>> like
>>>
>>>         /* check for absent controller due to hw unplug */
>>>         if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>>             return IRQ_NONE;
>>>
>>> Don't know if this is 'good style' - but the while statement allows
>>> it ;-)
>>>
>>>> Moreover, regarding to this patch, should the peak_pcmcia custom isr
>>>> always
>>>> test for the card presence? I mean, this costs at least 2xtwo
>>>> ioread8 for each
>>>> hw irq!
>>>
>>> I think the earlier we know the better.
> 
> 
> Ok, so I keep testing the presence of the card in the peak_pcmcia driver.
> 
> If I may ask something else: didn't find any "linux-pcmcia" mailing list
> (or something like that) in vger majordomo lists. Does someone know if
> there is such a list? By default, which ml should be posted the
> peak_pcmcia.c too?

Here is the entry from the kernel's MAINTAINER file with the relevant
mailing list address:

PCMCIA SUBSYSTEM
P:      Linux PCMCIA Team
L:      linux-pcmcia@lists.infradead.org
W:      http://lists.infradead.org/mailman/listinfo/linux-pcmcia
T:      git
git://git.kernel.org/pub/scm/linux/kernel/git/brodo/pcmcia-2.6.git
S:      Maintained
F:      Documentation/pcmcia/
F:      drivers/pcmcia/
F:      include/pcmcia/


>>> But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...
> 
> 
> Hmmm... I don't know what is the real goal of these pre_irq() and
> post_iqr() callbacks but I imagine that, for example, the "pre_irq()"
> could acquire a lock while the post_irq() would release it... So, I
> would prefer to call the post_irq() anytime (that is, no direct "return
> IRQ_xxx" but a 'break'  or a 'goto'). Does it make sense?

The pre_irq and post_irq callbacks have been introduced to allow device
specific interrupt handling/acknowledgement before and after the common
handler. Have a look to the peak_pci driver to understand what I mean.

> 
> 
>>> So if we add the 'return IRQ_NONE' to the above patch the private
>>> check for
>>> present hardware becomes obsolete as the private ISRs terminate
>>> properly when
>>> they get an IRQ_NONE.
>>>
>>> Right?
> 
> 
> I would also have say that before, but I checked that IRQ_NONE may be
> processed as spurious, so I don't even know, for the moment. LDD3
> chapter 10 also says:
> 
>> Interrupt handlers should return a value indicating whether there was
>> actually an
>> interrupt to handle. If the handler found that its device did, indeed,
>> need attention, it
>> should return IRQ_HANDLED; otherwise the return value should be IRQ_NONE.
> 
> So, what does this "need attention" stands for? Since the device is
> unplugged, we could say that it didn't need any attention anymore...
> But, on the other hand, the ems_pcmcia driver does return IRQ_HANDLED
> when its private check fails to detect the card and it has been
> approved, so, where is the truth? Near the perfection? ;-)

From http://lxr.linux.no/#linux+v3.2.6/include/linux/irqreturn.h:

  IRQ_NONE            interrupt was not from this device

Wolfgang.

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Stephane Grosjean]

Le 15/02/2012 16:06, Wolfgang Grandegger a écrit :
> Here is the entry from the kernel's MAINTAINER file with the relevant
> mailing list address:
>
> PCMCIA SUBSYSTEM
> P:      Linux PCMCIA Team
> L:      linux-pcmcia@lists.infradead.org
> W:      http://lists.infradead.org/mailman/listinfo/linux-pcmcia
> T:      git
> git://git.kernel.org/pub/scm/linux/kernel/git/brodo/pcmcia-2.6.git
> S:      Maintained
> F:      Documentation/pcmcia/
> F:      drivers/pcmcia/
> F:      include/pcmcia/
>


Ok thanks.
Should I wait for any "Acked-by" from the linux-can ml before?

Stéphane
--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp]

On 15.02.2012 09:37, Wolfgang Grandegger wrote:


> Why do we care so much about improper handling of the hardware. It is
> *illegal* to unplug the device while it's under operation and if it
> happens we cannot guarantee valid behavior and data anyway. The system
> should not hang, of course, everything else is not that dramatic, I
> think. Mark, what is your opinion?


Hello Wolfgang,

i wonder if people will stop pulling hotplug devices because it is *illegal*

http://www.deq.louisiana.gov/portal/portals/0/news/pdf/IllegalDumpingSign.jpg

Really funny :-)

Btw. i started to implement some code that catches every issue that can occur
when unplugging a sja1000 device:

1. Adding a new flag SJA1000_CHECK_FOR_UNPLUG to be checked via priv->flags

2. Adding the checks to sja1000_interrupt()

3. Adding the checks to sja1000_rx & sja1000_err right before netif_rx(skb)

Finally i came to the conclusion that this was a lot of stuff for a pretty
rare situation. IMO we should only fix a potential hang of the isr and ignore
the damaged data which is probably generated at plug-out.

That's why i sent the "[PATCH v2] can: sja1000 fix isr hang when hw is
unplugged under load" which does exactly fix that issue and is pretty cheap
with additional register reads.

Please consider that patch for inclusion.

Tnx & regards,
Oliver

Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp]

>> I would also have say that before, but I checked that IRQ_NONE may be
>> processed as spurious, so I don't even know, for the moment. LDD3
>> chapter 10 also says:
>>
>>> Interrupt handlers should return a value indicating whether there was
>>> actually an
>>> interrupt to handle. If the handler found that its device did, indeed,
>>> need attention, it
>>> should return IRQ_HANDLED; otherwise the return value should be IRQ_NONE.
>>
>> So, what does this "need attention" stands for? Since the device is
>> unplugged, we could say that it didn't need any attention anymore...
>> But, on the other hand, the ems_pcmcia driver does return IRQ_HANDLED
>> when its private check fails to detect the card and it has been
>> approved, so, where is the truth? Near the perfection? ;-)
> 
> From http://lxr.linux.no/#linux+v3.2.6/include/linux/irqreturn.h:
> 
>   IRQ_NONE            interrupt was not from this device


See:

http://lxr.linux.no/#linux+v3.2.6/kernel/irq/spurious.c#L178

I think at plug-out we get very little IRQ_NONE return values that should be
ok not to damage the entire handling of that specific irq line ...

Regards,
Oliver