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 <thesven73@gmail.com>

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 <thesven73@gmail.com>

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 <josef@toxicpanda.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>

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 <thesven73@gmail.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TheSven73 commented May 27, 2021

This comment has been minimized.

bjorn3 May 28, 2021

TheSven73 May 28, 2021

bjorn3 May 28, 2021

TheSven73 May 28, 2021

ksquirrel commented Jun 4, 2021

TheSven73 commented Jun 4, 2021

TheSven73 commented Jun 12, 2021

TheSven73 commented Jun 12, 2021

alex left a comment

alex Jun 12, 2021

TheSven73 Jun 13, 2021

nbdd0121 Jun 13, 2021

TheSven73 Jun 13, 2021

TheSven73 Jun 13, 2021

nbdd0121 Jun 13, 2021

TheSven73 Jun 13, 2021 •

edited

Loading

TheSven73 Jun 13, 2021

TheSven73 commented Jun 13, 2021

	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

Conversation

TheSven73 commented May 27, 2021

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

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksquirrel commented Jun 4, 2021

TheSven73 commented Jun 4, 2021

v1 -> v2:

TheSven73 commented Jun 12, 2021

v2 -> v3:

TheSven73 commented Jun 12, 2021

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSven73 Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSven73 commented Jun 13, 2021

TheSven73 Jun 13, 2021 •

edited

Loading