Skip to contentSkip to conversation

can: kvaser_usb: Add support for Kvaser CAN/USB devices

DISCUSSION Dan Carpenter 4 participants 7 messages Nov 19, 2015 – Nov 21, 2015 CAN network drivers

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Olivier Sobrie,

The patch 080f40a6fa28: "can: kvaser_usb: Add support for Kvaser CAN/USB devices" from Nov 21, 2012, leads to the following static checker warning:

	drivers/net/can/usb/kvaser_usb.c:949 kvaser_usb_rx_error()
	0x08 | 0x18 has 0x08 set on both sides

drivers/net/can/usb/kvaser_usb.c
941 switch (dev->family) {
942 case KVASER_LEAF:
943 if (es->leaf.error_factor) {
944 cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
945
946 if (es->leaf.error_factor & M16C_EF_ACKE)
947 cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
948 if (es->leaf.error_factor & M16C_EF_CRCE)
949 cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
950 CAN_ERR_PROT_LOC_CRC_DEL);

CAN_ERR_PROT_LOC_CRC_SEQ is 0x08
CAN_ERR_PROT_LOC_CRC_DEL is 0x18

It's weird that the bits overlap. Was that intentional? Why isn't it enough to just say?:

					cf->data[3] |= CAN_ERR_PROT_LOC_CRC_DEL;

The static checker complains about most places where CAN_ERR_PROT_LOC_CRC_SEQ is used. I see us setting the flag but not I don't see where we test for this so I'm not sure.

   951                          if (es->leaf.error_factor & M16C_EF_FORME)
   952                                  cf->data[2] |= CAN_ERR_PROT_FORM;
   953                          if (es->leaf.error_factor & M16C_EF_STFE)
   954                                  cf->data[2] |= CAN_ERR_PROT_STUFF;
   955                          if (es->leaf.error_factor & M16C_EF_BITE0)
   956                                  cf->data[2] |= CAN_ERR_PROT_BIT0;
   957                          if (es->leaf.error_factor & M16C_EF_BITE1)
   958                                  cf->data[2] |= CAN_ERR_PROT_BIT1;
   959                          if (es->leaf.error_factor & M16C_EF_TRE)
   960                                  cf->data[2] |= CAN_ERR_PROT_TX;
   961                  }
   962                  break;
   963          case KVASER_USBCAN:

regards,
dan carpenter

maintainer
#1 lore raw
Quoted 21 lines · Dan Carpenter

Hello Olivier Sobrie,

The patch 080f40a6fa28: "can: kvaser_usb: Add support for Kvaser CAN/USB devices" from Nov 21, 2012, leads to the following static checker warning:

	drivers/net/can/usb/kvaser_usb.c:949 kvaser_usb_rx_error()
	0x08 | 0x18 has 0x08 set on both sides

drivers/net/can/usb/kvaser_usb.c

   941          switch (dev->family) {
   942          case KVASER_LEAF:
   943                  if (es->leaf.error_factor) {
   944                          cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
   945  
   946                          if (es->leaf.error_factor & M16C_EF_ACKE)
   947                                  cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
   948                          if (es->leaf.error_factor & M16C_EF_CRCE)
   949                                  cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
   950                                                  CAN_ERR_PROT_LOC_CRC_DEL);
Quoted 6 lines

CAN_ERR_PROT_LOC_CRC_SEQ is 0x08
CAN_ERR_PROT_LOC_CRC_DEL is 0x18

It's weird that the bits overlap. Was that intentional? Why isn't it enough to just say?:

					cf->data[3] |= CAN_ERR_PROT_LOC_CRC_DEL;

Looking at include/uapi/linux/can/error.h I'd say, they are not meant to be used bitwise. Oliver?

Quoted 4 lines

The static checker complains about most places where CAN_ERR_PROT_LOC_CRC_SEQ is used. I see us setting the flag but not I don't see where we test for this so I'm not sure.

Quoted 13 lines
   951                          if (es->leaf.error_factor & M16C_EF_FORME)
   952                                  cf->data[2] |= CAN_ERR_PROT_FORM;
   953                          if (es->leaf.error_factor & M16C_EF_STFE)
   954                                  cf->data[2] |= CAN_ERR_PROT_STUFF;
   955                          if (es->leaf.error_factor & M16C_EF_BITE0)
   956                                  cf->data[2] |= CAN_ERR_PROT_BIT0;
   957                          if (es->leaf.error_factor & M16C_EF_BITE1)
   958                                  cf->data[2] |= CAN_ERR_PROT_BIT1;
   959                          if (es->leaf.error_factor & M16C_EF_TRE)
   960                                  cf->data[2] |= CAN_ERR_PROT_TX;
   961                  }
   962                  break;
   963          case KVASER_USBCAN:

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   |
#2 lore raw
Quoted 35 lines

Marc Kleine-Budde <[email protected]> hat am 19. November 2015 um 14:23 geschrieben:

Quoted 28 lines · Dan Carpenter

Hello Olivier Sobrie,

The patch 080f40a6fa28: "can: kvaser_usb: Add support for Kvaser CAN/USB devices" from Nov 21, 2012, leads to the following static checker warning:

	drivers/net/can/usb/kvaser_usb.c:949 kvaser_usb_rx_error()
	0x08 | 0x18 has 0x08 set on both sides

