* 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 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-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-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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).