* Re: peak-usb: peak_usb_netif_rx() ts_high ununsed
From: Marc Kleine-Budde @ 2017-12-08 9:59 UTC (permalink / raw)
To: Stéphane Grosjean, linux-can
In-Reply-To: <VI1PR0302MB33255A4E1224FB34C7B70572D6320@VI1PR0302MB3325.eurprd03.prod.outlook.com>
[-- Attachment #1.1: Type: text/plain, Size: 686 bytes --]
On 12/06/2017 04:04 PM, Stéphane Grosjean wrote:
> Yep, you're right: peak_usb_get_ts_time() handles 32-bit wrapping
> when computing delay between two consecutives events, so the ts_high
> argument is useless.
ts_high is never used in the driver now.
> Consequently, each call to "peak_usb_netif_rx(a, b, c, d);" in
> pcan_usb_fd.c can be changed into "peak_usb_netif_rx(a, b, c);"
ACK.
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: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] can: peak/pcie_fd: fix potential bug in restarting tx queue
From: Marc Kleine-Budde @ 2017-12-08 9:46 UTC (permalink / raw)
To: Stephane Grosjean, Oliver Hartkopp; +Cc: linux-can Mailing List
In-Reply-To: <20171207151343.26923-1-s.grosjean@peak-system.com>
[-- Attachment #1.1: Type: text/plain, Size: 643 bytes --]
On 12/07/2017 04:13 PM, Stephane Grosjean wrote:
> Don't rely on can_get_echo_skb() return value to wake the network tx
> queue up: can_get_echo_skb() returns 0 if the echo array slot was not
> occupied, but also when the DLC of the released echo frame was 0.
>
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
Applied to can.
Tnx,
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: 488 bytes --]
^ permalink raw reply
* [PATCH] can: peak/pcie_fd: fix potential bug in restarting tx queue
From: Stephane Grosjean @ 2017-12-07 15:13 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can Mailing List, Stephane Grosjean
Don't rely on can_get_echo_skb() return value to wake the network tx
queue up: can_get_echo_skb() returns 0 if the echo array slot was not
occupied, but also when the DLC of the released echo frame was 0.
Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
drivers/net/can/peak_canfd/peak_canfd.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
index 85268be0c913..55513411a82e 100644
--- a/drivers/net/can/peak_canfd/peak_canfd.c
+++ b/drivers/net/can/peak_canfd/peak_canfd.c
@@ -258,21 +258,18 @@ static int pucan_handle_can_rx(struct peak_canfd_priv *priv,
/* if this frame is an echo, */
if ((rx_msg_flags & PUCAN_MSG_LOOPED_BACK) &&
!(rx_msg_flags & PUCAN_MSG_SELF_RECEIVE)) {
- int n;
unsigned long flags;
spin_lock_irqsave(&priv->echo_lock, flags);
- n = can_get_echo_skb(priv->ndev, msg->client);
+ can_get_echo_skb(priv->ndev, msg->client);
spin_unlock_irqrestore(&priv->echo_lock, flags);
/* count bytes of the echo instead of skb */
stats->tx_bytes += cf_len;
stats->tx_packets++;
- if (n) {
- /* restart tx queue only if a slot is free */
- netif_wake_queue(priv->ndev);
- }
+ /* restart tx queue (a slot is free) */
+ netif_wake_queue(priv->ndev);
return 0;
}
--
2.14.1
^ permalink raw reply related
* RE: peak-usb: peak_usb_netif_rx() ts_high ununsed
From: Stéphane Grosjean @ 2017-12-06 15:04 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
In-Reply-To: <ee2e4702-5d2d-b6fc-a90c-5fc9b0d3f362@pengutronix.de>
Hello,
Yep, you're right: peak_usb_get_ts_time() handles 32-bit wrapping when computing delay between two consecutives events, so the ts_high argument is useless.
Consequently, each call to "peak_usb_netif_rx(a, b, c, d);" in pcan_usb_fd.c can be changed into "peak_usb_netif_rx(a, b, c);"
Stéphane.
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
Sent: mercredi 6 décembre 2017 15:09
To: linux-can <linux-can@vger.kernel.org>; Stéphane Grosjean <s.grosjean@peak-system.com>
Subject: peak-usb: peak_usb_netif_rx() ts_high ununsed
Hello,
we just found out that ts_high is never used:
> /*
> * post received skb after having set any hw timestamp */ int
> peak_usb_netif_rx(struct sk_buff *skb,
> struct peak_time_ref *time_ref, u32 ts_low, u32 ts_high) {
> struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
>
> peak_usb_get_ts_time(time_ref, ts_low, &hwts->hwtstamp);
>
> return netif_rx(skb);
> }
Is that expected? If yes, let's remove ts_high.
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 |
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
--
^ permalink raw reply
* [mkl-can-next:af_can 5/18] net/can/af_can.c:890:25: error: 'struct netns_can' has no member named 'can_stattimer'
From: kbuild test robot @ 2017-12-06 14:32 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: kbuild-all, linux-can
[-- Attachment #1: Type: text/plain, Size: 8101 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git af_can
head: f0c2db14e47365b89dd8e76a365f0f9d5a48a914
commit: 50f0b88d236c1404945f5707dd6eceb8564f0290 [5/18] can: netns: remove "can_" prefix from members struct netns_can
config: i386-randconfig-a0-201749 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
git checkout 50f0b88d236c1404945f5707dd6eceb8564f0290
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from include/linux/workqueue.h:9:0,
from include/linux/srcu.h:34,
from include/linux/notifier.h:16,
from include/linux/memory_hotplug.h:7,
from include/linux/mmzone.h:775,
from include/linux/gfp.h:6,
from include/linux/umh.h:4,
from include/linux/kmod.h:22,
from include/linux/module.h:13,
from net/can/af_can.c:43:
net/can/af_can.c: In function 'can_pernet_init':
>> net/can/af_can.c:890:25: error: 'struct netns_can' has no member named 'can_stattimer'
timer_setup(&net->can.can_stattimer, can_stat_update,
^
include/linux/timer.h:105:19: note: in definition of macro '__init_timer'
init_timer_key((_timer), (_fn), (_flags), #_timer, &__key);\
^
net/can/af_can.c:890:4: note: in expansion of macro 'timer_setup'
timer_setup(&net->can.can_stattimer, can_stat_update,
^
net/can/af_can.c:892:23: error: 'struct netns_can' has no member named 'can_stattimer'
mod_timer(&net->can.can_stattimer,
^
--
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from net/can/proc.c:42:
net/can/proc.c: In function 'can_stat_update':
>> net/can/proc.c:120:42: error: 'struct netns_can' has no member named 'can_stattimer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
include/linux/compiler.h:301:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^
include/linux/compiler.h:324:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/build_bug.h:47:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
include/linux/kernel.h:930:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/kernel.h:930:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/timer.h:144:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^
net/can/proc.c:120:20: note: in expansion of macro 'from_timer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
In file included from include/linux/compiler_types.h:58:0,
from include/uapi/linux/stddef.h:2,
from include/linux/stddef.h:5,
from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/list.h:5,
from include/linux/module.h:9,
from net/can/proc.c:42:
include/linux/compiler-gcc.h:166:2: error: 'struct netns_can' has no member named 'can_stattimer'
__builtin_offsetof(a, b)
^
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^
include/linux/kernel.h:933:21: note: in expansion of macro 'offsetof'
((type *)(__mptr - offsetof(type, member))); })
^
include/linux/timer.h:144:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^
net/can/proc.c:120:20: note: in expansion of macro 'from_timer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
net/can/proc.c: In function 'can_stats_proc_show':
net/can/proc.c:224:14: error: 'struct netns_can' has no member named 'can_stattimer'
if (net->can.can_stattimer.function == can_stat_update) {
^
net/can/proc.c: In function 'can_reset_stats_proc_show':
net/can/proc.c:294:14: error: 'struct netns_can' has no member named 'can_stattimer'
if (net->can.can_stattimer.function == can_stat_update) {
^
vim +890 net/can/af_can.c
0d66548a Oliver Hartkopp 2007-11-16 872
8e8cda6d Mario Kicherer 2017-02-21 873 static int can_pernet_init(struct net *net)
8e8cda6d Mario Kicherer 2017-02-21 874 {
50f0b88d Marc Kleine-Budde 2017-08-28 875 spin_lock_init(&net->can.rcvlists_lock);
50f0b88d Marc Kleine-Budde 2017-08-28 876 net->can.rx_alldev_list =
8e8cda6d Mario Kicherer 2017-02-21 877 kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL);
50f0b88d Marc Kleine-Budde 2017-08-28 878 if (!net->can.rx_alldev_list)
5a606223 Marc Kleine-Budde 2017-07-29 879 goto out;
54c8b3c2 Marc Kleine-Budde 2017-08-28 880 net->can.pkg_stats = kzalloc(sizeof(struct can_pkg_stats), GFP_KERNEL);
54c8b3c2 Marc Kleine-Budde 2017-08-28 881 if (!net->can.pkg_stats)
5a606223 Marc Kleine-Budde 2017-07-29 882 goto out_free_alldev_list;
54c8b3c2 Marc Kleine-Budde 2017-08-28 883 net->can.rcv_lists_stats = kzalloc(sizeof(struct can_rcv_lists_stats), GFP_KERNEL);
54c8b3c2 Marc Kleine-Budde 2017-08-28 884 if (!net->can.rcv_lists_stats)
5317dd6d Marc Kleine-Budde 2017-08-28 885 goto out_free_pkg_stats;
cb5635a3 Oliver Hartkopp 2017-04-25 886
cb5635a3 Oliver Hartkopp 2017-04-25 887 if (IS_ENABLED(CONFIG_PROC_FS)) {
cb5635a3 Oliver Hartkopp 2017-04-25 888 /* the statistics are updated every second (timer triggered) */
cb5635a3 Oliver Hartkopp 2017-04-25 889 if (stats_timer) {
1fccb565 Kees Cook 2017-10-16 @890 timer_setup(&net->can.can_stattimer, can_stat_update,
1fccb565 Kees Cook 2017-10-16 891 0);
cb5635a3 Oliver Hartkopp 2017-04-25 892 mod_timer(&net->can.can_stattimer,
cb5635a3 Oliver Hartkopp 2017-04-25 893 round_jiffies(jiffies + HZ));
cb5635a3 Oliver Hartkopp 2017-04-25 894 }
54c8b3c2 Marc Kleine-Budde 2017-08-28 895 net->can.pkg_stats->jiffies_init = jiffies;
8e8cda6d Mario Kicherer 2017-02-21 896 can_init_proc(net);
cb5635a3 Oliver Hartkopp 2017-04-25 897 }
8e8cda6d Mario Kicherer 2017-02-21 898
8e8cda6d Mario Kicherer 2017-02-21 899 return 0;
5a606223 Marc Kleine-Budde 2017-07-29 900
5317dd6d Marc Kleine-Budde 2017-08-28 901 out_free_pkg_stats:
54c8b3c2 Marc Kleine-Budde 2017-08-28 902 kfree(net->can.pkg_stats);
5a606223 Marc Kleine-Budde 2017-07-29 903 out_free_alldev_list:
50f0b88d Marc Kleine-Budde 2017-08-28 904 kfree(net->can.rx_alldev_list);
5a606223 Marc Kleine-Budde 2017-07-29 905 out:
5a606223 Marc Kleine-Budde 2017-07-29 906 return -ENOMEM;
8e8cda6d Mario Kicherer 2017-02-21 907 }
8e8cda6d Mario Kicherer 2017-02-21 908
:::::: The code at line 890 was first introduced by commit
:::::: 1fccb565e8b09e54467d41111f6faf08fcc9c3c1 net: can: Convert timers to use timer_setup()
:::::: TO: Kees Cook <keescook@chromium.org>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29240 bytes --]
^ permalink raw reply
* [mkl-can-next:af_can 5/18] include/linux/compiler-gcc.h:166:2: error: 'struct netns_can' has no member named 'can_stattimer'
From: kbuild test robot @ 2017-12-06 14:29 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: kbuild-all, linux-can
[-- Attachment #1: Type: text/plain, Size: 7938 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git af_can
head: f0c2db14e47365b89dd8e76a365f0f9d5a48a914
commit: 50f0b88d236c1404945f5707dd6eceb8564f0290 [5/18] can: netns: remove "can_" prefix from members struct netns_can
config: i386-randconfig-sb0-12061831 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
git checkout 50f0b88d236c1404945f5707dd6eceb8564f0290
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from net/can/proc.c:42:
net/can/proc.c: In function 'can_stat_update':
net/can/proc.c:120:42: error: 'struct netns_can' has no member named 'can_stattimer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
include/linux/compiler.h:301:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^
include/linux/compiler.h:324:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/build_bug.h:47:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
include/linux/kernel.h:930:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/kernel.h:930:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/timer.h:144:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^
net/can/proc.c:120:20: note: in expansion of macro 'from_timer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
In file included from include/linux/compiler_types.h:58:0,
from include/uapi/linux/stddef.h:2,
from include/linux/stddef.h:5,
from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/list.h:5,
from include/linux/module.h:9,
from net/can/proc.c:42:
>> include/linux/compiler-gcc.h:166:2: error: 'struct netns_can' has no member named 'can_stattimer'
__builtin_offsetof(a, b)
^
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^
include/linux/kernel.h:933:21: note: in expansion of macro 'offsetof'
((type *)(__mptr - offsetof(type, member))); })
^
include/linux/timer.h:144:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^
net/can/proc.c:120:20: note: in expansion of macro 'from_timer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
net/can/proc.c: In function 'can_stats_proc_show':
net/can/proc.c:224:14: error: 'struct netns_can' has no member named 'can_stattimer'
if (net->can.can_stattimer.function == can_stat_update) {
^
net/can/proc.c: In function 'can_reset_stats_proc_show':
net/can/proc.c:294:14: error: 'struct netns_can' has no member named 'can_stattimer'
if (net->can.can_stattimer.function == can_stat_update) {
^
--
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from net//can/proc.c:42:
net//can/proc.c: In function 'can_stat_update':
net//can/proc.c:120:42: error: 'struct netns_can' has no member named 'can_stattimer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
include/linux/compiler.h:301:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^
include/linux/compiler.h:324:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/build_bug.h:47:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
include/linux/kernel.h:930:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/kernel.h:930:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/timer.h:144:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^
net//can/proc.c:120:20: note: in expansion of macro 'from_timer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
In file included from include/linux/compiler_types.h:58:0,
from include/uapi/linux/stddef.h:2,
from include/linux/stddef.h:5,
from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/list.h:5,
from include/linux/module.h:9,
from net//can/proc.c:42:
>> include/linux/compiler-gcc.h:166:2: error: 'struct netns_can' has no member named 'can_stattimer'
__builtin_offsetof(a, b)
^
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^
include/linux/kernel.h:933:21: note: in expansion of macro 'offsetof'
((type *)(__mptr - offsetof(type, member))); })
^
include/linux/timer.h:144:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^
net//can/proc.c:120:20: note: in expansion of macro 'from_timer'
struct net *net = from_timer(net, t, can.can_stattimer);
^
net//can/proc.c: In function 'can_stats_proc_show':
net//can/proc.c:224:14: error: 'struct netns_can' has no member named 'can_stattimer'
if (net->can.can_stattimer.function == can_stat_update) {
^
net//can/proc.c: In function 'can_reset_stats_proc_show':
net//can/proc.c:294:14: error: 'struct netns_can' has no member named 'can_stattimer'
if (net->can.can_stattimer.function == can_stat_update) {
^
vim +166 include/linux/compiler-gcc.h
cb984d10 Joe Perches 2015-06-25 163
cb984d10 Joe Perches 2015-06-25 164 #define __used __attribute__((__used__))
cb984d10 Joe Perches 2015-06-25 165 #define __compiler_offsetof(a, b) \
cb984d10 Joe Perches 2015-06-25 @166 __builtin_offsetof(a, b)
cb984d10 Joe Perches 2015-06-25 167
:::::: The code at line 166 was first introduced by commit
:::::: cb984d101b30eb7478d32df56a0023e4603cba7f compiler-gcc: integrate the various compiler-gcc[345].h files
:::::: TO: Joe Perches <joe@perches.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27019 bytes --]
^ permalink raw reply
* peak-usb: peak_usb_netif_rx() ts_high ununsed
From: Marc Kleine-Budde @ 2017-12-06 14:09 UTC (permalink / raw)
To: linux-can, Stephane Grosjean
[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]
Hello,
we just found out that ts_high is never used:
> /*
> * post received skb after having set any hw timestamp
> */
> int peak_usb_netif_rx(struct sk_buff *skb,
> struct peak_time_ref *time_ref, u32 ts_low, u32 ts_high)
> {
> struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
>
> peak_usb_get_ts_time(time_ref, ts_low, &hwts->hwtstamp);
>
> return netif_rx(skb);
> }
Is that expected? If yes, let's remove ts_high.
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: 488 bytes --]
^ permalink raw reply
* Re: [mkl-can-next:af_can 16/18] drivers/net/can/slcan.c:531:51: error: invalid application of 'sizeof' to incomplete type 'struct can_ml_priv'
From: Marc Kleine-Budde @ 2017-12-06 13:13 UTC (permalink / raw)
To: kbuild test robot; +Cc: kbuild-all, linux-can
In-Reply-To: <201712062055.bgtxdnoo%fengguang.wu@intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 1217 bytes --]
On 12/06/2017 01:49 PM, kbuild test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git af_can
> head: f0c2db14e47365b89dd8e76a365f0f9d5a48a914
> commit: b6ecae37409c9654a3d6c4908ebfd6e0c1939c0c [16/18] can: introduce CAN midlayer private and allocate it automatically
> config: x86_64-randconfig-x001-201749 (attached as .config)
> compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
> reproduce:
> git checkout b6ecae37409c9654a3d6c4908ebfd6e0c1939c0c
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
> drivers/net/can/slcan.c: In function 'slc_alloc':
>>> drivers/net/can/slcan.c:531:51: error: invalid application of 'sizeof' to incomplete type 'struct can_ml_priv'
> size = ALIGN(sizeof(*sl), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
Should be fixed already.
Tnx,
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: 488 bytes --]
^ permalink raw reply
* [mkl-can-next:af_can 16/18] drivers/net/can/slcan.c:531:51: error: invalid application of 'sizeof' to incomplete type 'struct can_ml_priv'
From: kbuild test robot @ 2017-12-06 12:49 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: kbuild-all, linux-can
[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git af_can
head: f0c2db14e47365b89dd8e76a365f0f9d5a48a914
commit: b6ecae37409c9654a3d6c4908ebfd6e0c1939c0c [16/18] can: introduce CAN midlayer private and allocate it automatically
config: x86_64-randconfig-x001-201749 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
git checkout b6ecae37409c9654a3d6c4908ebfd6e0c1939c0c
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/net/can/slcan.c: In function 'slc_alloc':
>> drivers/net/can/slcan.c:531:51: error: invalid application of 'sizeof' to incomplete type 'struct can_ml_priv'
size = ALIGN(sizeof(*sl), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
^~~~~~
--
>> drivers/net/can/vxcan.c:295:71: error: invalid application of 'sizeof' to incomplete type 'struct can_ml_priv'
.priv_size = ALIGN(sizeof(struct vxcan_priv), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
^~~~~~
>> drivers/net/can/vxcan.c:295:90: error: expected '}' before ';' token
.priv_size = ALIGN(sizeof(struct vxcan_priv), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
^
drivers/net/can/vxcan.c:285:20: warning: 'vxcan_get_link_net' defined but not used [-Wunused-function]
static struct net *vxcan_get_link_net(const struct net_device *dev)
^~~~~~~~~~~~~~~~~~
drivers/net/can/vxcan.c:259:13: warning: 'vxcan_dellink' defined but not used [-Wunused-function]
static void vxcan_dellink(struct net_device *dev, struct list_head *head)
^~~~~~~~~~~~~
drivers/net/can/vxcan.c:165:12: warning: 'vxcan_newlink' defined but not used [-Wunused-function]
static int vxcan_newlink(struct net *net, struct net_device *dev,
^~~~~~~~~~~~~
drivers/net/can/vxcan.c:150:13: warning: 'vxcan_setup' defined but not used [-Wunused-function]
static void vxcan_setup(struct net_device *dev)
^~~~~~~~~~~
vim +531 drivers/net/can/slcan.c
509
510 /* Find a free SLCAN channel, and link in this `tty' line. */
511 static struct slcan *slc_alloc(dev_t line)
512 {
513 int i;
514 char name[IFNAMSIZ];
515 struct net_device *dev = NULL;
516 struct slcan *sl;
517 int size;
518
519 for (i = 0; i < maxdev; i++) {
520 dev = slcan_devs[i];
521 if (dev == NULL)
522 break;
523
524 }
525
526 /* Sorry, too many, all slots in use */
527 if (i >= maxdev)
528 return NULL;
529
530 sprintf(name, "slcan%d", i);
> 531 size = ALIGN(sizeof(*sl), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
532 dev = alloc_netdev(size, name, NET_NAME_UNKNOWN, slc_setup);
533 if (!dev)
534 return NULL;
535
536 dev->base_addr = i;
537 sl = netdev_priv(dev);
538
539 /* Initialize channel control data */
540 sl->magic = SLCAN_MAGIC;
541 sl->dev = dev;
542 spin_lock_init(&sl->lock);
543 INIT_WORK(&sl->tx_work, slcan_transmit);
544 slcan_devs[i] = dev;
545
546 return sl;
547 }
548
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27492 bytes --]
^ permalink raw reply
* Re: CAN-USB adapter unplug
From: Jimmy Assarsson @ 2017-12-06 9:30 UTC (permalink / raw)
To: Martin Kelly, linux-can@vger.kernel.org
Cc: Marc Kleine-Budde, Stefan Mätje,
Remigiusz Kołłątaj, Wolfgang Grandegger
In-Reply-To: <f28efe8a-e700-d6e8-6b2a-f95e92396bb6@xevo.com>
On 2017-12-06 00:42, Martin Kelly wrote:
> On 12/04/2017 12:39 AM, Jimmy Assarsson wrote:
>> On 2017-11-30 19:19, Martin Kelly wrote:
>>> On 11/30/2017 02:18 AM, Jimmy Assarsson wrote:
>>>> On 2017-11-29 18:07, Martin Kelly wrote:
>>>>> On 11/29/2017 07:21 AM, Stefan Mätje wrote:
>>>>>> Am 29.11.2017 um 13:20 schrieb Marc Kleine-Budde:
>>>>>>> On 11/28/2017 10:09 PM, Martin Kelly wrote:
>>>>>>>>> Both applied to can.
>>>>>>>>
>>>>>>>> Thanks! By the way as far as I can tell from code inspection, it
>>>>>>>> appears
>>>>>>>> that most of the other drivers in net/can/usb should have the same
>>>>>>>> disconnect bug. gs_usb appears to be clear, as it returns in its
>>>>>>>> default
>>>>>>>> case. Unfortunately mcba_usb is the only device I have to test
>>>>>>>> with, but
>>>>>>>> those with other devices may want to check for this.
>>>>>>>
>>>>>>> Can you create patches for the affected drivers and send them to the
>>>>>>> list and the maintainers of the driver on Cc?
>>>>>>>
>>>>>>> I don't have access to every USB adapter neither.
>>>>>>>
>>>>>>> Marc
>>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I have seen Martin's emails these days and tried to reproduce the
>>>>>> error here
>>>>>> with an esd CAN-USB/2 device (handled by esd_usb2.c). The only
>>>>>> thing I get
>>>>>> in the log are some messages like this:
>>>>>>
>>>>>> kernel: [14776.152459] usb 2-1: Rx URB aborted (-71)
>>>>>>
>>>>>> There is no endless loop. How could I reproduce the bad behavior?
>>>>>> For the
>>>>>> quick test I used an Ubuntu 4.4.0-101-generic kernel.
>>>>
>>>> Hi,
>>>>
>>>> I got same result for kvaser_usb, a single "Rx URB aborted (-71)".
>>>> When tested on Ubuntu with 4.4.0-93-generic.
>>>>
>>>>> Interesting. Do you see just a few -71 RX URB aborted messages (one
>>>>> per outstanding URB) rather than an endless loop? If so, then I
>>>>> think everything is OK on that device, as the URBs are not being
>>>>> resubmitted.
>>>>>
>>>>> In case it helps, my test case for the mcba_usb is on a Raspberry
>>>>> Pi 3. I don't know whether or not that could influence the USB
>>>>> error code we see, since you are seeing EPROTO instead of EPIPE
>>>>> when the device gets unplugged.
>>>>
>>>> However, I do get lots of (500+) "Rx URB aborted (-71)" printouts,
>>>> on Raspberry Pi 3, running Raspbian with kernel 4.9.35-v7. But no
>>>> EPIPE seen.
>>>>
>>>
>>> Very interesting; this means that there are multiple possible error
>>> codes from a USB disconnect. If that's the case, is it possible that
>>> the best thing to do is what gs_usb does? In gs_usb's receive
>>> callback, we have:
>>>
>>> switch (urb->status) {
>>> case 0: /* success */
>>> break;
>>> case -ENOENT:
>>> case -ESHUTDOWN:
>>> return;
>>> default:
>>> /* do not resubmit aborted urbs. eg: when device goes down */
>>> return;
>>> }
>>>
>>> The default case is to *not* resubmit URBs, rather than to resubmit
>>> is a default and try to catch all possible error codes in the
>>> non-default case, as the other drivers are doing.
>>>
>>> If we don't do something like this, then we need to understand all
>>> the possible error codes that could occur and catch them all.
>>
>> Well, gs_usb never resubmit any URBs if urb->status is non-zero. As
>> long as any error (urb->status != 0) is the result of a disconnect, it
>> should be safe to never resubmit? I doubt this is the case, but I
>> really don't know.
>>
>
> Yeah, it's unclear to me whether what gs_usb does is safe. I have kept
> my patches just using -EPIPE/-EPROTO rather than a catch-all default,
> just to be safe.
Sounds good :)
>>>> Regards,
>>>> Jimmy
>>>>
>>>>>>
>>>>>> In any case I will add the patch handling EPIPE on a test system
>>>>>> and have a
>>>>>> look what it might change.
>>>>>>
>>>>>> Regards,
>>>>>> Stefan Mätje
^ permalink raw reply
* Re: [PATCH 0/4] URB disconnect bug fixes
From: Marc Kleine-Budde @ 2017-12-06 9:20 UTC (permalink / raw)
To: Martin Kelly, linux-can
Cc: Wolfgang Grandegger, Stefan Mätje, Jimmy Assarsson
In-Reply-To: <20171205191550.703-1-mkelly@xevo.com>
[-- Attachment #1.1: Type: text/plain, Size: 1166 bytes --]
On 12/05/2017 08:15 PM, Martin Kelly wrote:
> This patch series is to fix a common bug I found in mcba_usb that appears to
> also exist in these drivers. Because I don't own the hardware, I cannot test the
> changes, but I wanted to send the change so that others could do so.
>
> The bug is that, when you disconnect the device, you get -EPIPE and -EPROTO
> error codes (exactly which depends on the timing and the system) in the URB read
> callback. The driver then resubmits the URB, which immediately fails because the
> device is unplugged. On a slower system like a Raspberry Pi, this can cause a
> kernel stall, while on a desktop, the disconnect code runs fast enough to
> prevent a stall and it's easy not to notice the issue.
>
> Fix this by not resubmitting an URB that receives an -EPIPE/-EPROTO code.
Applied this series and the mcba patch.
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: 488 bytes --]
^ permalink raw reply
* Re: CAN-USB adapter unplug
From: Martin Kelly @ 2017-12-05 23:42 UTC (permalink / raw)
To: Jimmy Assarsson, Marc Kleine-Budde, linux-can@vger.kernel.org
Cc: Stefan Mätje, Remigiusz Kołłątaj,
Wolfgang Grandegger
In-Reply-To: <29dbc892-e468-e828-3f5b-78153930d540@kvaser.com>
On 12/04/2017 12:39 AM, Jimmy Assarsson wrote:
> On 2017-11-30 19:19, Martin Kelly wrote:
>> On 11/30/2017 02:18 AM, Jimmy Assarsson wrote:
>>> On 2017-11-29 18:07, Martin Kelly wrote:
>>>> On 11/29/2017 07:21 AM, Stefan Mätje wrote:
>>>>> Am 29.11.2017 um 13:20 schrieb Marc Kleine-Budde:
>>>>>> On 11/28/2017 10:09 PM, Martin Kelly wrote:
>>>>>>>> Both applied to can.
>>>>>>>
>>>>>>> Thanks! By the way as far as I can tell from code inspection, it
>>>>>>> appears
>>>>>>> that most of the other drivers in net/can/usb should have the same
>>>>>>> disconnect bug. gs_usb appears to be clear, as it returns in its
>>>>>>> default
>>>>>>> case. Unfortunately mcba_usb is the only device I have to test
>>>>>>> with, but
>>>>>>> those with other devices may want to check for this.
>>>>>>
>>>>>> Can you create patches for the affected drivers and send them to the
>>>>>> list and the maintainers of the driver on Cc?
>>>>>>
>>>>>> I don't have access to every USB adapter neither.
>>>>>>
>>>>>> Marc
>>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> I have seen Martin's emails these days and tried to reproduce the
>>>>> error here
>>>>> with an esd CAN-USB/2 device (handled by esd_usb2.c). The only
>>>>> thing I get
>>>>> in the log are some messages like this:
>>>>>
>>>>> kernel: [14776.152459] usb 2-1: Rx URB aborted (-71)
>>>>>
>>>>> There is no endless loop. How could I reproduce the bad behavior?
>>>>> For the
>>>>> quick test I used an Ubuntu 4.4.0-101-generic kernel.
>>>
>>> Hi,
>>>
>>> I got same result for kvaser_usb, a single "Rx URB aborted (-71)".
>>> When tested on Ubuntu with 4.4.0-93-generic.
>>>
>>>> Interesting. Do you see just a few -71 RX URB aborted messages (one
>>>> per outstanding URB) rather than an endless loop? If so, then I
>>>> think everything is OK on that device, as the URBs are not being
>>>> resubmitted.
>>>>
>>>> In case it helps, my test case for the mcba_usb is on a Raspberry Pi
>>>> 3. I don't know whether or not that could influence the USB error
>>>> code we see, since you are seeing EPROTO instead of EPIPE when the
>>>> device gets unplugged.
>>>
>>> However, I do get lots of (500+) "Rx URB aborted (-71)" printouts, on
>>> Raspberry Pi 3, running Raspbian with kernel 4.9.35-v7. But no EPIPE
>>> seen.
>>>
>>
>> Very interesting; this means that there are multiple possible error
>> codes from a USB disconnect. If that's the case, is it possible that
>> the best thing to do is what gs_usb does? In gs_usb's receive
>> callback, we have:
>>
>> switch (urb->status) {
>> case 0: /* success */
>> break;
>> case -ENOENT:
>> case -ESHUTDOWN:
>> return;
>> default:
>> /* do not resubmit aborted urbs. eg: when device goes down */
>> return;
>> }
>>
>> The default case is to *not* resubmit URBs, rather than to resubmit is
>> a default and try to catch all possible error codes in the non-default
>> case, as the other drivers are doing.
>>
>> If we don't do something like this, then we need to understand all the
>> possible error codes that could occur and catch them all.
>
> Well, gs_usb never resubmit any URBs if urb->status is non-zero. As long
> as any error (urb->status != 0) is the result of a disconnect, it should
> be safe to never resubmit? I doubt this is the case, but I really don't
> know.
>
Yeah, it's unclear to me whether what gs_usb does is safe. I have kept
my patches just using -EPIPE/-EPROTO rather than a catch-all default,
just to be safe.
>>> Regards,
>>> Jimmy
>>>
>>>>>
>>>>> In any case I will add the patch handling EPIPE on a test system
>>>>> and have a
>>>>> look what it might change.
>>>>>
>>>>> Regards,
>>>>> Stefan Mätje
^ permalink raw reply
* [PATCH 4/4] can: usb_8dev: cancel urb on -EPIPE and -EPROTO
From: Martin Kelly @ 2017-12-05 19:15 UTC (permalink / raw)
To: linux-can
Cc: Marc Kleine-Budde, Wolfgang Grandegger, Stefan Mätje,
Jimmy Assarsson, Martin Kelly
In-Reply-To: <20171205191550.703-1-mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
drivers/net/can/usb/usb_8dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c
index d000cb62d6ae..27861c417c94 100644
--- a/drivers/net/can/usb/usb_8dev.c
+++ b/drivers/net/can/usb/usb_8dev.c
@@ -524,6 +524,8 @@ static void usb_8dev_read_bulk_callback(struct urb *urb)
break;
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.11.0
^ permalink raw reply related
* [PATCH 3/4] can: kvaser_usb: cancel urb on -EPIPE and -EPROTO
From: Martin Kelly @ 2017-12-05 19:15 UTC (permalink / raw)
To: linux-can
Cc: Marc Kleine-Budde, Wolfgang Grandegger, Stefan Mätje,
Jimmy Assarsson, Martin Kelly
In-Reply-To: <20171205191550.703-1-mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
drivers/net/can/usb/kvaser_usb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index f95945915d20..63587b8e6825 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1326,6 +1326,8 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
case 0:
break;
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
default:
--
2.11.0
^ permalink raw reply related
* [PATCH 2/4] can: esd_usb2: cancel urb on -EPIPE and -EPROTO
From: Martin Kelly @ 2017-12-05 19:15 UTC (permalink / raw)
To: linux-can
Cc: Marc Kleine-Budde, Wolfgang Grandegger, Stefan Mätje,
Jimmy Assarsson, Martin Kelly
In-Reply-To: <20171205191550.703-1-mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
drivers/net/can/usb/esd_usb2.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
index 9fdb0f0bfa06..c6dcf93675c0 100644
--- a/drivers/net/can/usb/esd_usb2.c
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -393,6 +393,8 @@ static void esd_usb2_read_bulk_callback(struct urb *urb)
break;
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.11.0
^ permalink raw reply related
* [PATCH 1/4] can: ems_usb: cancel urb on -EPIPE and -EPROTO
From: Martin Kelly @ 2017-12-05 19:15 UTC (permalink / raw)
To: linux-can
Cc: Marc Kleine-Budde, Wolfgang Grandegger, Stefan Mätje,
Jimmy Assarsson, Martin Kelly
In-Reply-To: <20171205191550.703-1-mkelly@xevo.com>
In mcba_usb, we have observed that when you unplug the device, the driver will
endlessly resubmit failing URBs, which can cause CPU stalls. This issue
is fixed in mcba_usb by catching the codes seen on device disconnect
(-EPIPE and -EPROTO).
This driver also resubmits in the case of -EPIPE and -EPROTO, so fix it
in the same way.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
drivers/net/can/usb/ems_usb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index b3d02759c226..b00358297424 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -288,6 +288,8 @@ static void ems_usb_read_interrupt_callback(struct urb *urb)
case -ECONNRESET: /* unlink */
case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.11.0
^ permalink raw reply related
* [PATCH 0/4] URB disconnect bug fixes
From: Martin Kelly @ 2017-12-05 19:15 UTC (permalink / raw)
To: linux-can
Cc: Marc Kleine-Budde, Wolfgang Grandegger, Stefan Mätje,
Jimmy Assarsson, Martin Kelly
This patch series is to fix a common bug I found in mcba_usb that appears to
also exist in these drivers. Because I don't own the hardware, I cannot test the
changes, but I wanted to send the change so that others could do so.
The bug is that, when you disconnect the device, you get -EPIPE and -EPROTO
error codes (exactly which depends on the timing and the system) in the URB read
callback. The driver then resubmits the URB, which immediately fails because the
device is unplugged. On a slower system like a Raspberry Pi, this can cause a
kernel stall, while on a desktop, the disconnect code runs fast enough to
prevent a stall and it's easy not to notice the issue.
Fix this by not resubmitting an URB that receives an -EPIPE/-EPROTO code.
Martin Kelly (4):
can: ems_usb: cancel urb on -EPIPE and -EPROTO
can: esd_usb2: cancel urb on -EPIPE and -EPROTO
can: kvaser_usb: cancel urb on -EPIPE and -EPROTO
can: usb_8dev: cancel urb on -EPIPE and -EPROTO
drivers/net/can/usb/ems_usb.c | 2 ++
drivers/net/can/usb/esd_usb2.c | 2 ++
drivers/net/can/usb/kvaser_usb.c | 2 ++
drivers/net/can/usb/usb_8dev.c | 2 ++
4 files changed, 8 insertions(+)
--
2.11.0
^ permalink raw reply
* [PATCH] can: mcba_usb: cancel urb on -EPROTO
From: Martin Kelly @ 2017-12-05 18:34 UTC (permalink / raw)
To: linux-can
Cc: Remigiusz Kołłątaj, Marc Kleine-Budde,
Wolfgang Grandegger, Martin Kelly
When we unplug the device, we can see both -EPIPE and -EPROTO depending
on exact timing and what system we run on. If we continue to resubmit
URBs, they will immediately fail, and they can cause stalls, especially
on slower CPUs.
Fix this by not resubmitting on -EPROTO, as we already do on -EPIPE.
Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
drivers/net/can/usb/mcba_usb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index ef417dcddbf7..8d8c2086424d 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -593,6 +593,7 @@ static void mcba_usb_read_bulk_callback(struct urb *urb)
case -ENOENT:
case -EPIPE:
+ case -EPROTO:
case -ESHUTDOWN:
return;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/2] dt-binding: can: mcp2517fd: document device tree bindings
From: Martin Sperl @ 2017-12-05 10:26 UTC (permalink / raw)
To: Patrick Menschel; +Cc: linux-can-u79uwXL29TY76Z2rM5mHXA, devicetree
In-Reply-To: <1ac487f7-17e3-c8a0-0f99-8138fe867373-1KBjaw7Xf1+zQB+pC5nmwQ@public.gmane.org>
Hi Patrick!
I had a quick look starting to implement gpiolib,
but I believe I would need to implement pin-ctrl on top to
make the Alternate functions work propperly.
I wonder if this effort is really worth it.
Martin
On 11/30/2017 06:49 PM, Patrick Menschel wrote:
> Am 30.11.2017 um 17:58 schriebkernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org:
>>> drivers/tty/serial/max310x.c
>>> Documentation/devicetree/bindings/serial/maxim,max310x.txt
>>> Look for "#ifdef CONFIG_GPIOLIB”.
>> This is a gpio-controller, for which this is what I would implement.
>>
> Hi,
>
> you are mistaken about that, max310x chips are spi to uart bridges with
> strong ties to rs485 which is very similar to CAN except for the missing
> arbitration in hardware. Max14830 has GPIOs with clocking support, so
> this chip's driver also fused the main function with secondary gpio
> functions.
> https://datasheets.maximintegrated.com/en/ds/MAX14830.pdf
>
> Anyway I'm curious what is best practice for this sort of configuration.
>
> Regards,
> Patrick
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: pull-request: can-next 2017-12-01,Re: pull-request: can-next 2017-12-01
From: David Miller @ 2017-12-04 18:20 UTC (permalink / raw)
To: mkl; +Cc: netdev, kernel, linux-can
In-Reply-To: <85177a44-2bf7-80f8-08c4-df133cd48917@pengutronix.de>
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Fri, 1 Dec 2017 14:50:04 +0100
> On 12/01/2017 02:37 PM, Marc Kleine-Budde wrote:
>> Doh, I had an error in my script which produced the wrong Linux version
>> in the tag. The correct name of the tag is:
>
> First test - than post - here's the correct tag:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git
> tags/linux-can-next-for-4.16-20171201
Pulled, thanks!
^ permalink raw reply
* Re: CAN-USB adapter unplug
From: Jimmy Assarsson @ 2017-12-04 8:39 UTC (permalink / raw)
To: Martin Kelly, Marc Kleine-Budde, linux-can@vger.kernel.org
Cc: Stefan Mätje, Remigiusz Kołłątaj,
Wolfgang Grandegger
In-Reply-To: <4f6687db-269d-d8ff-7a47-5094ad63043a@xevo.com>
On 2017-11-30 19:19, Martin Kelly wrote:
> On 11/30/2017 02:18 AM, Jimmy Assarsson wrote:
>> On 2017-11-29 18:07, Martin Kelly wrote:
>>> On 11/29/2017 07:21 AM, Stefan Mätje wrote:
>>>> Am 29.11.2017 um 13:20 schrieb Marc Kleine-Budde:
>>>>> On 11/28/2017 10:09 PM, Martin Kelly wrote:
>>>>>>> Both applied to can.
>>>>>>
>>>>>> Thanks! By the way as far as I can tell from code inspection, it
>>>>>> appears
>>>>>> that most of the other drivers in net/can/usb should have the same
>>>>>> disconnect bug. gs_usb appears to be clear, as it returns in its
>>>>>> default
>>>>>> case. Unfortunately mcba_usb is the only device I have to test
>>>>>> with, but
>>>>>> those with other devices may want to check for this.
>>>>>
>>>>> Can you create patches for the affected drivers and send them to the
>>>>> list and the maintainers of the driver on Cc?
>>>>>
>>>>> I don't have access to every USB adapter neither.
>>>>>
>>>>> Marc
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> I have seen Martin's emails these days and tried to reproduce the
>>>> error here
>>>> with an esd CAN-USB/2 device (handled by esd_usb2.c). The only thing
>>>> I get
>>>> in the log are some messages like this:
>>>>
>>>> kernel: [14776.152459] usb 2-1: Rx URB aborted (-71)
>>>>
>>>> There is no endless loop. How could I reproduce the bad behavior?
>>>> For the
>>>> quick test I used an Ubuntu 4.4.0-101-generic kernel.
>>
>> Hi,
>>
>> I got same result for kvaser_usb, a single "Rx URB aborted (-71)".
>> When tested on Ubuntu with 4.4.0-93-generic.
>>
>>> Interesting. Do you see just a few -71 RX URB aborted messages (one
>>> per outstanding URB) rather than an endless loop? If so, then I think
>>> everything is OK on that device, as the URBs are not being resubmitted.
>>>
>>> In case it helps, my test case for the mcba_usb is on a Raspberry Pi
>>> 3. I don't know whether or not that could influence the USB error
>>> code we see, since you are seeing EPROTO instead of EPIPE when the
>>> device gets unplugged.
>>
>> However, I do get lots of (500+) "Rx URB aborted (-71)" printouts, on
>> Raspberry Pi 3, running Raspbian with kernel 4.9.35-v7. But no EPIPE
>> seen.
>>
>
> Very interesting; this means that there are multiple possible error
> codes from a USB disconnect. If that's the case, is it possible that the
> best thing to do is what gs_usb does? In gs_usb's receive callback, we
> have:
>
> switch (urb->status) {
> case 0: /* success */
> break;
> case -ENOENT:
> case -ESHUTDOWN:
> return;
> default:
> /* do not resubmit aborted urbs. eg: when device goes down */
> return;
> }
>
> The default case is to *not* resubmit URBs, rather than to resubmit is a
> default and try to catch all possible error codes in the non-default
> case, as the other drivers are doing.
>
> If we don't do something like this, then we need to understand all the
> possible error codes that could occur and catch them all.
Well, gs_usb never resubmit any URBs if urb->status is non-zero. As long
as any error (urb->status != 0) is the result of a disconnect, it should
be safe to never resubmit? I doubt this is the case, but I really don't
know.
>> Regards,
>> Jimmy
>>
>>>>
>>>> In any case I will add the patch handling EPIPE on a test system and
>>>> have a
>>>> look what it might change.
>>>>
>>>> Regards,
>>>> Stefan Mätje
^ permalink raw reply
* Re: [PATCH 2/2] can: mcp2517fd: Add Microchip mcp2517fd CAN FD driver
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2017-12-03 18:34 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Wolfgang Grandegger, Rob Herring, Mark Rutland,
linux-can-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <ff920dd7-4535-dcaa-27f9-57844ce66c7b-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
H Marc!
> On 30.11.2017, at 14:04, Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>
> On 11/24/2017 07:35 PM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
>> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>
>> This patch adds support for the Microchip mcp2517fd CAN-FD controller.
>> The mcp2517fd is capable of transmitting and receiving standard data
>> frames, extended data frames, remote frames and Can-FD frames.
>> The mcp2517fd interfaces with the host over SPI.
>
> I've review about the last 1/3 of the driver. See comments inline.
Thanks for all the feedback!
I shall incorporate those into V2.
>> + switch (priv->config.gpio0_mode) {
>> + case gpio_mode_standby:
>> + case gpio_mode_int: /* asserted low on TXIF */
>> + case gpio_mode_out_low:
>> + case gpio_mode_out_high:
>> + case gpio_mode_in:
>
> Please add comments for fallthrough for all your switch-case.
I thought /* fall through */ is only needed for the cases
where there is code involved in each case block and you want
to fall through to the next block - checkpatch only complains
in such conditions (there is one location in the driver -
in mcp2517fd_can_ist_handle_status - that requires /* fall through */).
Also Documentation/process/coding-style.rst shows such an example
in the “indentation” section, so I would assume this is valid code.
>>
>> +int mcp2517fd_of_parse(struct mcp2517fd_priv *priv)
>> +{
>> +#ifdef CONFIG_OF_DYNAMIC
>
> Why does this code depend on OF_DYNAMIC?
>
Well - this is only in case of Device Tree - no need to include
all the DT-parsing in case that there is no DT support in the
first place…
I may arrange it as a #ifdef outside of the function
(like the other case you have mentioned).
>>
>> + if (!IS_ERR(clk)) {
>> + ret = clk_prepare_enable(clk);
>> + if (ret)
>> + goto out_free;
>> + }
>
> Why do you keep the clock running after the device has been probed?
> Usually we enable the clock during open().
I took the mcp251x as a blue-print and I believe it follows the
same pattern…
But I may have simplified things during early driver development,
so I will recheck it.
>
>> +
>> + /* check in device tree for overrrides */
>> + ret = mcp2517fd_of_parse(priv);
>> + if (ret)
>> + return ret;
>
> You're keeping the clock running.
See above - I shall look into it...
>> + dev_err(&spi->dev,
>> + "PLL clock frequency %i would exceed limit\n",
>> + priv->can.clock.freq
>> + );
>> + return -EINVAL;
>
> same here
Thanks, Martin--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: pull-request: can 2017-12-01
From: David Miller @ 2017-12-03 14:58 UTC (permalink / raw)
To: mkl; +Cc: netdev, linux-can, kernel
In-Reply-To: <20171201141739.5816-1-mkl@pengutronix.de>
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Fri, 1 Dec 2017 15:17:30 +0100
> this is a pull for net consisting of nine patches.
>
> The first three patches are by Jimmy Assarsson for the kvaser_usb driver
> and add the missing free()s in some error path, a signed/unsigned
> comparison and ratelimit the error messages in case of incomplete
> messages. Oliver Stäbler's patch for the ti_hecc driver fix the napi
> poll function's return value. The return values of the probe function of
> the peak_canfd and peak_pci PCI drivers are fixed by Stephane Grosjean's
> patch. Two patches by me for the flexcan driver update the
> bugs/features/quirks overview table and fix the error state transition
> for the VF610 SoC. The two patches by Martin Kelly for the mcba_usb
> driver fix a typo and a device disconnect bug.
Pulled, thanks Marc.
^ permalink raw reply
* [PATCH] vxcan: improve handling of missing peer name attribute
From: Oliver Hartkopp @ 2017-12-02 17:48 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Serhey Popovych
Picking up the patch from Serhey Popovych (commit 191cdb3822e5df6b3c8,
"veth: Be more robust on network device creation when no attributes").
When the peer name attribute is not provided the former implementation tries
to register the given device name twice ... which leads to -EEXIST.
If only one device name is given apply an automatic generated and valid name
for the peer.
CC: Serhey Popovych <serhe.popovych@gmail.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/vxcan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8404e8852a0f..b4c4a2c76437 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -194,7 +194,7 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
tbp = peer_tb;
}
- if (tbp[IFLA_IFNAME]) {
+ if (ifmp && tbp[IFLA_IFNAME]) {
nla_strlcpy(ifname, tbp[IFLA_IFNAME], IFNAMSIZ);
name_assign_type = NET_NAME_USER;
} else {
--
2.15.0
^ permalink raw reply related
* (unknown),
From: Rein Appeldoorn @ 2017-12-01 14:22 UTC (permalink / raw)
To: linux-can
unsubscribe linux-can
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox