rust: platform: add `ioremap_resource` and `get_resource` methods #682

m-falkowski1 · 2022-02-17T15:31:04Z

This patch adds a logic similar to devm_platform_ioremap_resource
function adding:

IoResource enumerated type that groups the IORESOURCE_* macros.
get_resource() method that is a binding of platform_get_resource
ioremap_resource that is newly written method similar to
devm_platform_ioremap_resource.

This patch is a dependency of a Samsung Exynos trng driver provided initially in #554.

Changes v3 -> v4:

Added 64-bit mask that is used to keep track of busy resources rust: platform: add ioremap_resource and get_resource methods #682 (comment),
Changed implementation of get_resource() and ioremap_resource() so that they check if
the resource was already taken rust: platform: add ioremap_resource and get_resource methods #682 (comment),
Documentation fixes rust: platform: add ioremap_resource and get_resource methods #682 (comment).

Changes v2 -> v3:

Error::EINVAL -> EINVAL,

Changes v1 -> v2:

Marked ioremap_resource() as unsafe.

Signed-off-by: Maciej Falkowski [email protected]

rust/kernel/platform.rs

m-falkowski1 · 2022-03-10T11:24:14Z

Rebased.

…ev_master_upper_dev_get_rcu Next patch uses l3mdev_master_upper_ifindex_by_index_rcu which throws a splat with debug kernels: [13783.087570] ------------[ cut here ]------------ [13783.093974] RTNL: assertion failed at net/core/dev.c (6702) [13783.100761] WARNING: CPU: 3 PID: 51132 at net/core/dev.c:6702 netdev_master_upper_dev_get+0x16a/0x1a0 [13783.184226] CPU: 3 PID: 51132 Comm: kworker/3:3 Not tainted 5.17.0-custom-100090-g6f963aafb1cc #682 [13783.194788] Hardware name: Mellanox Technologies Ltd. MSN2010/SA002610, BIOS 5.6.5 08/24/2017 [13783.204755] Workqueue: mld mld_ifc_work [ipv6] [13783.210338] RIP: 0010:netdev_master_upper_dev_get+0x16a/0x1a0 [13783.217209] Code: 0f 85 e3 fe ff ff e8 65 ac ec fe ba 2e 1a 00 00 48 c7 c6 60 6f 38 83 48 c7 c7 c0 70 38 83 c6 05 5e b5 d7 01 01 e8 c6 29 52 00 <0f> 0b e9 b8 fe ff ff e8 5a 6c 35 ff e9 1c ff ff ff 48 89 ef e8 7d [13783.238659] RSP: 0018:ffffc9000b37f5a8 EFLAGS: 00010286 [13783.244995] RAX: 0000000000000000 RBX: ffff88812ee5c000 RCX: 0000000000000000 [13783.253379] RDX: ffff88811ce09d40 RSI: ffffffff812d0fcd RDI: fffff5200166fea7 [13783.261769] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff8882375f4287 [13783.270138] R10: ffffed1046ebe850 R11: 0000000000000001 R12: dffffc0000000000 [13783.278510] R13: 0000000000000275 R14: ffffc9000b37f688 R15: ffff8881273b4af8 [13783.286870] FS: 0000000000000000(0000) GS:ffff888237400000(0000) knlGS:0000000000000000 [13783.296352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [13783.303177] CR2: 00007ff25fc9b2e8 CR3: 0000000174d23000 CR4: 00000000001006e0 [13783.311546] Call Trace: [13783.314660] <TASK> [13783.317553] l3mdev_master_upper_ifindex_by_index_rcu+0x43/0xe0 ... Change l3mdev_master_upper_ifindex_by_index_rcu to use netdev_master_upper_dev_get_rcu. Fixes: 6a6d668 ("l3mdev: add function to retreive upper master") Signed-off-by: Ido Schimmel <[email protected]> Signed-off-by: David Ahern <[email protected]> Cc: Alexis Bauvin <[email protected]> Signed-off-by: Jakub Kicinski <[email protected]>

