Simple Rust driver that touches real hardware: PR 4/6 #305

TheSven73 · 2021-05-27T21:35:52Z

Simple Rust driver that touches real h/w: step 4 of 6

@alex and @ojeda suggested that #254 should be split into smaller PRs, so they can be reviewed and merged individually.

Roadmap:

bjorn3 · 2021-05-28T08:45:06Z

drivers/char/hw_random/bcm2835_rng_rust.rs

+            return Ok(0);
+        }
+
+        data.write(&0_u32)?;


Maybe add a FIXME just in case?

Thanks for the review! While the PR x/6 effort is ongoing, the driver is a continual WIP. I'm just adding functionality, as it becomes available in the Rust kernel core.

Do you think there is merit in placing a "warning this is a work in progress, not fully functional" comment on top of the driver?

Sounds reasonable to me. I'll add it.

ksquirrel · 2021-06-04T19:40:28Z

Review of d0472669f89e:

✔️ Commit 669248b: Looks fine!
✔️ Commit d047266: Looks fine!

TheSven73 · 2021-06-04T19:40:49Z

v1 -> v2:

rebased against latest rust branch

Device driver data corresponds to per-device context data or state. It is allocated on `probe()` and de-allocated in `remove()`. Signed-off-by: Sven Van Asbroeck <[email protected]>

To demonstrate device driver data in a non-trivial way, create a `miscdev` for each discovered device. This `miscdev` can only be opened and read. When read from userspace, it returns four zero bytes at a time. Note that `DrvData` consists of a `Pin<Box<miscdev::Registration>>`, which is a pinned structure. This demonstrates that pinned or self- referential structures may be used in `DrvData`. Signed-off-by: Sven Van Asbroeck <[email protected]>

TheSven73 · 2021-06-12T23:43:13Z

v2 -> v3:

rebased against latest rust branch

TheSven73 · 2021-06-12T23:46:59Z

I don't think this PR is particularly controversial or complex? It'd be great if more people could review, so the whole PR series could progress.

alex

One small question, otherwise LGTM.

alex · 2021-06-12T23:49:40Z

rust/kernel/platdev.rs

