* Re: [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type [not found] <201501301018.HN7rQ3rB%fengguang.wu@intel.com> @ 2015-01-30 8:04 ` Marc Kleine-Budde 2015-01-30 11:26 ` Andri Yngvason 2015-02-01 12:04 ` Ahmed S. Darwish 0 siblings, 2 replies; 5+ messages in thread From: Marc Kleine-Budde @ 2015-01-30 8:04 UTC (permalink / raw) To: kbuild test robot; +Cc: kbuild-all, Ahmed S. Darwish, linux-can@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 2644 bytes --] Ahmed S. Darwish Cc'ed On 01/30/2015 03:53 AM, kbuild test robot wrote: > tree: jtkirshe-net-next/core-queue > head: c265eda3429432b5e9e53c8082861ffbd2ffd2c2 > commit: 96d7f10634e66b27e23854c774fbcc0a2a654e82 [1002/1025] can: kvaser_usb: Consolidate and unify state change handling > config: avr32-allyesconfig (attached as .config) > reproduce: > wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > git checkout 96d7f10634e66b27e23854c774fbcc0a2a654e82 > # save the attached .config to linux build tree > make.cross ARCH=avr32 > > All warnings: > > drivers/net/can/usb/kvaser_usb.c: In function 'kvaser_usb_rx_error_update_can_state': >>> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type >>> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type > > vim +639 drivers/net/can/usb/kvaser_usb.c > > 623 const struct kvaser_usb_error_summary *es, > 624 struct can_frame *cf) > 625 { > 626 struct net_device_stats *stats; > 627 enum can_state cur_state, new_state, tx_state, rx_state; > 628 > 629 netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status); > 630 > 631 stats = &priv->netdev->stats; > 632 new_state = cur_state = priv->can.state; > 633 > 634 if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) > 635 new_state = CAN_STATE_BUS_OFF; > 636 else if (es->status & M16C_STATE_BUS_PASSIVE) > 637 new_state = CAN_STATE_ERROR_PASSIVE; > 638 else if (es->status & M16C_STATE_BUS_ERROR) { > > 639 if ((es->txerr >= 256) || (es->rxerr >= 256)) > 640 new_state = CAN_STATE_BUS_OFF; > 641 else if ((es->txerr >= 128) || (es->rxerr >= 128)) > 642 new_state = CAN_STATE_ERROR_PASSIVE; > 643 else if ((es->txerr >= 96) || (es->rxerr >= 96)) > 644 new_state = CAN_STATE_ERROR_WARNING; > 645 else if (cur_state > CAN_STATE_ERROR_ACTIVE) > 646 new_state = CAN_STATE_ERROR_ACTIVE; > 647 } > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > http://lists.01.org/mailman/listinfo/kbuild Intel Corporation > -- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type 2015-01-30 8:04 ` [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type Marc Kleine-Budde @ 2015-01-30 11:26 ` Andri Yngvason 2015-02-01 13:00 ` Ahmed S. Darwish 2015-02-01 12:04 ` Ahmed S. Darwish 1 sibling, 1 reply; 5+ messages in thread From: Andri Yngvason @ 2015-01-30 11:26 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: kbuild-all, Ahmed S. Darwish, linux-can@vger.kernel.org Quoting Marc Kleine-Budde (2015-01-30 08:04:47) > Ahmed S. Darwish Cc'ed > > On 01/30/2015 03:53 AM, kbuild test robot wrote: > > tree: jtkirshe-net-next/core-queue > > head: c265eda3429432b5e9e53c8082861ffbd2ffd2c2 > > commit: 96d7f10634e66b27e23854c774fbcc0a2a654e82 [1002/1025] can: kvaser_usb: Consolidate and unify state change handling > > config: avr32-allyesconfig (attached as .config) > > reproduce: > > wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > git checkout 96d7f10634e66b27e23854c774fbcc0a2a654e82 > > # save the attached .config to linux build tree > > make.cross ARCH=avr32 > > > > All warnings: > > > > drivers/net/can/usb/kvaser_usb.c: In function 'kvaser_usb_rx_error_update_can_state': > >>> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type > >>> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type > > > > vim +639 drivers/net/can/usb/kvaser_usb.c > > > > 623 const struct kvaser_usb_error_summary *es, > > 624 struct can_frame *cf) > > 625 { > > 626 struct net_device_stats *stats; > > 627 enum can_state cur_state, new_state, tx_state, rx_state; > > 628 > > 629 netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status); > > 630 > > 631 stats = &priv->netdev->stats; > > 632 new_state = cur_state = priv->can.state; > > 633 > > 634 if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) > > 635 new_state = CAN_STATE_BUS_OFF; > > 636 else if (es->status & M16C_STATE_BUS_PASSIVE) > > 637 new_state = CAN_STATE_ERROR_PASSIVE; > > 638 else if (es->status & M16C_STATE_BUS_ERROR) { > > > 639 if ((es->txerr >= 256) || (es->rxerr >= 256)) > > 640 new_state = CAN_STATE_BUS_OFF; > > 641 else if ((es->txerr >= 128) || (es->rxerr >= 128)) > > 642 new_state = CAN_STATE_ERROR_PASSIVE; > > 643 else if ((es->txerr >= 96) || (es->rxerr >= 96)) > > 644 new_state = CAN_STATE_ERROR_WARNING; > > 645 else if (cur_state > CAN_STATE_ERROR_ACTIVE) > > 646 new_state = CAN_STATE_ERROR_ACTIVE; > > 647 } > > > > --- > > 0-DAY kernel test infrastructure Open Source Technology Center > > http://lists.01.org/mailman/listinfo/kbuild Intel Corporation > > > Isn't this redundant anyway? This is always handled by a different interrupt, right? In case it isn't you could probably do something like this: if ((es->txerr >= 128) || (es->rxerr >= 128)) { if(cur_state == CAN_STATE_ERROR_PASSIVE) new_state = CAN_STATE_BUS_OFF; else new_state = CAN_STATE_ERROR_PASSIVE; } else if ((es->txerr >= 96) || (es->rxerr >= 96)) { new_state = CAN_STATE_ERROR_WARNING; } else if (cur_state > CAN_STATE_ERROR_ACTIVE) { new_state = CAN_STATE_ERROR_ACTIVE; } -- Andri ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type 2015-01-30 11:26 ` Andri Yngvason @ 2015-02-01 13:00 ` Ahmed S. Darwish 0 siblings, 0 replies; 5+ messages in thread From: Ahmed S. Darwish @ 2015-02-01 13:00 UTC (permalink / raw) To: Andri Yngvason Cc: Marc Kleine-Budde, kbuild test robot, kbuild-all, Ahmed S. Darwish, linux-can@vger.kernel.org Hi! Andri wrote: > > drivers/net/can/usb/kvaser_usb.c: In function 'kvaser_usb_rx_error_update_can_state': > > >> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type > > >> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type > > > > vim +639 drivers/net/can/usb/kvaser_usb.c > > > > 623 const struct kvaser_usb_error_summary *es, > > 624 struct can_frame *cf) > > 625 { > > 626 struct net_device_stats *stats; > > 627 enum can_state cur_state, new_state, tx_state, rx_state; > > 628 > > 629 netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status); > > 630 > > 631 stats = &priv->netdev->stats; > > 632 new_state = cur_state = priv->can.state; > > 633 > > 634 if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) > > 635 new_state = CAN_STATE_BUS_OFF; > > 636 else if (es->status & M16C_STATE_BUS_PASSIVE) > > 637 new_state = CAN_STATE_ERROR_PASSIVE; > > 638 else if (es->status & M16C_STATE_BUS_ERROR) { > > > 639 if ((es->txerr >= 256) || (es->rxerr >= 256)) > > 640 new_state = CAN_STATE_BUS_OFF; > > 641 else if ((es->txerr >= 128) || (es->rxerr >= 128)) > > 642 new_state = CAN_STATE_ERROR_PASSIVE; > > 643 else if ((es->txerr >= 96) || (es->rxerr >= 96)) > > 644 new_state = CAN_STATE_ERROR_WARNING; > > 645 else if (cur_state > CAN_STATE_ERROR_ACTIVE) > > 646 new_state = CAN_STATE_ERROR_ACTIVE; > > 647 } > > > > Isn't this redundant anyway? This is always handled by a different > interrupt, right? > The BUS_ERROR check might have been a bit pedantic indeed, but I've faced a number of cases where the firmware reported error counters > 128 without having any M16C_STATE_BUS_PASSIVE flags checked either in the status flag of the current message or any previous messages. That's why I've added such elaborate checks. But since on bus error, good state was always reported and the driver did indeed transition to bus error state as expected, and since also the checks the compiler warns about doesn't work anyway, I'll just remove the two lines: 639 if ((es->txerr >= 256) || (es->rxerr >= 256)) 640 new_state = CAN_STATE_BUS_OFF; And test the state changes again. > In case it isn't you could probably do something like this: > if ((es->txerr >= 128) || (es->rxerr >= 128)) { > if(cur_state == CAN_STATE_ERROR_PASSIVE) > new_state = CAN_STATE_BUS_OFF; > else > new_state = CAN_STATE_ERROR_PASSIVE; That unfortunately won't work. As can be seen by candump traces I've sent on submission v6 here: http://article.gmane.org/gmane.linux.can/7485 The firmware can report error counters, in an increasing fashion but in the same state range several times. So unless I'm missing something, I'll just remove the useless checks and see what happens. > } else if ((es->txerr >= 96) || (es->rxerr >= 96)) { > new_state = CAN_STATE_ERROR_WARNING; > } else if (cur_state > CAN_STATE_ERROR_ACTIVE) { > new_state = CAN_STATE_ERROR_ACTIVE; > } > Thanks, Darwish ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type 2015-01-30 8:04 ` [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type Marc Kleine-Budde 2015-01-30 11:26 ` Andri Yngvason @ 2015-02-01 12:04 ` Ahmed S. Darwish 2015-02-04 12:48 ` Ahmed S. Darwish 1 sibling, 1 reply; 5+ messages in thread From: Ahmed S. Darwish @ 2015-02-01 12:04 UTC (permalink / raw) To: Marc Kleine-Budde Cc: kbuild test robot, kbuild-all, Ahmed S. Darwish, linux-can@vger.kernel.org Hi! Quoting buildbot: > tree: jtkirshe-net-next/core-queue > head: c265eda3429432b5e9e53c8082861ffbd2ffd2c2 > commit: 96d7f10634e66b27e23854c774fbcc0a2a654e82 [1002/1025] can: kvaser_usb: Consolidate and unify state change handling > config: avr32-allyesconfig (attached as .config) > reproduce: > wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > git checkout 96d7f10634e66b27e23854c774fbcc0a2a654e82 > # save the attached .config to linux build tree > make.cross ARCH=avr32 > > All warnings: > > drivers/net/can/usb/kvaser_usb.c: In function 'kvaser_usb_rx_error_update_can_state': > >> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type > >> drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type > Oh, my apologies. I wonder why such warning did not appear on my end even though I've compiled and tested the driver tens of times. $ gcc --version gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2 Either because I was using 'make M=drivers/net/can/usb/' all the time or there's some compiler flag config I've unchecked... > vim +639 drivers/net/can/usb/kvaser_usb.c > > 623 const struct kvaser_usb_error_summary *es, > 624 struct can_frame *cf) > 625 { > 626 struct net_device_stats *stats; > 627 enum can_state cur_state, new_state, tx_state, rx_state; > 628 > 629 netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status); > 630 > 631 stats = &priv->netdev->stats; > 632 new_state = cur_state = priv->can.state; > 633 > 634 if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) > 635 new_state = CAN_STATE_BUS_OFF; > 636 else if (es->status & M16C_STATE_BUS_PASSIVE) > 637 new_state = CAN_STATE_ERROR_PASSIVE; > 638 else if (es->status & M16C_STATE_BUS_ERROR) { > > 639 if ((es->txerr >= 256) || (es->rxerr >= 256)) > 640 new_state = CAN_STATE_BUS_OFF; I'll submit a fix shortly, and see how to cleanly handle the same issue in the patches merged in -next > 641 else if ((es->txerr >= 128) || (es->rxerr >= 128)) > 642 new_state = CAN_STATE_ERROR_PASSIVE; > 643 else if ((es->txerr >= 96) || (es->rxerr >= 96)) > 644 new_state = CAN_STATE_ERROR_WARNING; > 645 else if (cur_state > CAN_STATE_ERROR_ACTIVE) > 646 new_state = CAN_STATE_ERROR_ACTIVE; > 647 } Thanks, Darwish ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type 2015-02-01 12:04 ` Ahmed S. Darwish @ 2015-02-04 12:48 ` Ahmed S. Darwish 0 siblings, 0 replies; 5+ messages in thread From: Ahmed S. Darwish @ 2015-02-04 12:48 UTC (permalink / raw) To: Marc Kleine-Budde Cc: kbuild test robot, kbuild-all, Ahmed S. Darwish, linux-can@vger.kernel.org, Andri Yngvason On Sun, Feb 01, 2015 at 07:04:56AM -0500, Ahmed S. Darwish wrote: > Hi! > > Quoting buildbot: > ... > > >> drivers/net/can/usb/kvaser_usb.c:639: warning: > > >> comparison is always false due to limited range of data type > > >> drivers/net/can/usb/kvaser_usb.c:639: warning: > > >> comparison is always false due to limited range of data type > > > > Oh, my apologies. I wonder why such warning did not appear on my > end even though I've compiled and tested the driver tens of times. > > $ gcc --version > gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2 > > Either because I was using 'make M=drivers/net/can/usb/' all the time > or there's some compiler flag config I've unchecked... > > > vim +639 drivers/net/can/usb/kvaser_usb.c > > > > 623 const struct kvaser_usb_error_summary *es, > > 624 struct can_frame *cf) > > 625 { > > 626 struct net_device_stats *stats; > > 627 enum can_state cur_state, new_state, tx_state, rx_state; > > 628 > > 629 netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status); > > 630 > > 631 stats = &priv->netdev->stats; > > 632 new_state = cur_state = priv->can.state; > > 633 > > 634 if (es->status & (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) > > 635 new_state = CAN_STATE_BUS_OFF; > > 636 else if (es->status & M16C_STATE_BUS_PASSIVE) > > 637 new_state = CAN_STATE_ERROR_PASSIVE; > > 638 else if (es->status & M16C_STATE_BUS_ERROR) { > > >639 if ((es->txerr >= 256) || (es->rxerr >= 256)) > > 640 new_state = CAN_STATE_BUS_OFF; > > I'll submit a fix shortly, and see how to cleanly handle the same > issue in the patches merged in -next > A patch that implicitly fixes this warning, along with other things, has now been posted to linux-can: can: kvaser_usb: Ignore spurious error events after a busoff http://article.gmane.org/gmane.linux.can/7587 > > 641 else if ((es->txerr >= 128) || (es->rxerr >= 128)) > > 642 new_state = CAN_STATE_ERROR_PASSIVE; > > 643 else if ((es->txerr >= 96) || (es->rxerr >= 96)) > > 644 new_state = CAN_STATE_ERROR_WARNING; > > 645 else if (cur_state > CAN_STATE_ERROR_ACTIVE) > > 646 new_state = CAN_STATE_ERROR_ACTIVE; > > 647 } > Regards, Darwish ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-02-04 12:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201501301018.HN7rQ3rB%fengguang.wu@intel.com>
2015-01-30 8:04 ` [jtkirshe-net-next:core-queue 1002/1025] drivers/net/can/usb/kvaser_usb.c:639: warning: comparison is always false due to limited range of data type Marc Kleine-Budde
2015-01-30 11:26 ` Andri Yngvason
2015-02-01 13:00 ` Ahmed S. Darwish
2015-02-01 12:04 ` Ahmed S. Darwish
2015-02-04 12:48 ` Ahmed S. Darwish
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.