Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

[Wolfgang Grandegger]

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
<khurramgulzar@gmail.com> 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.

Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

[Oliver Hartkopp]

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
> <khurramgulzar@gmail.com> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

[Oliver Hartkopp]

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


On 15.04.2014 13:25, khurram gulzar wrote:
> 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 <socketcan@hartkopp.net
> <mailto:socketcan@hartkopp.net>> 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
>     > <khurramgulzar@gmail.com <mailto:khurramgulzar@gmail.com>> 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 majordomo@vger.kernel.org <mailto:majordomo@vger.kernel.org>
>     > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     >
> 
> 

Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

[Oliver Hartkopp]

As I wrote in the other thread:

> 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

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

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

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

should make it.

> I am now running linux kernel version 3.2 

A good choice :-)

Best regards,
Oliver

Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

[Marc Kleine-Budde]

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

On 04/15/2014 02:00 PM, Oliver Hartkopp wrote:
> As I wrote in the other thread:
> 
>> 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....

> 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);
> }
> 
> 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

-- 
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: 242 bytes --]

Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

[Oliver Hartkopp]

Something like this?

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

Just compile-tested by now ...

diff --git a/drivers/net/can/sja1000/sja1000_isa.c b/drivers/net/can/sja1000/sja1000_isa.c
index df136a2..46e1784 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,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) {



On 15.04.2014 14:33, Marc Kleine-Budde wrote:
> On 04/15/2014 02:00 PM, Oliver Hartkopp wrote:
>> As I wrote in the other thread:
>>
>>> 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....
> 
>> 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);
>> }
>>
>> 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
> 

Re: Problem with Eurotech COM1273 Dual Channel CAN PC104 Module.

[Marc Kleine-Budde]

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

On 04/15/2014 03:19 PM, Oliver Hartkopp wrote:
> Something like this?

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

Marc

> It uses the dev->dev_id which was missed in the 3e66d0138c05d9792f patch before.
> 
> Just compile-tested by now ...
> 
> diff --git a/drivers/net/can/sja1000/sja1000_isa.c b/drivers/net/can/sja1000/sja1000_isa.c
> index df136a2..46e1784 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,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) {
> 
> 
> 
> On 15.04.2014 14:33, Marc Kleine-Budde wrote:
>> On 04/15/2014 02:00 PM, Oliver Hartkopp wrote:
>>> As I wrote in the other thread:
>>>
>>>> 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....
>>
>>> 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);
>>> }
>>>
>>> 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
>>
> 


-- 
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: 242 bytes --]

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

[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 <mkl@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

---

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) {

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

[Thomas Gleixner]

On Tue, 15 Apr 2014, 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 <mkl@pengutronix.de>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> ---
> 
> 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 */

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
  

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

[Marc Kleine-Budde]

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

On 04/15/2014 11:26 PM, Thomas Gleixner wrote:
> On Tue, 15 Apr 2014, 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 <mkl@pengutronix.de>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>
>> ---
>>
>> 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 */
> 
> 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.

There's a spin_lock_init in the probe function:

>  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;

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: 242 bytes --]

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

[Thomas Gleixner]

On Tue, 15 Apr 2014, Marc Kleine-Budde wrote:

> On 04/15/2014 11:26 PM, Thomas Gleixner wrote:
> > On Tue, 15 Apr 2014, 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 <mkl@pengutronix.de>
> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >>
> >> ---
> >>
> >> 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 */
> > 
> > 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.
> 
> There's a spin_lock_init in the probe function:
> 
> >  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]);

Ooops, missed that :)

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

[Marc Kleine-Budde]

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

On 04/15/2014 07:30 PM, 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 <mkl@pengutronix.de>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

Applied to can/master.

Thanks,
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: 242 bytes --]

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

[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

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 <mkl@pengutronix.de>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> ---
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Oliver Hartkopp]

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

On 24.04.2014 11:17, khurram gulzar wrote:
> 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 <socketcan@hartkopp.net
> <mailto:socketcan@hartkopp.net>> 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 <mkl@pengutronix.de
>     <mailto:mkl@pengutronix.de>>
>     > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net
>     <mailto:socketcan@hartkopp.net>>
>     >
>     > ---
>     >
>     > 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 majordomo@vger.kernel.org <mailto:majordomo@vger.kernel.org>
>     > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     >
> 
> 

[PATCH] slip: fix spinlock variant

[Oliver Hartkopp]

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 <socketcan@hartkopp.net>

---

diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index cc70ecf..ad4a94e 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -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)

Re: [PATCH] slip: fix spinlock variant

[David Miller]

From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Sat, 26 Apr 2014 21:18:32 +0200

> 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 <socketcan@hartkopp.net>

Applied and queued up for -stable, thanks.