@@ -30,13 +30,31 @@ pub struct Registration {
 // (it is fine for multiple threads to have a shared reference to it).
 unsafe impl Sync for Registration {}

+extern "C" {
+    #[allow(improper_ctypes)]


What's the improper ctype here?

Without it, I get the following warning:

RUSTC L rust/kernel.o warning: `extern` block uses type `arch_spinlock_t`, which is not FFI-safe --> rust/kernel/platdev.rs:36:15 | 36 | pdev: *const bindings::platform_device, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe | = note: `#[warn(improper_ctypes)]` on by default = help: consider adding a member to this struct = note: this struct has no fields note: the type is defined here --> /home/sva/repos/kernel-arcx/rust/bindings_generated.rs:7331:1 | 7331 | pub struct arch_spinlock_t {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ warning: 1 warning emitted

What type is using arch_spinlock_t? Is it raw_spinlock? You can add the type to the opaque list:

linux/rust/bindgen_parameters

Line 10 in d8c6b9c

--opaque-type spinlock

.

Hi Gary, thanks for the help! I suspect that on arm 32-bit, arch_spinlock_t is just its own type, and not a typedef. So should I add it to --opaque-type?

linux/arch/arm/include/asm/spinlock_types.h

Lines 11 to 24 in d8c6b9c

typedef struct {

union {

u32 slock;

struct __raw_tickets {

#ifdef __ARMEB__

u16 next;

u16 owner;

#else

u16 owner;

u16 next;

#endif

} tickets;

};

} arch_spinlock_t;

Ok, when I add arch_spinlock_t to --opaque-type, I seem to enter some kind of rabbit-hole where different structures trigger the warning, and when you add those to --opaque-type, there are more structures that trigger it. So far I have:

--opaque-type arch_spinlock_t --opaque-type lock_class_key --opaque-type arch_rwlock_t --opaque-type vm_userfaultfd_ctx

and still the warnings keep coming.

Setting arch_spinlock_t itself to opaque type just propagate the issue one struct up :)

Do you need to peek into fields of platform_device? If not, just setting that to opaque type will be sufficient.

Edited wait a minute... yes, we do need to peek into platform_device. So that won't solve the issue...

But the peeking is just a hack, and will disappear once we proceed. I'll make a mental note to add platform_device to --opaque-type later, and remove the #[allow(improper_ctypes)].

TheSven73 · 2021-06-13T01:05:55Z

Thank you @alex, appreciate your kind assistance :)

While doing error injection testing I got the following panic kernel BUG at fs/btrfs/tree-log.c:1862! invalid opcode: 0000 [#1] SMP NOPTI CPU: 1 PID: 7836 Comm: mount Not tainted 5.13.0-rc1+ Rust-for-Linux#305 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014 RIP: 0010:link_to_fixup_dir+0xd5/0xe0 RSP: 0018:ffffb5800180fa30 EFLAGS: 00010216 RAX: fffffffffffffffb RBX: 00000000fffffffb RCX: ffff8f595287faf0 RDX: ffffb5800180fa37 RSI: ffff8f5954978800 RDI: 0000000000000000 RBP: ffff8f5953af9450 R08: 0000000000000019 R09: 0000000000000001 R10: 000151f408682970 R11: 0000000120021001 R12: ffff8f5954978800 R13: ffff8f595287faf0 R14: ffff8f5953c77dd0 R15: 0000000000000065 FS: 00007fc5284c8c40(0000) GS:ffff8f59bbd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fc5287f47c0 CR3: 000000011275e002 CR4: 0000000000370ee0 Call Trace: replay_one_buffer+0x409/0x470 ? btree_read_extent_buffer_pages+0xd0/0x110 walk_up_log_tree+0x157/0x1e0 walk_log_tree+0xa6/0x1d0 btrfs_recover_log_trees+0x1da/0x360 ? replay_one_extent+0x7b0/0x7b0 open_ctree+0x1486/0x1720 btrfs_mount_root.cold+0x12/0xea ? __kmalloc_track_caller+0x12f/0x240 legacy_get_tree+0x24/0x40 vfs_get_tree+0x22/0xb0 vfs_kern_mount.part.0+0x71/0xb0 btrfs_mount+0x10d/0x380 ? vfs_parse_fs_string+0x4d/0x90 legacy_get_tree+0x24/0x40 vfs_get_tree+0x22/0xb0 path_mount+0x433/0xa10 __x64_sys_mount+0xe3/0x120 do_syscall_64+0x3d/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae We can get -EIO or any number of legitimate errors from btrfs_search_slot(), panicing here is not the appropriate response. The error path for this code handles errors properly, simply return the error. Signed-off-by: Josef Bacik <[email protected]> Reviewed-by: David Sterba <[email protected]> Signed-off-by: David Sterba <[email protected]>

If "try_verify_in_tasklet" is set for dm-verity, DM_BUFIO_CLIENT_NO_SLEEP is enabled for dm-bufio. However, when bufio tries to evict buffers, there is a chance to trigger scheduling in spin_lock_bh, the following warning is hit: BUG: sleeping function called from invalid context at drivers/md/dm-bufio.c:2745 in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 123, name: kworker/2:2 preempt_count: 201, expected: 0 RCU nest depth: 0, expected: 0 4 locks held by kworker/2:2/123: #0: ffff88800a2d1548 ((wq_completion)dm_bufio_cache){....}-{0:0}, at: process_one_work+0xe46/0x1970 #1: ffffc90000d97d20 ((work_completion)(&dm_bufio_replacement_work)){....}-{0:0}, at: process_one_work+0x763/0x1970 #2: ffffffff8555b528 (dm_bufio_clients_lock){....}-{3:3}, at: do_global_cleanup+0x1ce/0x710 #3: ffff88801d5820b8 (&c->spinlock){....}-{2:2}, at: do_global_cleanup+0x2a5/0x710 Preemption disabled at: [<0000000000000000>] 0x0 CPU: 2 UID: 0 PID: 123 Comm: kworker/2:2 Not tainted 6.16.0-rc3-g90548c634bd0 #305 PREEMPT(voluntary) Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014 Workqueue: dm_bufio_cache do_global_cleanup Call Trace: <TASK> dump_stack_lvl+0x53/0x70 __might_resched+0x360/0x4e0 do_global_cleanup+0x2f5/0x710 process_one_work+0x7db/0x1970 worker_thread+0x518/0xea0 kthread+0x359/0x690 ret_from_fork+0xf3/0x1b0 ret_from_fork_asm+0x1a/0x30 </TASK> That can be reproduced by: veritysetup format --data-block-size=4096 --hash-block-size=4096 /dev/vda /dev/vdb SIZE=$(blockdev --getsz /dev/vda) dmsetup create myverity -r --table "0 $SIZE verity 1 /dev/vda /dev/vdb 4096 4096 <data_blocks> 1 sha256 <root_hash> <salt> 1 try_verify_in_tasklet" mount /dev/dm-0 /mnt -o ro echo 102400 > /sys/module/dm_bufio/parameters/max_cache_size_bytes [read files in /mnt] Cc: [email protected] # v6.4+ Fixes: 450e8de ("dm bufio: improve concurrent IO performance") Signed-off-by: Wang Shuai <[email protected]> Signed-off-by: Sheng Yong <[email protected]> Signed-off-by: Mikulas Patocka <[email protected]>

This comment has been minimized.

Sign in to view

TheSven73 changed the title ~~Simple Rust driver that touches real hardware: PR 3/6~~ Simple Rust driver that touches real hardware: PR 4/6 May 27, 2021

bjorn3 reviewed May 28, 2021

View reviewed changes

TheSven73 force-pushed the rust-for-linux-pdev-pr4 branch from fa1e44a to d047266 Compare June 4, 2021 19:40

Sven Van Asbroeck added 2 commits June 12, 2021 19:33

rust/kernel/platdev: add support for device driver data

1c7a51e

Device driver data corresponds to per-device context data or state. It is allocated on `probe()` and de-allocated in `remove()`. Signed-off-by: Sven Van Asbroeck <[email protected]>

TheSven73 force-pushed the rust-for-linux-pdev-pr4 branch from d047266 to 615cc58 Compare June 12, 2021 23:42

TheSven73 requested review from alex and wedsonaf June 12, 2021 23:43

alex reviewed Jun 12, 2021

View reviewed changes

alex approved these changes Jun 13, 2021

View reviewed changes

alex merged commit 523b4dd into Rust-for-Linux:rust Jun 13, 2021

TheSven73 deleted the rust-for-linux-pdev-pr4 branch June 13, 2021 01:05

	typedef struct {
	union {
	u32 slock;
	struct __raw_tickets {
	#ifdef __ARMEB__
	u16 next;
	u16 owner;
	#else
	u16 owner;
	u16 next;
	#endif
	} tickets;
	};
	} arch_spinlock_t;

Simple Rust driver that touches real hardware: PR 4/6 #305

Simple Rust driver that touches real hardware: PR 4/6 #305

Uh oh!

Conversation

TheSven73 commented May 27, 2021

Simple Rust driver that touches real h/w: step 4 of 6

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksquirrel commented Jun 4, 2021

Uh oh!

TheSven73 commented Jun 4, 2021

v1 -> v2:

Uh oh!

TheSven73 commented Jun 12, 2021

v2 -> v3:

Uh oh!

TheSven73 commented Jun 12, 2021

Uh oh!

alex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSven73 Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSven73 commented Jun 13, 2021

Uh oh!

Uh oh!

TheSven73 Jun 13, 2021 •

edited

Loading