drivers/net/can/usb/kvaser_usb.c

   941          switch (dev->family) {
   942          case KVASER_LEAF:
   943                  if (es->leaf.error_factor) {
   944                          cf->can_id |= CAN_ERR_BUSERROR |
CAN_ERR_PROT;
   945  
   946                          if (es->leaf.error_factor & M16C_EF_ACKE)
   947                                  cf->data[3] |=
(CAN_ERR_PROT_LOC_ACK);
   948                          if (es->leaf.error_factor & M16C_EF_CRCE)
   949                                  cf->data[3] |=
(CAN_ERR_PROT_LOC_CRC_SEQ |
   950
                                                 CAN_ERR_PROT_LOC_CRC_DEL);

CAN_ERR_PROT_LOC_CRC_SEQ is 0x08
CAN_ERR_PROT_LOC_CRC_DEL is 0x18

Quoted 6 lines
Quoted 3 lines

It's weird that the bits overlap. Was that intentional? Why isn't it enough to just say?:

					cf->data[3] |= CAN_ERR_PROT_LOC_CRC_DEL;

Looking at include/uapi/linux/can/error.h I'd say, they are not meant to be used bitwise. Oliver?

You are right!

The values reflect more or less the incident position (location) where the error in bit stream processing occurred.

Instead of

cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ | ...

it should be

cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;

There's no need for bitwise OR'ed values in data[3].

Regards,
Oliver

#3 lore raw
In reply to Dan Carpenter

Hello Dan,

Quoted 24 lines · Dan Carpenter

Hello Olivier Sobrie,

The patch 080f40a6fa28: "can: kvaser_usb: Add support for Kvaser CAN/USB devices" from Nov 21, 2012, leads to the following static checker warning:

	drivers/net/can/usb/kvaser_usb.c:949 kvaser_usb_rx_error()
	0x08 | 0x18 has 0x08 set on both sides

drivers/net/can/usb/kvaser_usb.c

   941          switch (dev->family) {
   942          case KVASER_LEAF:
   943                  if (es->leaf.error_factor) {
   944                          cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
   945  
   946                          if (es->leaf.error_factor & M16C_EF_ACKE)
   947                                  cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
   948                          if (es->leaf.error_factor & M16C_EF_CRCE)
   949                                  cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
   950                                                  CAN_ERR_PROT_LOC_CRC_DEL);

CAN_ERR_PROT_LOC_CRC_SEQ is 0x08
CAN_ERR_PROT_LOC_CRC_DEL is 0x18

Quoted 7 lines

It's weird that the bits overlap. Was that intentional? Why isn't it enough to just say?:

					cf->data[3] |= CAN_ERR_PROT_LOC_CRC_DEL;

The static checker complains about most places where CAN_ERR_PROT_LOC_CRC_SEQ is used. I see us setting the flag but not I don't see where we test for this so I'm not sure.

No it wasn't intentional. I think that it was inspired from other CAN drivers. I see the same in net/can/c_can/c_can.c and net/can/m_can/m_can.c. Sorry for the error.
Btw, which static checker are you using?

As suggested by Oliver, I assume we can transform this in
cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
and fix all the other places where bitwise operations are done for errors in data[3].

Should I send a patch to fix this? Or do you or someone else plan to send a patch?

Thanks,

Olivier

author
#4 lore raw

Could you fix this and give me a reported-by tag? I can't test it and it's weird to me that CAN_ERR_PROT_LOC_CRC_SEQ is always used as a bitmap.

The static checker is a Smatch thing I'm working on but haven't published yet. It only finds this bug and a couple false positives.

regards,
dan carpenter

#5 lore raw
In reply to Olivier Sobrie

Hello Olivier,

Quoted 5 lines · Olivier Sobrie

No it wasn't intentional. I think that it was inspired from other CAN drivers. I see the same in net/can/c_can/c_can.c and net/can/m_can/m_can.c. Sorry for the error.
Btw, which static checker are you using?

Quoted 7 lines

As suggested by Oliver, I assume we can transform this in
cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
and fix all the other places where bitwise operations are done for errors in data[3].

Should I send a patch to fix this? Or do you or someone else plan to send a patch?

I already prepared a patch to fix the data[3] assignment issue.

It's a bunch of drivers that do it wrong:

drivers/net/can/c_can/c_can.c:
drivers/net/can/cc770/cc770.c:
drivers/net/can/flexcan.c:
drivers/net/can/m_can/m_can.c:
drivers/net/can/pch_can.c:
drivers/net/can/rcar_can.c:
drivers/net/can/ti_hecc.c:
drivers/net/can/usb/kvaser_usb.c:
drivers/net/can/usb/usb_8dev.c:
drivers/net/can/xilinx_can.c:

So if it's ok for you I'll post it this weekend :-)

Best regards,
Oliver

#6 lore raw
Quoted 32 lines · Oliver Hartkopp

Hello Olivier,

Quoted 5 lines · Olivier Sobrie

No it wasn't intentional. I think that it was inspired from other CAN drivers. I see the same in net/can/c_can/c_can.c and net/can/m_can/m_can.c. Sorry for the error.
Btw, which static checker are you using?

Quoted 7 lines

As suggested by Oliver, I assume we can transform this in
cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
and fix all the other places where bitwise operations are done for errors in data[3].

Should I send a patch to fix this? Or do you or someone else plan to send a patch?

I already prepared a patch to fix the data[3] assignment issue.

It's a bunch of drivers that do it wrong:

drivers/net/can/c_can/c_can.c:
drivers/net/can/cc770/cc770.c:
drivers/net/can/flexcan.c:
drivers/net/can/m_can/m_can.c:
drivers/net/can/pch_can.c:
drivers/net/can/rcar_can.c:
drivers/net/can/ti_hecc.c:
drivers/net/can/usb/kvaser_usb.c:
drivers/net/can/usb/usb_8dev.c:
drivers/net/can/xilinx_can.c:

Quoted 1 line

So if it's ok for you I'll post it this weekend :-)

Of course it's ok for me :-)
Thank you,

Olivier