re: can: kvaser_usb: Consolidate and unify state change handling

re: can: kvaser_usb: Consolidate and unify state change handling

[Dan Carpenter]

Hello Ahmed S. Darwish,

The patch 96d7f10634e6: "can: kvaser_usb: Consolidate and unify state
change handling" from Jan 26, 2015, leads to the following Smatch
warning:

	drivers/net/can/usb/kvaser_usb.c:827 kvaser_usb_rx_error_update_can_state()
	warn: impossible condition '(es->txerr >= 256) => (0-255 >= 256)'

drivers/net/can/usb/kvaser_usb.c
   824          else if (es->status & M16C_STATE_BUS_PASSIVE)
   825                  new_state = CAN_STATE_ERROR_PASSIVE;
   826          else if (es->status & M16C_STATE_BUS_ERROR) {
   827                  if ((es->txerr >= 256) || (es->rxerr >= 256))
   828                          new_state = CAN_STATE_BUS_OFF;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Dead code.  We normally ignore these "impossible condition" warnings if
it feels like the condition makes the code more readable.  But in this
case, I wonder if the code works as intended.  It's not clear to me.

   829                  else if ((es->txerr >= 128) || (es->rxerr >= 128))
   830                          new_state = CAN_STATE_ERROR_PASSIVE;
   831                  else if ((es->txerr >= 96) || (es->rxerr >= 96))

regards,
dan carpenter

Re: can: kvaser_usb: Consolidate and unify state change handling

[Marc Kleine-Budde]

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

Hey Dan,

On 02/05/2015 09:12 AM, Dan Carpenter wrote:
> The patch 96d7f10634e6: "can: kvaser_usb: Consolidate and unify state
> change handling" from Jan 26, 2015, leads to the following Smatch
> warning:
> 
> 	drivers/net/can/usb/kvaser_usb.c:827 kvaser_usb_rx_error_update_can_state()
> 	warn: impossible condition '(es->txerr >= 256) => (0-255 >= 256)'

A patch that implicitly fixes this warning, along with other
things, is in my latest pull request to David Miller.

    can: kvaser_usb: Ignore spurious error events after a busoff
    http://article.gmane.org/gmane.linux.can/7587
    http://www.spinics.net/lists/netdev/msg315062.html

regards,
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: 819 bytes --]

Re: can: kvaser_usb: Consolidate and unify state change handling

[Marc Kleine-Budde]

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

On 02/05/2015 09:17 AM, Marc Kleine-Budde wrote:
> Hey Dan,
> 
> On 02/05/2015 09:12 AM, Dan Carpenter wrote:
>> The patch 96d7f10634e6: "can: kvaser_usb: Consolidate and unify state
>> change handling" from Jan 26, 2015, leads to the following Smatch
>> warning:
>>
>> 	drivers/net/can/usb/kvaser_usb.c:827 kvaser_usb_rx_error_update_can_state()
>> 	warn: impossible condition '(es->txerr >= 256) => (0-255 >= 256)'
> 
> A patch that implicitly fixes this warning, along with other
> things, is in my latest pull request to David Miller.

...which has just been merged.

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