wedsonaf · 2022-05-04T18:35:09Z

rust/kernel/platform.rs

@@ -180,6 +181,36 @@ impl Device {
        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
        unsafe { (*self.ptr).id }
    }
+
+    /// Gets a system resources of a platform device.
+    pub fn get_resource(&self, rtype: IoResource, num: usize) -> Option<Resource> {


One thing we do in amba and probably should do here too is to "remember" whether a resource was already taken. The idea is to limit the usage of a resource to only once so that we can avoid, for example, violating Rust's aliasing rules. See amba::Device::take_resource.

Can we do something similar here?

One thing we do in amba and probably should do here too is to "remember" whether a resource was already taken. The idea is to limit the usage of a resource to only once so that we can avoid, for example, violating Rust's aliasing rules. See amba::Device::take_resource.

I see, this seems like a good idea. Some of the resource need to be shared across drivers -
that is something that needs to be consider, but here this covers Rust's aliasing rules of the single module
if I understand correctly?

Can we do something similar here?

I'll give a try to implement it.

I noticed that in bindings::amba_device, the device data structure already contains resource data. However, in bindings::platform_device, the device data structure contains a pointer to resource data. So, the implementation of platform driver should give index when calling ioremap_resource(). This means the abstraction may differ from amba.

Moreover, I noticed that the io_mem::IoMem data structure must be initialized with a const SIZE. I think it only fits the model of amba rather than platform, as we need to choose resources by index in the runtime.

In a word, I think it might need more changes to implement this.

I noticed that in bindings::amba_device, the device data structure already contains resource data. However, in bindings::platform_device, the device data structure contains a pointer to resource data. So, the implementation of platform driver should give index when calling ioremap_resource(). This means the abstraction may differ from amba.

Yeah, but essentially the issue remains the same that is to ensure proper aliasing of retrieved resources.

Moreover, I noticed that the io_mem::IoMem data structure must be initialized with a const SIZE. I think it only fits the model of amba rather than platform, as we need to choose resources by index in the runtime.

I think that SIZE has nothing to do with this issue.

In a word, I think it might need more changes to implement this.

Yeah, it's harder the fact it's necessary to check if the given (RES_TYPE, index) pair was used previously.

I think that SIZE has nothing to do with this issue.

The driver may be compatible to multiple devices, and each of them may have different size of resource. So you don't know the size of the resource you get in the compile time.

The driver may be compatible to multiple devices, and each of them may have different size of resource. So you don't know the size of the resource you get in the compile time.

SIZE is the minimum size. This is not an issue.

Yeah, it's harder the fact it's necessary to check if the given (RES_TYPE, index) pair was used previously.

What I was thinking for this was to have a 64-bit number for each resource type. Each bit indicates whether the resource was previously taken. This obviously limits the number of resources to 64 per type but it's a simple implementation and covers the vast majority of devices.

What I was thinking for this was to have a 64-bit number for each resource type. Each bit indicates whether the resource was previously taken. This obviously limits the number of resources to 64 per type but it's a simple implementation and covers the vast majority of devices.

Sure, I was thinking about taking advantage that each resource has its unique position in the array so that its array index may be calculated using pointer difference and then masked.
WDYT?

It seems that your solution is less hacky and easier to code though.

Sure, I was thinking about taking advantage that each resource has its unique position in the array so that its array index may be calculated using pointer difference and then masked. WDYT?

I like this idea. I think it's simpler than mine because it requires a single mask. (Perhaps more fragile because it would break if the underlying implementation changed, but we can detect that the pointer is within the range we expect.)

Anyway, I'm happy with your proposed solution.

@wedsonaf I provided the solution with with a single 64-bit mask. That should be fine for smaller drivers. In future this could be replaced with wider bitmap<SIZE> or similar. May you please review?

rust/kernel/platform.rs

This patch adds a logic similar to `devm_platform_ioremap_resource` function adding: - `IoResource` enumerated type that groups the `IORESOURCE_*` macros. - `get_resource()` method that is a binding of `platform_get_resource` - `ioremap_resource` that is newly written method similar to `devm_platform_ioremap_resource`. Signed-off-by: Maciej Falkowski <[email protected]>

m-falkowski1 · 2022-05-13T18:01:08Z

v4 posted.

wedsonaf

Looks good, just a couple of related comments about detecting potential breaking changes in the future.

wedsonaf · 2022-06-01T15:53:09Z

rust/kernel/platform.rs

+        let index = unsafe { res.offset_from((*self.ptr).resource) } as usize;
+
+        // Make sure that the index does not exceed the 64-bit mask.
+        assert!(index < 64);


This isn't necessarily true, is it?

Instead of an assert this, should just be a check. If it's greater than or equal to 64, we fail the request.

wedsonaf · 2022-06-01T15:54:46Z

rust/kernel/platform.rs

+        //   - `self.ptr` is valid by the type invariant.
+        //   - `res` is a displaced pointer to one of the array's elements,
+        //     and `resource` is its base pointer.
+        let index = unsafe { res.offset_from((*self.ptr).resource) } as usize;


This happens to be how this is implemented today, but isn't necessarily going to always be this way. So I suggest we explicitly test here that the pointer is between self.ptr.resource and self.ptr.resource + 64. If it isn't, we should fail the request.

ojeda reviewed Feb 17, 2022

View reviewed changes

rust/kernel/platform.rs Outdated Show resolved Hide resolved

m-falkowski1 force-pushed the add_platform_ioremap_resource branch from b619c42 to e13b84f Compare February 17, 2022 15:55

m-falkowski1 mentioned this pull request Feb 18, 2022

Add Samsung Exynos true random number generator driver #554

Draft

10 tasks

m-falkowski1 force-pushed the add_platform_ioremap_resource branch from e13b84f to 606d6cc Compare March 10, 2022 11:23

m-falkowski1 force-pushed the add_platform_ioremap_resource branch from 606d6cc to d63e769 Compare March 17, 2022 13:35

wedsonaf reviewed May 4, 2022

View reviewed changes

m-falkowski1 force-pushed the add_platform_ioremap_resource branch from d63e769 to 40c2d5e Compare May 13, 2022 18:00

m-falkowski1 requested a review from wedsonaf May 13, 2022 18:02

wedsonaf reviewed Jun 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rust: platform: add `ioremap_resource` and `get_resource` methods #682

rust: platform: add `ioremap_resource` and `get_resource` methods #682

Uh oh!

m-falkowski1 commented Feb 17, 2022 •

edited

Loading

Uh oh!

Uh oh!

m-falkowski1 commented Mar 10, 2022

Uh oh!

wedsonaf May 4, 2022

Uh oh!

m-falkowski1 May 5, 2022

Uh oh!

SocialistDalao May 10, 2022

Uh oh!

m-falkowski1 May 11, 2022

Uh oh!

SocialistDalao May 12, 2022

Uh oh!

wedsonaf May 12, 2022

Uh oh!

m-falkowski1 May 12, 2022

Uh oh!

wedsonaf May 12, 2022

Uh oh!

m-falkowski1 Jun 1, 2022

Uh oh!

Uh oh!

m-falkowski1 commented May 13, 2022

Uh oh!

wedsonaf left a comment

Uh oh!

wedsonaf Jun 1, 2022

Uh oh!

wedsonaf Jun 1, 2022

Uh oh!

Uh oh!

rust: platform: add ioremap_resource and get_resource methods #682

Are you sure you want to change the base?

rust: platform: add ioremap_resource and get_resource methods #682

Uh oh!

Conversation

m-falkowski1 commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

m-falkowski1 commented Mar 10, 2022

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!

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!

Uh oh!

m-falkowski1 commented May 13, 2022

Uh oh!

wedsonaf 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!

Uh oh!

rust: platform: add `ioremap_resource` and `get_resource` methods #682

rust: platform: add `ioremap_resource` and `get_resource` methods #682

m-falkowski1 commented Feb 17, 2022 •

edited

Loading