| Message ID | 20220614122821.3646071-6-dario.binacchi@amarulasolutions.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: slcan: extend supported features | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject, async |
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: 39bbbe2356a2a13ec26a8765d8a4e6dfe7ce33e9 ("[PATCH v4 05/12] can: slcan: use CAN network device driver API")
url: https://github.com/intel-lab-lkp/linux/commits/Dario-Binacchi/can-slcan-extend-supported-features/20220614-203636
patch link: https://lore.kernel.org/lkml/20220614122821.3646071-6-dario.binacchi@amarulasolutions.com
in testcase: ltp
version: ltp-x86_64-14c1f76-1_20220625
with following parameters:
test: pty
ucode: 0x21
test-description: The LTP testsuite contains a collection of tools for testing the Linux kernel and related features.
test-url: http://linux-test-project.github.io/
on test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz with 8G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <oliver.sang@intel.com>
[ 164.290583][ T3878] BUG: sleeping function called from invalid context at mm/page_alloc.c:5203
[ 164.290589][ T3878] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 3878, name: pty03
[ 164.290592][ T3878] preempt_count: 1, expected: 0
[ 164.290595][ T3878] CPU: 1 PID: 3878 Comm: pty03 Tainted: G S 5.19.0-rc2-00005-g39bbbe2356a2 #1
[ 164.290600][ T3878] Hardware name: Hewlett-Packard p6-1451cx/2ADA, BIOS 8.15 02/05/2013
[ 164.290602][ T3878] Call Trace:
[ 164.290604][ T3878] <TASK>
[ 164.290606][ T3878] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1))
[ 164.290615][ T3878] __might_resched.cold (kernel/sched/core.c:9792)
[ 164.290621][ T3878] prepare_alloc_pages+0x330/0x500
[ 164.290628][ T3878] ? tty_set_ldisc (drivers/tty/tty_ldisc.c:555)
[ 164.290634][ T3878] __alloc_pages (mm/page_alloc.c:5415)
[ 164.290637][ T3878] ? __alloc_pages_slowpath+0x15c0/0x15c0
[ 164.290641][ T3878] ? tty_set_ldisc (drivers/tty/tty_ldisc.c:555)
[ 164.290645][ T3878] ? kasan_save_stack (mm/kasan/common.c:40)
[ 164.290650][ T3878] ? kasan_set_track (mm/kasan/common.c:45)
[ 164.290654][ T3878] ? __kasan_slab_free (mm/kasan/common.c:368 mm/kasan/common.c:328 mm/kasan/common.c:374)
[ 164.290658][ T3878] ? entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
[ 164.290663][ T3878] ? do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 164.290668][ T3878] ? entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
[ 164.290672][ T3878] kmalloc_large_node (mm/slub.c:4432)
[ 164.290677][ T3878] __kmalloc_node (include/asm-generic/getorder.h:41 mm/slub.c:4450)
[ 164.290681][ T3878] kvmalloc_node (mm/util.c:619)
[ 164.290687][ T3878] alloc_netdev_mqs (net/core/dev.c:10577)
[ 164.290694][ T3878] ? can_get_state_str (drivers/net/can/dev/dev.c:222) can_dev
[ 164.290704][ T3878] ? _raw_spin_lock (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:134 kernel/locking/spinlock.c:154)
[ 164.290708][ T3878] alloc_candev_mqs (drivers/net/can/dev/dev.c:262) can_dev
[ 164.290717][ T3878] slcan_open (drivers/net/can/slcan.c:531 drivers/net/can/slcan.c:584) slcan
[ 164.290723][ T3878] tty_ldisc_open (drivers/tty/tty_ldisc.c:433)
[ 164.290728][ T3878] tty_set_ldisc (drivers/tty/tty_ldisc.c:558)
[ 164.290733][ T3878] tty_ioctl (drivers/tty/tty_io.c:2714)
[ 164.290736][ T3878] ? do_sys_openat2 (fs/open.c:1288)
[ 164.290740][ T3878] ? tty_release (drivers/tty/tty_io.c:2655)
[ 164.290744][ T3878] ? do_sys_openat2 (fs/open.c:1288)
[ 164.290747][ T3878] ? build_open_flags (fs/open.c:1264)
[ 164.290751][ T3878] ? __ia32_sys_stat (fs/stat.c:396)
[ 164.290755][ T3878] ? userns_owner (kernel/user_namespace.c:371)
[ 164.290760][ T3878] ? __fget_files (arch/x86/include/asm/atomic64_64.h:22 include/linux/atomic/atomic-arch-fallback.h:2363 include/linux/atomic/atomic-arch-fallback.h:2388 include/linux/atomic/atomic-arch-fallback.h:2404 include/linux/atomic/atomic-long.h:497 include/linux/atomic/atomic-instrumented.h:1854 fs/file.c:882 fs/file.c:913)
[ 164.290764][ T3878] __x64_sys_ioctl (fs/ioctl.c:52 fs/ioctl.c:870 fs/ioctl.c:856 fs/ioctl.c:856)
[ 164.290768][ T3878] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 164.290773][ T3878] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
[ 164.290777][ T3878] RIP: 0033:0x7f86f3323cc7
[ 164.290781][ T3878] Code: 00 00 00 48 8b 05 c9 91 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 99 91 0c 00 f7 d8 64 89 01 48
All code
========
0: 00 00 add %al,(%rax)
2: 00 48 8b add %cl,-0x75(%rax)
5: 05 c9 91 0c 00 add $0xc91c9,%eax
a: 64 c7 00 26 00 00 00 movl $0x26,%fs:(%rax)
11: 48 c7 c0 ff ff ff ff mov $0xffffffffffffffff,%rax
18: c3 retq
19: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
20: 00 00 00
23: b8 10 00 00 00 mov $0x10,%eax
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d 99 91 0c 00 mov 0xc9199(%rip),%rcx # 0xc91d3
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d 99 91 0c 00 mov 0xc9199(%rip),%rcx # 0xc91a9
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
[ 164.290784][ T3878] RSP: 002b:00007fff6a8fb8b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 164.290789][ T3878] RAX: ffffffffffffffda RBX: 00007f86f322c6c0 RCX: 00007f86f3323cc7
[ 164.290792][ T3878] RDX: 000055911985c700 RSI: 0000000000005423 RDI: 000000000000000d
[ 164.290795][ T3878] RBP: 000055911985c700 R08: 0000000000000000 R09: cccccccccccccccd
[ 164.290797][ T3878] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000d
[ 164.290799][ T3878] R13: 0000000000000006 R14: 000055911985c6a0 R15: 0000000000000004
[ 164.290803][ T3878] </TASK>
[ 164.738226][ T3878] BUG: scheduling while atomic: pty03/3878/0x00000002
[ 164.738233][ T3878] Modules linked in: slcan can_dev n_hdlc netconsole btrfs blake2b_generic xor raid6_pq zstd_compress libcrc32c sd_mod t10_pi crc64_rocksoft_generic crc64_rocksoft crc64 sg i915 intel_rapl_msr intel_gtt intel_rapl_common drm_buddy x86_pkg_temp_thermal drm_display_helper intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul crc32c_intel ahci ttm ghash_clmulni_intel rapl libahci intel_cstate ipmi_devintf drm_kms_helper intel_uncore ipmi_msghandler usb_storage mei_me syscopyarea sysfillrect libata sysimgblt mei fb_sys_fops video drm fuse ip_tables
[ 164.738298][ T3878] CPU: 3 PID: 3878 Comm: pty03 Tainted: G S W 5.19.0-rc2-00005-g39bbbe2356a2 #1
[ 164.738302][ T3878] Hardware name: Hewlett-Packard p6-1451cx/2ADA, BIOS 8.15 02/05/2013
[ 164.738305][ T3878] Call Trace:
[ 164.738307][ T3878] <TASK>
[ 164.738309][ T3878] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1))
[ 164.738318][ T3878] __schedule_bug.cold (kernel/sched/core.c:5661)
[ 164.738323][ T3878] schedule_debug (arch/x86/include/asm/preempt.h:35 kernel/sched/core.c:5688)
[ 164.738329][ T3878] __schedule (arch/x86/include/asm/jump_label.h:27 include/linux/jump_label.h:207 kernel/sched/features.h:40 kernel/sched/core.c:6324)
[ 164.738335][ T3878] ? node_tag_clear (arch/x86/include/asm/bitops.h:94 include/asm-generic/bitops/instrumented-non-atomic.h:43 lib/radix-tree.c:107 lib/radix-tree.c:1000)
[ 164.738340][ T3878] ? io_schedule_timeout (kernel/sched/core.c:6310)
[ 164.738344][ T3878] ? idr_alloc_u32 (lib/idr.c:55)
[ 164.738349][ T3878] ? _raw_spin_lock_irq (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:120 kernel/locking/spinlock.c:170)
[ 164.738353][ T3878] schedule (include/linux/instrumented.h:71 (discriminator 1) include/asm-generic/bitops/instrumented-non-atomic.h:134 (discriminator 1) include/linux/thread_info.h:118 (discriminator 1) include/linux/sched.h:2196 (discriminator 1) kernel/sched/core.c:6502 (discriminator 1))
[ 164.738358][ T3878] rwsem_down_write_slowpath (arch/x86/include/asm/current.h:15 kernel/locking/rwsem.c:1174)
[ 164.738362][ T3878] ? __down_timeout (kernel/locking/rwsem.c:1099)
[ 164.738365][ T3878] ? _raw_write_lock_irq (kernel/locking/spinlock.c:153)
[ 164.738370][ T3878] ? idr_get_free (lib/radix-tree.c:1533)
[ 164.738373][ T3878] ? kernfs_dir_fop_release (fs/kernfs/dir.c:584)
[ 164.738379][ T3878] down_write (kernel/locking/rwsem.c:1544)
[ 164.738383][ T3878] ? down_write_killable (kernel/locking/rwsem.c:1540)
[ 164.738386][ T3878] ? idr_alloc (lib/idr.c:118)
[ 164.738389][ T3878] ? _raw_spin_lock (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:134 kernel/locking/spinlock.c:154)
[ 164.738393][ T3878] ? __fprop_add_percpu_max (lib/idr.c:35)
[ 164.738397][ T3878] kernfs_add_one (include/linux/kernfs.h:332 fs/kernfs/dir.c:737)
[ 164.738402][ T3878] ? kernfs_new_node (fs/kernfs/dir.c:659)
[ 164.738406][ T3878] ? device_remove_bin_file (drivers/base/core.c:2089)
[ 164.738411][ T3878] __kernfs_create_file (fs/kernfs/file.c:1016)
[ 164.738415][ T3878] sysfs_add_file_mode_ns (fs/sysfs/file.c:301)
[ 164.738419][ T3878] ? projid_m_show (kernel/user_namespace.c:308)
[ 164.738423][ T3878] ? _raw_write_lock_irq (kernel/locking/spinlock.c:153)
[ 164.738427][ T3878] create_files (fs/sysfs/group.c:66 (discriminator 3))
[ 164.738431][ T3878] ? make_kgid (kernel/user_namespace.c:475)
[ 164.738434][ T3878] internal_create_group (fs/sysfs/group.c:148)
[ 164.738438][ T3878] ? down_write (arch/x86/include/asm/atomic64_64.h:34 include/linux/atomic/atomic-long.h:41 include/linux/atomic/atomic-instrumented.h:1280 kernel/locking/rwsem.c:139 kernel/locking/rwsem.c:256 kernel/locking/rwsem.c:1286 kernel/locking/rwsem.c:1296 kernel/locking/rwsem.c:1543)
[ 164.738441][ T3878] ? create_files (fs/sysfs/group.c:109)
[ 164.738444][ T3878] ? __cond_resched (kernel/sched/core.c:8217)
[ 164.738448][ T3878] ? down_write (arch/x86/include/asm/atomic64_64.h:34 include/linux/atomic/atomic-long.h:41 include/linux/atomic/atomic-instrumented.h:1280 kernel/locking/rwsem.c:139 kernel/locking/rwsem.c:256 kernel/locking/rwsem.c:1286 kernel/locking/rwsem.c:1296 kernel/locking/rwsem.c:1543)
[ 164.738452][ T3878] internal_create_groups+0x7c/0x140
[ 164.738456][ T3878] device_add_attrs (drivers/base/core.c:2621)
[ 164.738460][ T3878] ? klist_children_put (drivers/base/core.c:2614)
[ 164.738464][ T3878] ? kernfs_create_link (fs/kernfs/symlink.c:48)
[ 164.738467][ T3878] ? kernfs_put (arch/x86/include/asm/atomic.h:123 (discriminator 1) include/linux/atomic/atomic-instrumented.h:576 (discriminator 1) fs/kernfs/dir.c:521 (discriminator 1))
[ 164.738471][ T3878] device_add (drivers/base/core.c:3368)
[ 164.738476][ T3878] ? device_initialize (drivers/base/core.c:3200)
[ 164.738479][ T3878] ? __fw_devlink_link_to_suppliers (drivers/base/core.c:3299)
[ 164.738484][ T3878] ? __hrtimer_init (kernel/time/hrtimer.c:1559)
[ 164.738488][ T3878] netdev_register_kobject (net/core/net-sysfs.c:2014)
[ 164.738493][ T3878] register_netdevice (net/core/dev.c:10047)
[ 164.738498][ T3878] ? __cond_resched (kernel/sched/core.c:8217)
[ 164.738502][ T3878] ? netdev_change_features (net/core/dev.c:9942)
[ 164.738507][ T3878] register_netdev (net/core/dev.c:10171)
[ 164.738511][ T3878] register_candev (drivers/net/can/dev/dev.c:460) can_dev
[ 164.738522][ T3878] ? can_set_termination (drivers/net/can/dev/dev.c:460) can_dev
[ 164.738529][ T3878] ? _raw_spin_lock (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:111 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:134 kernel/locking/spinlock.c:154)
[ 164.738533][ T3878] ? alloc_candev_mqs (drivers/net/can/dev/dev.c:284) can_dev
[ 164.738541][ T3878] slcan_open (drivers/net/can/slcan.c:598) slcan
[ 164.738547][ T3878] tty_ldisc_open (drivers/tty/tty_ldisc.c:433)
[ 164.738552][ T3878] tty_set_ldisc (drivers/tty/tty_ldisc.c:558)
[ 164.738557][ T3878] tty_ioctl (drivers/tty/tty_io.c:2714)
[ 164.738561][ T3878] ? tty_release (drivers/tty/tty_io.c:2655)
[ 164.738565][ T3878] ? __switch_to (arch/x86/kernel/process.h:38 arch/x86/kernel/process_64.c:627)
[ 164.738569][ T3878] ? __schedule (kernel/sched/core.c:6310)
[ 164.738574][ T3878] ? io_schedule_timeout (kernel/sched/core.c:6310)
[ 164.738578][ T3878] ? __fget_files (arch/x86/include/asm/atomic64_64.h:22 include/linux/atomic/atomic-arch-fallback.h:2363 include/linux/atomic/atomic-arch-fallback.h:2388 include/linux/atomic/atomic-arch-fallback.h:2404 include/linux/atomic/atomic-long.h:497 include/linux/atomic/atomic-instrumented.h:1854 fs/file.c:882 fs/file.c:913)
[ 164.738583][ T3878] __x64_sys_ioctl (fs/ioctl.c:52 fs/ioctl.c:870 fs/ioctl.c:856 fs/ioctl.c:856)
[ 164.738587][ T3878] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 164.738591][ T3878] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
[ 164.738596][ T3878] RIP: 0033:0x7f86f3323cc7
[ 164.738599][ T3878] Code: 00 00 00 48 8b 05 c9 91 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 99 91 0c 00 f7 d8 64 89 01 48
All code
========
0: 00 00 add %al,(%rax)
2: 00 48 8b add %cl,-0x75(%rax)
5: 05 c9 91 0c 00 add $0xc91c9,%eax
a: 64 c7 00 26 00 00 00 movl $0x26,%fs:(%rax)
11: 48 c7 c0 ff ff ff ff mov $0xffffffffffffffff,%rax
18: c3 retq
19: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
20: 00 00 00
23: b8 10 00 00 00 mov $0x10,%eax
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d 99 91 0c 00 mov 0xc9199(%rip),%rcx # 0xc91d3
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d 99 91 0c 00 mov 0xc9199(%rip),%rcx # 0xc91a9
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
[ 164.738603][ T3878] RSP: 002b:00007fff6a8fb8b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 164.738608][ T3878] RAX: ffffffffffffffda RBX: 00007f86f322c6c0 RCX: 00007f86f3323cc7
[ 164.738611][ T3878] RDX: 000055911985c700 RSI: 0000000000005423 RDI: 000000000000000d
[ 164.738613][ T3878] RBP: 000055911985c700 R08: 0000000000000000 R09: cccccccccccccccd
[ 164.738616][ T3878] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000d
[ 164.738618][ T3878] R13: 0000000000000006 R14: 000055911985c6a0 R15: 0000000000000004
[ 164.738621][ T3878] </TASK>
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
On 14.06.2022 14:28:14, Dario Binacchi wrote: > As suggested by commit [1], now the driver uses the functions and the > data structures provided by the CAN network device driver interface. > > Currently the driver doesn't implement a way to set bitrate for SLCAN > based devices via ip tool, so you'll have to do this by slcand or > slcan_attach invocation through the -sX parameter: > > - slcan_attach -f -s6 -o /dev/ttyACM0 > - slcand -f -s8 -o /dev/ttyUSB0 > > where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to > 1Mbit/s. > See the table below for further CAN bitrates: > - s0 -> 10 Kbit/s > - s1 -> 20 Kbit/s > - s2 -> 50 Kbit/s > - s3 -> 100 Kbit/s > - s4 -> 125 Kbit/s > - s5 -> 250 Kbit/s > - s6 -> 500 Kbit/s > - s7 -> 800 Kbit/s > - s8 -> 1000 Kbit/s > > In doing so, the struct can_priv::bittiming.bitrate of the driver is not > set and since the open_candev() checks that the bitrate has been set, it > must be a non-zero value, the bitrate is set to a fake value (-1U) > before it is called. > > The patch also changes the slcan_devs locking from rtnl to spin_lock. The > change was tested with a kernel with the CONFIG_PROVE_LOCKING option > enabled that did not show any errors. You're not allowed to call alloc_candev() with a spin_lock held. See today's kernel test robot mail: | https://lore.kernel.org/all/YrpqO5jepAvv4zkf@xsang-OptiPlex-9020 I think it's best to keep the rtnl for now. regards, Marc
Hi Marc, On Tue, Jun 28, 2022 at 11:28 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 14.06.2022 14:28:14, Dario Binacchi wrote: > > As suggested by commit [1], now the driver uses the functions and the > > data structures provided by the CAN network device driver interface. > > > > Currently the driver doesn't implement a way to set bitrate for SLCAN > > based devices via ip tool, so you'll have to do this by slcand or > > slcan_attach invocation through the -sX parameter: > > > > - slcan_attach -f -s6 -o /dev/ttyACM0 > > - slcand -f -s8 -o /dev/ttyUSB0 > > > > where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to > > 1Mbit/s. > > See the table below for further CAN bitrates: > > - s0 -> 10 Kbit/s > > - s1 -> 20 Kbit/s > > - s2 -> 50 Kbit/s > > - s3 -> 100 Kbit/s > > - s4 -> 125 Kbit/s > > - s5 -> 250 Kbit/s > > - s6 -> 500 Kbit/s > > - s7 -> 800 Kbit/s > > - s8 -> 1000 Kbit/s > > > > In doing so, the struct can_priv::bittiming.bitrate of the driver is not > > set and since the open_candev() checks that the bitrate has been set, it > > must be a non-zero value, the bitrate is set to a fake value (-1U) > > before it is called. > > > > The patch also changes the slcan_devs locking from rtnl to spin_lock. The > > change was tested with a kernel with the CONFIG_PROVE_LOCKING option > > enabled that did not show any errors. > > You're not allowed to call alloc_candev() with a spin_lock held. See > today's kernel test robot mail: > > | https://lore.kernel.org/all/YrpqO5jepAvv4zkf@xsang-OptiPlex-9020 > > I think it's best to keep the rtnl for now. The rtnl_lock() uses a mutex while I used a spin_lock. static DEFINE_MUTEX(rtnl_mutex); void rtnl_lock(void) { mutex_lock(&rtnl_mutex); } EXPORT_SYMBOL(rtnl_lock); So might it be worth trying with a mutex instead of rtnl_lock(), or do you think it is safer to return to rtn_lock () anyway? Thanks and regards, Dario > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 28.06.2022 11:48:48, Dario Binacchi wrote: > Hi Marc, > > On Tue, Jun 28, 2022 at 11:28 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > On 14.06.2022 14:28:14, Dario Binacchi wrote: > > > As suggested by commit [1], now the driver uses the functions and the > > > data structures provided by the CAN network device driver interface. > > > > > > Currently the driver doesn't implement a way to set bitrate for SLCAN > > > based devices via ip tool, so you'll have to do this by slcand or > > > slcan_attach invocation through the -sX parameter: > > > > > > - slcan_attach -f -s6 -o /dev/ttyACM0 > > > - slcand -f -s8 -o /dev/ttyUSB0 > > > > > > where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to > > > 1Mbit/s. > > > See the table below for further CAN bitrates: > > > - s0 -> 10 Kbit/s > > > - s1 -> 20 Kbit/s > > > - s2 -> 50 Kbit/s > > > - s3 -> 100 Kbit/s > > > - s4 -> 125 Kbit/s > > > - s5 -> 250 Kbit/s > > > - s6 -> 500 Kbit/s > > > - s7 -> 800 Kbit/s > > > - s8 -> 1000 Kbit/s > > > > > > In doing so, the struct can_priv::bittiming.bitrate of the driver is not > > > set and since the open_candev() checks that the bitrate has been set, it > > > must be a non-zero value, the bitrate is set to a fake value (-1U) > > > before it is called. > > > > > > The patch also changes the slcan_devs locking from rtnl to spin_lock. The > > > change was tested with a kernel with the CONFIG_PROVE_LOCKING option > > > enabled that did not show any errors. > > > > You're not allowed to call alloc_candev() with a spin_lock held. See > > today's kernel test robot mail: > > > > | https://lore.kernel.org/all/YrpqO5jepAvv4zkf@xsang-OptiPlex-9020 > > > > I think it's best to keep the rtnl for now. > > The rtnl_lock() uses a mutex while I used a spin_lock. > > static DEFINE_MUTEX(rtnl_mutex); > > void rtnl_lock(void) > { > mutex_lock(&rtnl_mutex); > } > EXPORT_SYMBOL(rtnl_lock); > > So might it be worth trying with a mutex instead of rtnl_lock(), or do > you think it is > safer to return to rtn_lock () anyway? As Max pointed out the whole static dev array is not needed at all. Just keep the rtnl, until removing the array altogether. regards, Marc
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index b2dcc1e5a388..45997d39621c 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -28,26 +28,6 @@ config CAN_VXCAN This driver can also be built as a module. If so, the module will be called vxcan. -config CAN_SLCAN - tristate "Serial / USB serial CAN Adaptors (slcan)" - depends on TTY - help - CAN driver for several 'low cost' CAN interfaces that are attached - via serial lines or via USB-to-serial adapters using the LAWICEL - ASCII protocol. The driver implements the tty linediscipline N_SLCAN. - - As only the sending and receiving of CAN frames is implemented, this - driver should work with the (serial/USB) CAN hardware from: - www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de - - Userspace tools to attach the SLCAN line discipline (slcan_attach, - slcand) can be found in the can-utils at the linux-can project, see - https://github.com/linux-can/can-utils for details. - - The slcan driver supports up to 10 CAN netdevices by default which - can be changed by the 'maxdev=xx' module option. This driver can - also be built as a module. If so, the module will be called slcan. - config CAN_DEV tristate "Platform CAN drivers with Netlink support" default y @@ -118,6 +98,26 @@ config CAN_KVASER_PCIEFD Kvaser Mini PCI Express HS v2 Kvaser Mini PCI Express 2xHS v2 +config CAN_SLCAN + tristate "Serial / USB serial CAN Adaptors (slcan)" + depends on TTY + help + CAN driver for several 'low cost' CAN interfaces that are attached + via serial lines or via USB-to-serial adapters using the LAWICEL + ASCII protocol. The driver implements the tty linediscipline N_SLCAN. + + As only the sending and receiving of CAN frames is implemented, this + driver should work with the (serial/USB) CAN hardware from: + www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de + + Userspace tools to attach the SLCAN line discipline (slcan_attach, + slcand) can be found in the can-utils at the linux-can project, see + https://github.com/linux-can/can-utils for details. + + The slcan driver supports up to 10 CAN netdevices by default which + can be changed by the 'maxdev=xx' module option. This driver can + also be built as a module. If so, the module will be called slcan. + config CAN_SUN4I tristate "Allwinner A10 CAN controller" depends on MACH_SUN4I || MACH_SUN7I || COMPILE_TEST diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c index c39580b142e0..c7ff11dd2278 100644 --- a/drivers/net/can/slcan.c +++ b/drivers/net/can/slcan.c @@ -56,7 +56,6 @@ #include <linux/can.h> #include <linux/can/dev.h> #include <linux/can/skb.h> -#include <linux/can/can-ml.h> MODULE_ALIAS_LDISC(N_SLCAN); MODULE_DESCRIPTION("serial line CAN interface"); @@ -79,6 +78,7 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces"); #define SLC_EFF_ID_LEN 8 struct slcan { + struct can_priv can; int magic; /* Various fields. */ @@ -100,6 +100,7 @@ struct slcan { }; static struct net_device **slcan_devs; +static DEFINE_SPINLOCK(slcan_lock); /************************************************************************ * SLCAN ENCAPSULATION FORMAT * @@ -394,6 +395,8 @@ static int slc_close(struct net_device *dev) clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags); } netif_stop_queue(dev); + close_candev(dev); + sl->can.state = CAN_STATE_STOPPED; sl->rcount = 0; sl->xleft = 0; spin_unlock_bh(&sl->lock); @@ -405,20 +408,34 @@ static int slc_close(struct net_device *dev) static int slc_open(struct net_device *dev) { struct slcan *sl = netdev_priv(dev); + int err; if (sl->tty == NULL) return -ENODEV; + /* The baud rate is not set with the command + * `ip link set <iface> type can bitrate <baud>' and therefore + * can.bittiming.bitrate is CAN_BITRATE_UNSET (0), causing + * open_candev() to fail. So let's set to a fake value. + */ + sl->can.bittiming.bitrate = CAN_BITRATE_UNKNOWN; + err = open_candev(dev); + if (err) { + netdev_err(dev, "failed to open can device\n"); + return err; + } + + sl->can.state = CAN_STATE_ERROR_ACTIVE; sl->flags &= BIT(SLF_INUSE); netif_start_queue(dev); return 0; } -/* Hook the destructor so we can free slcan devs at the right point in time */ -static void slc_free_netdev(struct net_device *dev) +static void slc_dealloc(struct slcan *sl) { - int i = dev->base_addr; + int i = sl->dev->base_addr; + free_candev(sl->dev); slcan_devs[i] = NULL; } @@ -434,24 +451,6 @@ static const struct net_device_ops slc_netdev_ops = { .ndo_change_mtu = slcan_change_mtu, }; -static void slc_setup(struct net_device *dev) -{ - dev->netdev_ops = &slc_netdev_ops; - dev->needs_free_netdev = true; - dev->priv_destructor = slc_free_netdev; - - dev->hard_header_len = 0; - dev->addr_len = 0; - dev->tx_queue_len = 10; - - dev->mtu = CAN_MTU; - dev->type = ARPHRD_CAN; - - /* New-style flags. */ - dev->flags = IFF_NOARP; - dev->features = NETIF_F_HW_CSUM; -} - /****************************************** Routines looking at TTY side. ******************************************/ @@ -514,11 +513,8 @@ static void slc_sync(void) static struct slcan *slc_alloc(void) { int i; - char name[IFNAMSIZ]; struct net_device *dev = NULL; - struct can_ml_priv *can_ml; struct slcan *sl; - int size; for (i = 0; i < maxdev; i++) { dev = slcan_devs[i]; @@ -531,16 +527,14 @@ static struct slcan *slc_alloc(void) if (i >= maxdev) return NULL; - sprintf(name, "slcan%d", i); - size = ALIGN(sizeof(*sl), NETDEV_ALIGN) + sizeof(struct can_ml_priv); - dev = alloc_netdev(size, name, NET_NAME_UNKNOWN, slc_setup); + dev = alloc_candev(sizeof(*sl), 1); if (!dev) return NULL; + snprintf(dev->name, sizeof(dev->name), "slcan%d", i); + dev->netdev_ops = &slc_netdev_ops; dev->base_addr = i; sl = netdev_priv(dev); - can_ml = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN); - can_set_ml_priv(dev, can_ml); /* Initialize channel control data */ sl->magic = SLCAN_MAGIC; @@ -573,11 +567,7 @@ static int slcan_open(struct tty_struct *tty) if (tty->ops->write == NULL) return -EOPNOTSUPP; - /* RTnetlink lock is misused here to serialize concurrent - opens of slcan channels. There are better ways, but it is - the simplest one. - */ - rtnl_lock(); + spin_lock(&slcan_lock); /* Collect hanged up channels. */ slc_sync(); @@ -605,13 +595,15 @@ static int slcan_open(struct tty_struct *tty) set_bit(SLF_INUSE, &sl->flags); - err = register_netdevice(sl->dev); - if (err) + err = register_candev(sl->dev); + if (err) { + pr_err("slcan: can't register candev\n"); goto err_free_chan; + } } /* Done. We have linked the TTY line to a channel. */ - rtnl_unlock(); + spin_unlock(&slcan_lock); tty->receive_room = 65536; /* We don't flow control */ /* TTY layer expects 0 on success */ @@ -621,14 +613,10 @@ static int slcan_open(struct tty_struct *tty) sl->tty = NULL; tty->disc_data = NULL; clear_bit(SLF_INUSE, &sl->flags); - slc_free_netdev(sl->dev); - /* do not call free_netdev before rtnl_unlock */ - rtnl_unlock(); - free_netdev(sl->dev); - return err; + slc_dealloc(sl); err_exit: - rtnl_unlock(); + spin_unlock(&slcan_lock); /* Count references from TTY module */ return err; @@ -658,9 +646,11 @@ static void slcan_close(struct tty_struct *tty) synchronize_rcu(); flush_work(&sl->tx_work); - /* Flush network side */ - unregister_netdev(sl->dev); - /* This will complete via sl_free_netdev */ + slc_close(sl->dev); + unregister_candev(sl->dev); + spin_lock(&slcan_lock); + slc_dealloc(sl); + spin_unlock(&slcan_lock); } static void slcan_hangup(struct tty_struct *tty) @@ -768,18 +758,29 @@ static void __exit slcan_exit(void) dev = slcan_devs[i]; if (!dev) continue; - slcan_devs[i] = NULL; - sl = netdev_priv(dev); - if (sl->tty) { - netdev_err(dev, "tty discipline still running\n"); - } + spin_lock(&slcan_lock); + dev = slcan_devs[i]; + if (dev) { + slcan_devs[i] = NULL; + spin_unlock(&slcan_lock); + sl = netdev_priv(dev); + if (sl->tty) { + netdev_err(dev, + "tty discipline still running\n"); + } - unregister_netdev(dev); + slc_close(dev); + unregister_candev(dev); + } else { + spin_unlock(&slcan_lock); + } } + spin_lock(&slcan_lock); kfree(slcan_devs); slcan_devs = NULL; + spin_unlock(&slcan_lock); tty_unregister_ldisc(&slc_ldisc); }
As suggested by commit [1], now the driver uses the functions and the data structures provided by the CAN network device driver interface. Currently the driver doesn't implement a way to set bitrate for SLCAN based devices via ip tool, so you'll have to do this by slcand or slcan_attach invocation through the -sX parameter: - slcan_attach -f -s6 -o /dev/ttyACM0 - slcand -f -s8 -o /dev/ttyUSB0 where -s6 in will set adapter's bitrate to 500 Kbit/s and -s8 to 1Mbit/s. See the table below for further CAN bitrates: - s0 -> 10 Kbit/s - s1 -> 20 Kbit/s - s2 -> 50 Kbit/s - s3 -> 100 Kbit/s - s4 -> 125 Kbit/s - s5 -> 250 Kbit/s - s6 -> 500 Kbit/s - s7 -> 800 Kbit/s - s8 -> 1000 Kbit/s In doing so, the struct can_priv::bittiming.bitrate of the driver is not set and since the open_candev() checks that the bitrate has been set, it must be a non-zero value, the bitrate is set to a fake value (-1U) before it is called. The patch also changes the slcan_devs locking from rtnl to spin_lock. The change was tested with a kernel with the CONFIG_PROVE_LOCKING option enabled that did not show any errors. [1] commit 39549eef3587f ("can: CAN Network device driver and Netlink interface") Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- Changes in v4: - Update the commit description. - Use the CAN_BITRATE_UNKNOWN macro. - Use kfree_skb() instead of can_put_echo_skb() in the slc_xmit(). - Remove the `if (slcan_devs)' check in the slc_dealloc(). Changes in v3: - Replace (-1) with (-1U) in the commit description. Changes in v2: - Move CAN_SLCAN Kconfig option inside CAN_DEV scope. - Improve the commit message. drivers/net/can/Kconfig | 40 +++++++-------- drivers/net/can/slcan.c | 107 ++++++++++++++++++++-------------------- 2 files changed, 74 insertions(+), 73 deletions(-)