Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

PATCH Applied Wolfgang Grandegger 5 participants 16 messages Apr 15, 2014 – Apr 28, 2014 4 messages not in the archive

Subsystems (3)

Networking Network drivers CAN network drivers

Share & tools

A message that is not in the archive (sent off-list or not yet received): CA+s8ZrqFBun4bo2rJJFHb0FrV7m7PLybUVM+jjahKsdW8bdnaQ@mail.gmail.com

Wolfgang Grandegger

author

2014-04-15 09:55 UTC #1 lore raw

Hi Khurram,

we have switched to a new mailing list some time ago, which I have therefore added to the CC.

Quoted 6 lines · khurram gulzar

Hi Oliver and Wolfgang,
Thanks for your support in advance
I have recently updated the system from Kernel 2.6 to use the new kernel 3.2 for linux. I have noticed that the socket-can is integrated in the kernel. I enabled the driver in the kernel from the kernel config. The drivers are loaded successfully and both the CAN are transmitting

the

Quoted 3 lines

packets. However its unable to receive any packets on any of the CAN. The system configurations are still the same loaded drivers -> can.ko-> can-raw.ko-> sja1000-iomux

There is now module named "sja1000-iomux" in the mainline kernel. Where did you get it from?

Quoted 4 lines

The address of can is configured to base=0x200,0x204 and irq=10,11 with speed=1000,1000 i.e. 1Mbits/sec
I have checked the irq in /proc/interrupts. IRQ 10 and 11 are showing connected to CAN0 and CAN1.

Do the IRQ counts increase when you send messages?

Quoted 2 lines

Can you please tell me what seems to be wrong- Waiting for your reply

Could you switch to a recent mainline kernel version?

Wolfgang.

Oliver Hartkopp

2014-04-15 10:51 UTC #2 lore raw

Hello Khurram,

we're obviously in a time loop :-)

See your same request ~5 years ago:

http://socket-can.996257.n3.nabble.com/Support-needed-for-Eurotech-COM1273-Dual-Channel-CAN-PC104-Module-td1524.html

I assume the sja1000_isa driver will be the right one for this old card. It is part of mainline Linux - so no need to compile old SocketCAN sources from the SVN.

Regards,
Oliver

Quoted 40 lines · Wolfgang Grandegger

Hi Khurram,
we have switched to a new mailing list some time ago, which I have therefore added to the CC.
Quoted 6 lines · khurram gulzar
Hi Oliver and Wolfgang,
Thanks for your support in advance
I have recently updated the system from Kernel 2.6 to use the new kernel 3.2 for linux. I have noticed that the socket-can is integrated in the kernel. I enabled the driver in the kernel from the kernel config. The drivers are loaded successfully and both the CAN are transmitting
the
Quoted 3 lines
packets. However its unable to receive any packets on any of the CAN. The system configurations are still the same loaded drivers -> can.ko-> can-raw.ko-> sja1000-iomux
There is now module named "sja1000-iomux" in the mainline kernel. Where did you get it from?
Quoted 4 lines
The address of can is configured to base=0x200,0x204 and irq=10,11 with speed=1000,1000 i.e. 1Mbits/sec
I have checked the irq in /proc/interrupts. IRQ 10 and 11 are showing connected to CAN0 and CAN1.
Do the IRQ counts increase when you send messages?
Quoted 2 lines
Can you please tell me what seems to be wrong- Waiting for your reply
Could you switch to a recent mainline kernel version?
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CA+s8ZroY7j8MLpFQh568QdeB24zBwVAVXbi7RzQF+baFi+Mpkw@mail.gmail.com

Oliver Hartkopp

2014-04-15 11:56 UTC #4 lore raw

Hello Khurram,

the speed is only set with the 'ip' tool now.

And regarding the former iomux handling it should be like this:

modprobe sja1000_isa port=0x200,0x204 irq=10,11 indirect=1,1

See the code for further module parameter details:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/can/sja1000/sja1000_isa.c

Regards,
Oliver

Quoted 73 lines · khurram gulzar

Thanx Oliver,
Yes indeed its a v.big time loop.
I was previously using sja1000-iomux
sja1000_isa i can only load one module can0 at address 0x200 but cant load 0x204 bus
modprobe sja1000_isa port=0x200,0x204 irq=10,11 with speed parameter set by ip link set command
there is an indirect parameter, I have no idea how it is used could you elaborate my base address is 0x200 and 0x204 with irq 10 and 11 speed 1000,1000
On Tue, Apr 15, 2014 at 1:50 PM, Oliver Hartkopp <[email protected] <mailto:[email protected]>> wrote:
Hello Khurram,
we're obviously in a time loop :-)
See your same request ~5 years ago:
http://socket-can.996257.n3.nabble.com/Support-needed-for-Eurotech-COM1273-Dual-Channel-CAN-PC104-Module-td1524.html
I assume the sja1000_isa driver will be the right one for this old card. It is part of mainline Linux - so no need to compile old SocketCAN sources from the SVN.
Regards,
Oliver
On 15.04.2014 11:55, Wolfgang Grandegger wrote:
> Hi Khurram,
>
> we have switched to a new mailing list some time ago, which I have
> therefore added to the CC.
>
> On Tue, 15 Apr 2014 11:07:15 +0300, khurram gulzar
> <[email protected] <mailto:[email protected]>> wrote:
>> Hi Oliver and Wolfgang,
>> Thanks for your support in advance
>> I have recently updated the system from Kernel 2.6 to use the new kernel >> 3.2 for linux. I have noticed that the socket-can is integrated in the >> kernel. I enabled the driver in the kernel from the kernel config. >> The drivers are loaded successfully and both the CAN are transmitting > the
>> packets. However its unable to receive any packets on any of the CAN. >> The system configurations are still the same loaded drivers
>> -> can.ko-> can-raw.ko-> sja1000-iomux
>
> There is now module named "sja1000-iomux" in the mainline kernel.
> Where did you get it from?
>
>> The address of can is configured to base=0x200,0x204 and irq=10,11 with >> speed=1000,1000 i.e. 1Mbits/sec
>> I have checked the irq in /proc/interrupts. IRQ 10 and 11 are showing >> connected to CAN0 and CAN1.
>
> Do the IRQ counts increase when you send messages?
>
>> Can you please tell me what seems to be wrong- Waiting for your
>> reply
>
> Could you switch to a recent mainline kernel version?
>
> Wolfgang.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in > the body of a message to [email protected] <mailto:[email protected]> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>

A message that is not in the archive (sent off-list or not yet received): CA+s8Zrqrd_U0g1GRCDzXncgWLA_kO3Hyhjwy6Oe1S0cYohm2og@mail.gmail.com

Oliver Hartkopp

2014-04-15 12:00 UTC #6 lore raw

As I wrote in the other thread:

Quoted 2 lines

The command i am executing is
modprobe sja1000_isa port=0x200,0x204 irq=10,11 indirect=1

The parameters for each interface are given separately

Quoted 2 lines

but in dmesg it says
Quoted 1 line
Quoted 1 line
probe of sja1000_isa.1 failed with error -16

That's correct as you did not provide the 'indirect' flags to the _second_ interface :-)

	modprobe sja1000_isa port=0x200,0x204 irq=10,11 indirect=1,1

should make it.

Quoted 1 line

I am now running linux kernel version 3.2

A good choice :-)

Best regards,
Oliver

Marc Kleine-Budde

maintainer

2014-04-15 12:33 UTC #7 lore raw

Quoted 6 lines · Oliver Hartkopp

As I wrote in the other thread:
Quoted 2 lines
The command i am executing is
modprobe sja1000_isa port=0x200,0x204 irq=10,11 indirect=1
The parameters for each interface are given separately

The indirect mode is racy, btw....

Quoted 9 lines

static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv *priv,
int reg)
{
        unsigned long base = (unsigned long)priv->reg_base;

        outb(reg, base);
        return inb(base + 1);
}

Quoted 8 lines

static void sja1000_isa_port_write_reg_indirect(const struct sja1000_priv *priv,
int reg, u8 val)
{
        unsigned long base = (unsigned long)priv->reg_base;

        outb(reg, base);
        outb(val, base + 1);
}

We need locks.

Marc

Signature · 4 lines

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   |

Oliver Hartkopp

2014-04-15 13:19 UTC #8 lore raw

Something like this?

It uses the dev->dev_id which was missed in the 3e66d0138c05d9792f patch before.

Just compile-tested by now ...

drivers/net/can/sja1000/sja1000_isa.c +15 -3

index df136a2..46e1784 100644
@@ -46,6 +46,7 @@ static int clk[MAXDEV];
 static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
+static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access mode */
 
 module_param_array(port, ulong, NULL, S_IRUGO);
 MODULE_PARM_DESC(port, "I/O port number");

@@ -101,19 +102,28 @@ static void sja1000_isa_port_write_reg(const struct sja1000_priv *priv,
 static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv *priv,
 					     int reg)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long base, flags;
+	u8 readval;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
+	base = (unsigned long)priv->reg_base;
 	outb(reg, base);
-	return inb(base + 1);
+	readval = inb(base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
+
+	return readval;
 }
 
 static void sja1000_isa_port_write_reg_indirect(const struct sja1000_priv *priv,
 						int reg, u8 val)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long base, flags;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
+	base = (unsigned long)priv->reg_base;
 	outb(reg, base);
 	outb(val, base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
 }
 
 static int sja1000_isa_probe(struct platform_device *pdev)

@@ -169,6 +179,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 		if (iosize == SJA1000_IOSIZE_INDIRECT) {
 			priv->read_reg = sja1000_isa_port_read_reg_indirect;
 			priv->write_reg = sja1000_isa_port_write_reg_indirect;
+			spin_lock_init(&indirect_lock[idx]);
 		} else {
 			priv->read_reg = sja1000_isa_port_read_reg;
 			priv->write_reg = sja1000_isa_port_write_reg;

@@ -198,6 +209,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
+	dev->dev_id = idx;
 
 	err = register_sja1000dev(dev);
 	if (err) {

Quoted 32 lines · Marc Kleine-Budde

Quoted 6 lines · Oliver Hartkopp
As I wrote in the other thread:
Quoted 2 lines
The command i am executing is
modprobe sja1000_isa port=0x200,0x204 irq=10,11 indirect=1
The parameters for each interface are given separately
The indirect mode is racy, btw....
Quoted 9 lines
static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv *priv,
int reg)
{
        unsigned long base = (unsigned long)priv->reg_base;

        outb(reg, base);
        return inb(base + 1);
}
Quoted 8 lines
static void sja1000_isa_port_write_reg_indirect(const struct sja1000_priv *priv,
int reg, u8 val)
{
        unsigned long base = (unsigned long)priv->reg_base;

        outb(reg, base);
        outb(val, base + 1);
}
We need locks.
Marc

Marc Kleine-Budde

maintainer

2014-04-15 13:42 UTC #9 lore raw

Quoted 1 line · Oliver Hartkopp

Something like this?

Yes, no need to move the "base = pric->reg_base" under the lock though.

Marc

Quoted 101 lines

It uses the dev->dev_id which was missed in the 3e66d0138c05d9792f patch before.

Just compile-tested by now ...

drivers/net/can/sja1000/sja1000_isa.c +15 -3

index df136a2..46e1784 100644
@@ -46,6 +46,7 @@ static int clk[MAXDEV];
 static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
+static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access mode */
 
 module_param_array(port, ulong, NULL, S_IRUGO);
 MODULE_PARM_DESC(port, "I/O port number");

@@ -101,19 +102,28 @@ static void sja1000_isa_port_write_reg(const struct sja1000_priv *priv,
 static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv *priv,
 					     int reg)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long base, flags;
+	u8 readval;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
+	base = (unsigned long)priv->reg_base;
 	outb(reg, base);
-	return inb(base + 1);
+	readval = inb(base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
+
+	return readval;
 }
 
 static void sja1000_isa_port_write_reg_indirect(const struct sja1000_priv *priv,
 						int reg, u8 val)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long base, flags;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
+	base = (unsigned long)priv->reg_base;
 	outb(reg, base);
 	outb(val, base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
 }
 
 static int sja1000_isa_probe(struct platform_device *pdev)

@@ -169,6 +179,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 		if (iosize == SJA1000_IOSIZE_INDIRECT) {
 			priv->read_reg = sja1000_isa_port_read_reg_indirect;
 			priv->write_reg = sja1000_isa_port_write_reg_indirect;
+			spin_lock_init(&indirect_lock[idx]);
 		} else {
 			priv->read_reg = sja1000_isa_port_read_reg;
 			priv->write_reg = sja1000_isa_port_write_reg;

@@ -198,6 +209,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
+	dev->dev_id = idx;
 
 	err = register_sja1000dev(dev);
 	if (err) {

Quoted 32 lines · Marc Kleine-Budde

Quoted 6 lines · Oliver Hartkopp
As I wrote in the other thread:
Quoted 2 lines
The command i am executing is
modprobe sja1000_isa port=0x200,0x204 irq=10,11 indirect=1
The parameters for each interface are given separately
The indirect mode is racy, btw....
Quoted 9 lines
static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv *priv,
int reg)
{
        unsigned long base = (unsigned long)priv->reg_base;

        outb(reg, base);
        return inb(base + 1);
}
Quoted 8 lines
static void sja1000_isa_port_write_reg_indirect(const struct sja1000_priv *priv,
int reg, u8 val)
{
        unsigned long base = (unsigned long)priv->reg_base;

        outb(reg, base);
        outb(val, base + 1);
}
We need locks.
Marc

Signature · 4 lines

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   |

Oliver Hartkopp

2014-04-15 17:30 UTC #10 lore raw

PATCH 1/1 can: sja1000_isa: add locking for indirect register access mode

When accessing the SJA1000 controller registers in the indirect access mode, writing the register number and reading/writing the data has to be an atomic attempt.

As the sja1000_isa driver is an old style driver with a fixed number of instances the locking variable depends on the same index like all the other configuration elements given on the module command line.

As a positive side effect dev->dev_id is populated by the instance index, which was missing in 3e66d0138c05d9 ("can: populate netdev::dev_id for udev discrimination").

Reported-by Marc Kleine-Budde <[email protected]>

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

---

drivers/net/can/sja1000/sja1000_isa.c +13 -3

index df136a2..014695d 100644
@@ -46,6 +46,7 @@ static int clk[MAXDEV];
 static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
+static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access mode */
 
 module_param_array(port, ulong, NULL, S_IRUGO);
 MODULE_PARM_DESC(port, "I/O port number");

@@ -101,19 +102,26 @@ static void sja1000_isa_port_write_reg(const struct sja1000_priv *priv,
 static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv *priv,
 					     int reg)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long flags, base = (unsigned long)priv->reg_base;
+	u8 readval;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
 	outb(reg, base);
-	return inb(base + 1);
+	readval = inb(base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
+
+	return readval;
 }
 
 static void sja1000_isa_port_write_reg_indirect(const struct sja1000_priv *priv,
 						int reg, u8 val)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long flags, base = (unsigned long)priv->reg_base;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
 	outb(reg, base);
 	outb(val, base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
 }
 
 static int sja1000_isa_probe(struct platform_device *pdev)

@@ -169,6 +177,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 		if (iosize == SJA1000_IOSIZE_INDIRECT) {
 			priv->read_reg = sja1000_isa_port_read_reg_indirect;
 			priv->write_reg = sja1000_isa_port_write_reg_indirect;
+			spin_lock_init(&indirect_lock[idx]);
 		} else {
 			priv->read_reg = sja1000_isa_port_read_reg;
 			priv->write_reg = sja1000_isa_port_write_reg;

@@ -198,6 +207,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
+	dev->dev_id = idx;
 
 	err = register_sja1000dev(dev);
 	if (err) {

Thomas Gleixner

2014-04-15 21:26 UTC #11 lore raw

Re: [PATCH] can: sja1000_isa: add locking for indirect register access mode

Quoted 22 lines · Oliver Hartkopp

When accessing the SJA1000 controller registers in the indirect access mode, writing the register number and reading/writing the data has to be an atomic attempt.
As the sja1000_isa driver is an old style driver with a fixed number of instances the locking variable depends on the same index like all the other configuration elements given on the module command line.
As a positive side effect dev->dev_id is populated by the instance index, which was missing in 3e66d0138c05d9 ("can: populate netdev::dev_id for udev discrimination").
Reported-by Marc Kleine-Budde <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
drivers/net/can/sja1000/sja1000_isa.c +0 -0
index df136a2..014695d 100644

drivers/net/can/sja1000/sja1000_isa.c static int clk[MAXDEV]; · line 49

Quoted 4 lines

drivers/net/can/sja1000/sja1000_isa.c +1 -0

@@ -46,6 +46,7 @@ static int clk[MAXDEV];
 static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
+static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access mode */

You need to initialize the spinlock if you want to survive lockdep.

Either use the proper static initializer or do an explicit spin_lock_init() on each of the locks before using them.

Thanks,

tglx

Marc Kleine-Budde

maintainer

2014-04-15 21:49 UTC #12 lore raw

Re: [PATCH] can: sja1000_isa: add locking for indirect register access mode

Quoted 31 lines · Thomas Gleixner

Quoted 22 lines · Oliver Hartkopp
When accessing the SJA1000 controller registers in the indirect access mode, writing the register number and reading/writing the data has to be an atomic attempt.
As the sja1000_isa driver is an old style driver with a fixed number of instances the locking variable depends on the same index like all the other configuration elements given on the module command line.
As a positive side effect dev->dev_id is populated by the instance index, which was missing in 3e66d0138c05d9 ("can: populate netdev::dev_id for udev discrimination").
Reported-by Marc Kleine-Budde <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
drivers/net/can/sja1000/sja1000_isa.c +0 -0
index df136a2..014695d 100644
drivers/net/can/sja1000/sja1000_isa.c static int clk[MAXDEV]; · line 49
Quoted 4 lines
drivers/net/can/sja1000/sja1000_isa.c +1 -0
@@ -46,6 +46,7 @@ static int clk[MAXDEV];
 static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
+static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access mode */
You need to initialize the spinlock if you want to survive lockdep.

Quoted 2 lines

Either use the proper static initializer or do an explicit spin_lock_init() on each of the locks before using them.

There's a spin_lock_init in the probe function:

drivers/net/can/sja1000/sja1000_isa.c static int sja1000_isa_probe(struct platform_device *pdev) · line 183

Quoted 9 lines

static int sja1000_isa_probe(struct platform_device *pdev)

drivers/net/can/sja1000/sja1000_isa.c +1 -0

@@ -169,6 +177,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 		if (iosize == SJA1000_IOSIZE_INDIRECT) {
 			priv->read_reg = sja1000_isa_port_read_reg_indirect;
 			priv->write_reg = sja1000_isa_port_write_reg_indirect;
+			spin_lock_init(&indirect_lock[idx]);
 		} else {
 			priv->read_reg = sja1000_isa_port_read_reg;
 			priv->write_reg = sja1000_isa_port_write_reg;

Marc

Signature · 4 lines

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   |

Thomas Gleixner

2014-04-16 08:48 UTC #13 lore raw

Re: [PATCH] can: sja1000_isa: add locking for indirect register access mode

Quoted 35 lines · Marc Kleine-Budde

Quoted 31 lines · Thomas Gleixner
Quoted 22 lines · Oliver Hartkopp
When accessing the SJA1000 controller registers in the indirect access mode, writing the register number and reading/writing the data has to be an atomic attempt.
As the sja1000_isa driver is an old style driver with a fixed number of instances the locking variable depends on the same index like all the other configuration elements given on the module command line.
As a positive side effect dev->dev_id is populated by the instance index, which was missing in 3e66d0138c05d9 ("can: populate netdev::dev_id for udev discrimination").
Reported-by Marc Kleine-Budde <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
drivers/net/can/sja1000/sja1000_isa.c +0 -0
index df136a2..014695d 100644
drivers/net/can/sja1000/sja1000_isa.c static int clk[MAXDEV]; · line 49
Quoted 4 lines
drivers/net/can/sja1000/sja1000_isa.c +1 -0
@@ -46,6 +46,7 @@ static int clk[MAXDEV];
 static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
+static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access mode */
You need to initialize the spinlock if you want to survive lockdep.
Quoted 2 lines
Either use the proper static initializer or do an explicit spin_lock_init() on each of the locks before using them.

Quoted 8 lines

There's a spin_lock_init in the probe function:
drivers/net/can/sja1000/sja1000_isa.c static int sja1000_isa_probe(struct platform_device *pdev) · line 180
Quoted 6 lines
static int sja1000_isa_probe(struct platform_device *pdev)
drivers/net/can/sja1000/sja1000_isa.c +1 -0
@@ -169,6 +177,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 		if (iosize == SJA1000_IOSIZE_INDIRECT) {
 			priv->read_reg = sja1000_isa_port_read_reg_indirect;
 			priv->write_reg = sja1000_isa_port_write_reg_indirect;
+			spin_lock_init(&indirect_lock[idx]);

Ooops, missed that :)

Marc Kleine-Budde

maintainer Applied

2014-04-17 19:23 UTC #14 lore raw

Re: [PATCH] can: sja1000_isa: add locking for indirect register access mode

In reply to Oliver Hartkopp

Quoted 8 lines · Oliver Hartkopp

When accessing the SJA1000 controller registers in the indirect access mode, writing the register number and reading/writing the data has to be an atomic attempt.
As the sja1000_isa driver is an old style driver with a fixed number of instances the locking variable depends on the same index like all the other configuration elements given on the module command line.

Quoted 6 lines

As a positive side effect dev->dev_id is populated by the instance index, which was missing in 3e66d0138c05d9 ("can: populate netdev::dev_id for udev discrimination").
Reported-by Marc Kleine-Budde <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>

Applied to can/master.

Thanks,
Marc

Signature · 4 lines

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   |

Oliver Hartkopp

2014-04-21 17:39 UTC #15 lore raw

Re: [PATCH] can: sja1000_isa: add locking for indirect register access mode

In reply to Oliver Hartkopp

Hello Khurram,

is it possible for you to test this patch with your hardware?

It should apply on your used kernel without problems.

Thanks & best regards,
Oliver

Quoted 82 lines · Oliver Hartkopp

When accessing the SJA1000 controller registers in the indirect access mode, writing the register number and reading/writing the data has to be an atomic attempt.

As a positive side effect dev->dev_id is populated by the instance index, which was missing in 3e66d0138c05d9 ("can: populate netdev::dev_id for udev discrimination").

Reported-by Marc Kleine-Budde <[email protected]>

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

---

drivers/net/can/sja1000/sja1000_isa.c +13 -4

index df136a2..014695d 100644
@@ -46,6 +46,7 @@ static int clk[MAXDEV];
 static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
 static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
+static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access mode */
 
 module_param_array(port, ulong, NULL, S_IRUGO);
 MODULE_PARM_DESC(port, "I/O port number");

@@ -101,19 +102,26 @@ static void sja1000_isa_port_write_reg(const struct sja1000_priv *priv,
 static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv *priv,
 					     int reg)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long flags, base = (unsigned long)priv->reg_base;
+	u8 readval;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
 	outb(reg, base);
-	return inb(base + 1);
+	readval = inb(base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
+
+	return readval;
 }
 
 static void sja1000_isa_port_write_reg_indirect(const struct sja1000_priv *priv,
 						int reg, u8 val)
 {
-	unsigned long base = (unsigned long)priv->reg_base;
+	unsigned long flags, base = (unsigned long)priv->reg_base;
 
+	spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
 	outb(reg, base);
 	outb(val, base + 1);
+	spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
 }
 
 static int sja1000_isa_probe(struct platform_device *pdev)

@@ -169,6 +177,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 		if (iosize == SJA1000_IOSIZE_INDIRECT) {
 			priv->read_reg = sja1000_isa_port_read_reg_indirect;
 			priv->write_reg = sja1000_isa_port_write_reg_indirect;
+			spin_lock_init(&indirect_lock[idx]);
 		} else {
 			priv->read_reg = sja1000_isa_port_read_reg;
 			priv->write_reg = sja1000_isa_port_write_reg;

@@ -198,6 +207,7 @@ static int sja1000_isa_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
+	dev->dev_id = idx;
 
 	err = register_sja1000dev(dev);
 	if (err) {
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CA+s8ZroSGpijgG4ruk2T1V5Vp1WrCcVjVPPrqGU-rQSDEUxzXg@mail.gmail.com

Oliver Hartkopp

2014-04-24 11:41 UTC #17 lore raw

Re: [PATCH] can: sja1000_isa: add locking for indirect register access mode

Hi Khurram,

usual faults, when there's no traffic:

- interface is not 'up'
- bitrate is not set
- CAN bus is not terminated
- no second CAN node available(*) on the CAN bus to acknowledge the CAN frames

* = which the same correct bittiming

Are there any interrupts showing up for irq10 or irq11 in /proc/interrupts ??

Because when you see a successful transmission a transmit interrupt must have been there.

Can you sent the outputs of:

grep " 1[01]:" /proc/interrupts grep " can[01]" /proc/net/dev ip -det link show can0
ip -det link show can1

Thanks,
Oliver

Quoted 122 lines · khurram gulzar

Hi Oliver,
I could test it but i have other problems with sja1000_isa driver I can load the driver successfully. the data is transmitted successfully but its not able to receive any data.
Can Trace shows the data is transmitted from the device i am controlling but no interrupt is generated for received data i.e. no data is received :( The same can card work without any problems with 2.6.26 kernel when socketcan was not part of kernel.
I have double checked the interrupt jumpers and also the interrupt table in /proc/interrupts shows the interrupts are linked with 10 to can0 and 11 to can1 but unfortunately no interrupt

Any ideas ?

On Mon, Apr 21, 2014 at 8:39 PM, Oliver Hartkopp <[email protected] <mailto:[email protected]>> wrote:

    Hello Khurram,

    is it possible for you to test this patch with your hardware?

    It should apply on your used kernel without problems.

    Thanks & best regards,
    Oliver

    On 15.04.2014 19:30, Oliver Hartkopp wrote:
    > When accessing the SJA1000 controller registers in the indirect access mode,
    > writing the register number and reading/writing the data has to be an atomic
    > attempt.
    >
    > As the sja1000_isa driver is an old style driver with a fixed number of
    > instances the locking variable depends on the same index like all the other
    > configuration elements given on the module command line.
    >
    > As a positive side effect dev->dev_id is populated by the instance index,
    > which was missing in 3e66d0138c05d9 ("can: populate netdev::dev_id for udev
    > discrimination").
    >
    > Reported-by: Marc Kleine-Budde <[email protected]
    <mailto:[email protected]>>
    > Signed-off-by: Oliver Hartkopp <[email protected]
    <mailto:[email protected]>>
    >
    > ---
    >
    > diff --git a/drivers/net/can/sja1000/sja1000_isa.c
    b/drivers/net/can/sja1000/sja1000_isa.c
    > index df136a2..014695d 100644
    > --- a/drivers/net/can/sja1000/sja1000_isa.c
    > +++ b/drivers/net/can/sja1000/sja1000_isa.c
    > @@ -46,6 +46,7 @@ static int clk[MAXDEV];
    >  static unsigned char cdr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
    >  static unsigned char ocr[MAXDEV] = {[0 ... (MAXDEV - 1)] = 0xff};
    >  static int indirect[MAXDEV] = {[0 ... (MAXDEV - 1)] = -1};
    > +static spinlock_t indirect_lock[MAXDEV];  /* lock for indirect access
    mode */
    >
    >  module_param_array(port, ulong, NULL, S_IRUGO);
    >  MODULE_PARM_DESC(port, "I/O port number");
    > @@ -101,19 +102,26 @@ static void sja1000_isa_port_write_reg(const
    struct sja1000_priv *priv,
    >  static u8 sja1000_isa_port_read_reg_indirect(const struct sja1000_priv
    *priv,
    >                                            int reg)
    >  {
    > -     unsigned long base = (unsigned long)priv->reg_base;
    > +     unsigned long flags, base = (unsigned long)priv->reg_base;
    > +     u8 readval;
    >
    > +     spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
    >       outb(reg, base);
    > -     return inb(base + 1);
    > +     readval = inb(base + 1);
    > +     spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
    > +
    > +     return readval;
    >  }
    >
    >  static void sja1000_isa_port_write_reg_indirect(const struct
    sja1000_priv *priv,
    >                                               int reg, u8 val)
    >  {
    > -     unsigned long base = (unsigned long)priv->reg_base;
    > +     unsigned long flags, base = (unsigned long)priv->reg_base;
    >
    > +     spin_lock_irqsave(&indirect_lock[priv->dev->dev_id], flags);
    >       outb(reg, base);
    >       outb(val, base + 1);
    > +     spin_unlock_irqrestore(&indirect_lock[priv->dev->dev_id], flags);
    >  }
    >
    >  static int sja1000_isa_probe(struct platform_device *pdev)
    > @@ -169,6 +177,7 @@ static int sja1000_isa_probe(struct platform_device
    *pdev)
    >               if (iosize == SJA1000_IOSIZE_INDIRECT) {
    >                       priv->read_reg = sja1000_isa_port_read_reg_indirect;
    >                       priv->write_reg = sja1000_isa_port_write_reg_indirect;
    > +                     spin_lock_init(&indirect_lock[idx]);
    >               } else {
    >                       priv->read_reg = sja1000_isa_port_read_reg;
    >                       priv->write_reg = sja1000_isa_port_write_reg;
    > @@ -198,6 +207,7 @@ static int sja1000_isa_probe(struct platform_device
    *pdev)
    >
    >       platform_set_drvdata(pdev, dev);
    >       SET_NETDEV_DEV(dev, &pdev->dev);
    > +     dev->dev_id = idx;
    >
    >       err = register_sja1000dev(dev);
    >       if (err) {
    >
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-can" in
    > the body of a message to [email protected] <mailto:[email protected]>
    > More majordomo info at  http://vger.kernel.org/majordomo-info.html
    >

Oliver Hartkopp

2014-04-26 19:18 UTC #18 lore raw

PATCH 1/1 slip: fix spinlock variant

In reply to Marc Kleine-Budde

With commit cc9fa74e2a ("slip/slcan: added locking in wakeup function") a formerly missing locking was added to slip.c and slcan.c by Andre Naujoks.

Alexander Stein contributed the fix 367525c8c2 ("can: slcan: Fix spinlock variant") as the kernel lock debugging advised to use spin_lock_bh() instead of just using spin_lock().

This fix has to be applied to the same code section in slip.c for the same reason too.

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

---

drivers/net/slip/slip.c +3 -3

index cc70ecf..ad4a94e 100644
@@ -429,13 +429,13 @@ static void slip_write_wakeup(struct tty_struct *tty)
 	if (!sl || sl->magic != SLIP_MAGIC || !netif_running(sl->dev))
 		return;
 
-	spin_lock(&sl->lock);
+	spin_lock_bh(&sl->lock);
 	if (sl->xleft <= 0)  {
 		/* Now serial buffer is almost free & we can start
 		 * transmission of another packet */
 		sl->dev->stats.tx_packets++;
 		clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
-		spin_unlock(&sl->lock);
+		spin_unlock_bh(&sl->lock);
 		sl_unlock(sl);
 		return;
 	}

@@ -443,7 +443,7 @@ static void slip_write_wakeup(struct tty_struct *tty)
 	actual = tty->ops->write(tty, sl->xhead, sl->xleft);
 	sl->xleft -= actual;
 	sl->xhead += actual;
-	spin_unlock(&sl->lock);
+	spin_unlock_bh(&sl->lock);
 }
 
 static void sl_tx_timeout(struct net_device *dev)

David Miller

maintainer

2014-04-28 03:35 UTC #19 lore raw

Re: [PATCH] slip: fix spinlock variant

From: Oliver Hartkopp <[email protected]>
Date: Sat, 26 Apr 2014 21:18:32 +0200

Quoted 11 lines

With commit cc9fa74e2a ("slip/slcan: added locking in wakeup function") a formerly missing locking was added to slip.c and slcan.c by Andre Naujoks.
Alexander Stein contributed the fix 367525c8c2 ("can: slcan: Fix spinlock variant") as the kernel lock debugging advised to use spin_lock_bh() instead of just using spin_lock().
This fix has to be applied to the same code section in slip.c for the same reason too.
Signed-off-by Oliver Hartkopp <[email protected]>

Applied and queued up for -stable, thanks.