Message ID | 20230224-rust-iopt-rtkit-v1-5-49ced3391295@asahilina.net |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp836962wrd; Fri, 24 Feb 2023 03:01:45 -0800 (PST) X-Google-Smtp-Source: AK7set8H2Q+m8uZkgWFrVr8ubBBbpOzUr3AzsSHAGe5e6eoDVbBXj7mJGm49GSXwshnNEQPUBCAW X-Received: by 2002:a50:fc05:0:b0:4af:59c0:5a30 with SMTP id i5-20020a50fc05000000b004af59c05a30mr10666092edr.38.1677236505410; Fri, 24 Feb 2023 03:01:45 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1677236505; cv=none; d=google.com; s=arc-20160816; b=w/c11jtKWfE81C0EV7SEcX4MT+6DJov5B2SAouF0ztPOVDqWOXF9s03hocSzI+tUu3 xgaR3xnBuVeXSFYQSwjmWFonc00Nb+VqoRvFnb3VRrrRv3U+hduaz6tzjVoV/eYQJnaU pmk0s645t+JMP1mGetLZg6Ia6w4zO0CXwJPwrOpQvYA6XKad5tosG+yDKfYw6ydsQlqJ GdNMtsXF2JxDEBTwjbaMuNrGEKP/VwNzKrqfZG8POBp9mTssVx2G4Ea4Cr8B5krk3t3p oryXG8EJHhXznLsDbh3bbzfY7MhAPlc++n4vY5dshD6zGDPfeyZt8b4zCBKChc57BJM7 Tm0w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:in-reply-to:references:message-id :content-transfer-encoding:mime-version:subject:date:from :dkim-signature; bh=Ib32KPlKkGdBac9Oy/KyJF5GBykuAsqtiUUeH1X66Gs=; b=MkJ0QdhBunNO634rEmXiEF7Gfuc9cKsw5TWfQ+eAdN9tof6j98IiMnk+1ifYaZGxUq I8z1UAwn1+IWhL+7Brg5BQZvxahnMXCWbrnrdud//+rxmdQRHgi3k8g3jA6dvxD2d24L qRWr/xyGX50h9+ES/pUgY/SLYPd4W43eJ0b7M/9oYI1WjF8DMDLqhfaxE7vjStCkcM3K EeWILdmowGUca9xGoKEsfXNlvleoHf/p1M4B8TiKRbtqyR+aGrGfkBR+Ik7RoI0fh2fj wuYeO9AyT0TQA24Un4h5ODnThLyN50SPTaNZOx8RrsH5JL00fCVD4RXIm7Adl9GYDa07 CYdg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@asahilina.net header.s=default header.b=QQ+STDuF; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=asahilina.net Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id fh4-20020a1709073a8400b008cf79b051d8si17106461ejc.558.2023.02.24.03.00.49; Fri, 24 Feb 2023 03:01:45 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@asahilina.net header.s=default header.b=QQ+STDuF; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=asahilina.net Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230232AbjBXKzB (ORCPT <rfc822;jeff.pang.chn@gmail.com> + 99 others); Fri, 24 Feb 2023 05:55:01 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54928 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230197AbjBXKyf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 24 Feb 2023 05:54:35 -0500 Received: from mail.marcansoft.com (marcansoft.com [212.63.210.85]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3ADB665CFA; Fri, 24 Feb 2023 02:54:08 -0800 (PST) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: linasend@asahilina.net) by mail.marcansoft.com (Postfix) with ESMTPSA id 70EE74206F; Fri, 24 Feb 2023 10:54:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=asahilina.net; s=default; t=1677236046; bh=EdCqFOCkFmNCQy+An4xXL1lM8eljicqRMAsCfNlFpXc=; h=From:Date:Subject:References:In-Reply-To:To:Cc; b=QQ+STDuF5KmAnOI1efDmHlNdV4hDOgItX2AUUxdagG3FUFVa90uZxAGL7b8dUB7Pt 5QjG7nWPiB/Iq8Ltdn2HxdrrDHWPOJb3c0rtMWBaq+tDdwz4TYBgFKoe9oQDJFMyYN LQFz/vE/drpBjenLI8bhQyU/zpctXEnwUdi3m22lq8s0Sai1gZvvwXgMtsCjXAVlm0 7DayCYMhW3AuA7oiut0NtQNjrGvsadhl7fZI5KOY/UHYi42N1QGIR1ieMKtv0PLF1z JEbeIJBkLLKjIgRTi5TFHJpEFpZRr4qxYw16W2LiDqglXiENJ78ZjNj0jDto5vxOt8 hRpzEfbVxlyRg== From: Asahi Lina <lina@asahilina.net> Date: Fri, 24 Feb 2023 19:53:17 +0900 Subject: [PATCH 5/5] rust: device: Add a stub abstraction for devices MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20230224-rust-iopt-rtkit-v1-5-49ced3391295@asahilina.net> References: <20230224-rust-iopt-rtkit-v1-0-49ced3391295@asahilina.net> In-Reply-To: <20230224-rust-iopt-rtkit-v1-0-49ced3391295@asahilina.net> To: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>, Joerg Roedel <joro@8bytes.org>, Hector Martin <marcan@marcan.st>, Sven Peter <sven@svenpeter.dev>, Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Alyssa Rosenzweig <alyssa@rosenzweig.io>, Neal Gompa <neal@gompa.dev>, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, asahi@lists.linux.dev, Asahi Lina <lina@asahilina.net> X-Mailer: b4 0.12.0 X-Developer-Signature: v=1; a=ed25519-sha256; t=1677236013; l=5017; i=lina@asahilina.net; s=20230221; h=from:subject:message-id; bh=cdKIkyLwKJzj4pWUi4LyfvGZS5F3kfJKVHncm9kraaI=; b=OEIdscGIoIcKDDmhOPjbLGxA2i8Nhrg9rQMhaRWYJa7GQDqVwWSu3E/ZXAwXPBLNGXyvChxkV TnzkvIyCmpxAoIODcjIWil4i3mShoVRVROu+yHknTma9STuRcYVcEqq X-Developer-Key: i=lina@asahilina.net; a=ed25519; pk=Qn8jZuOtR1m5GaiDfTrAoQ4NE1XoYVZ/wmt5YtXWFC4= X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1758709945877295962?= X-GMAIL-MSGID: =?utf-8?q?1758709945877295962?= |
Series |
rust: Add io_pgtable and RTKit abstractions
|
|
Commit Message
Asahi Lina
Feb. 24, 2023, 10:53 a.m. UTC
From: Wedson Almeida Filho <wedsonaf@gmail.com> Add a Device type which represents an owned reference to a generic struct device. This minimal implementation just handles reference counting and allows the user to get the device name. Lina: Rewrote commit message, dropped the Amba bits, and squashed in simple changes to the core Device code from latter commits in rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata helper which will be needed by consumers later on anyway. Co-developed-by: Miguel Ojeda <ojeda@kernel.org> Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> Signed-off-by: Asahi Lina <lina@asahilina.net> --- rust/helpers.c | 13 +++++++++ rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-)
Comments
On Fri, Feb 24, 2023 at 07:53:17PM +0900, Asahi Lina wrote: > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > Add a Device type which represents an owned reference to a generic > struct device. This minimal implementation just handles reference > counting and allows the user to get the device name. What good is just the device name? I'm all for proper bindings to hook up properly to the driver core, but this feels like it's not really doing much of anything. Do you have a real user that we can see how this is interacting? And what does a driver care about the device name anyway? It should only be using the dev_*() calls to print that info out to the log, and never messing around with it in any other format as that's what userspace expects. > Lina: Rewrote commit message, dropped the Amba bits, and squashed in > simple changes to the core Device code from latter commits in > rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata > helper which will be needed by consumers later on anyway. > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Asahi Lina <lina@asahilina.net> > --- > rust/helpers.c | 13 +++++++++ > rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/rust/helpers.c b/rust/helpers.c > index 04b9be46e887..54954fd80c77 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -20,6 +20,7 @@ > > #include <linux/bug.h> > #include <linux/build_bug.h> > +#include <linux/device.h> > #include <linux/err.h> > #include <linux/refcount.h> > > @@ -65,6 +66,18 @@ long rust_helper_PTR_ERR(__force const void *ptr) > } > EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR); > > +void *rust_helper_dev_get_drvdata(struct device *dev) > +{ > + return dev_get_drvdata(dev); > +} > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata); No matching dev_set_drvdata()? What good is getting a random void pointer if you couldn't set it in the first place? :) > +const char *rust_helper_dev_name(const struct device *dev) > +{ > + return dev_name(dev); > +} > +EXPORT_SYMBOL_GPL(rust_helper_dev_name); Again, why? What is going to use this? And I don't really understand the rules you are putting on the name string after calling this, more below: > + > /* > * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type > * as the Rust `usize` type, so we can use it in contexts where Rust > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > index 9be021e393ca..e57da622d817 100644 > --- a/rust/kernel/device.rs > +++ b/rust/kernel/device.rs > @@ -4,7 +4,7 @@ > //! > //! C header: [`include/linux/device.h`](../../../../include/linux/device.h) > > -use crate::bindings; > +use crate::{bindings, str::CStr}; > > /// A raw device. > /// > @@ -20,4 +20,78 @@ use crate::bindings; > pub unsafe trait RawDevice { > /// Returns the raw `struct device` related to `self`. > fn raw_device(&self) -> *mut bindings::device; > + > + /// Returns the name of the device. > + fn name(&self) -> &CStr { > + let ptr = self.raw_device(); > + > + // SAFETY: `ptr` is valid because `self` keeps it alive. > + let name = unsafe { bindings::dev_name(ptr) }; > + > + // SAFETY: The name of the device remains valid while it is alive (because the device is > + // never renamed, per the safety requirement of this trait). This is guaranteed to be the > + // case because the reference to `self` outlives the one of the returned `CStr` (enforced > + // by the compiler because of their lifetimes). > + unsafe { CStr::from_char_ptr(name) } Why can the device never be renamed? Devices are renamed all the time, sometimes when you least expect it (i.e. by userspace). So how is this considered "safe"? and actually correct? Again, maybe seeing a real user of this might make more sense, but as-is, this feels wrong and not needed at all. > + } > +} > + > +/// A ref-counted device. > +/// > +/// # Invariants > +/// > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by > +/// `self`, and will be decremented when `self` is dropped. > +pub struct Device { > + pub(crate) ptr: *mut bindings::device, > +} > + > +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread. > +unsafe impl Send for Device {} > + > +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used > +// from any thread. > +unsafe impl Sync for Device {} > + > +impl Device { > + /// Creates a new device instance. > + /// > + /// # Safety > + /// > + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. > + pub unsafe fn new(ptr: *mut bindings::device) -> Self { > + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. > + unsafe { bindings::get_device(ptr) }; You don't check the return value of get_device()? What if it failed (hint, it can)? > + // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self` > + // owns a reference. This is satisfied by the call to `get_device` above. > + Self { ptr } > + } > + > + /// Creates a new device instance from an existing [`RawDevice`] instance. > + pub fn from_dev(dev: &dyn RawDevice) -> Self { I am a rust newbie, but I don't understand this "RawDevice" here at all. > + // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety > + // requirements. > + unsafe { Self::new(dev.raw_device()) } > + } > +} > + > +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference. > +unsafe impl RawDevice for Device { > + fn raw_device(&self) -> *mut bindings::device { > + self.ptr > + } What does this raw device do? What is the relationship to a "real" device? Maybe it's just my lack of rust knowledge here showing, so any hints would be appreciated. > +} > + > +impl Drop for Device { > + fn drop(&mut self) { > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // relinquish it now. > + unsafe { bindings::put_device(self.ptr) }; So it is now ok for this to be freed? One meta-comment, why is any of this needed at all? No driver should ever be dealing with a "raw" struct device pointer at all, right? They should be calling into subsystems that give it a pointer to a bus-specific pointer type (gpio, usb, pci, etc.) So I'm thinking that adding support for "raw" struct device pointers feels ripe for abuse in a "the code should not be doing that" type of thing. Unless you are writing a new bus/class in rust? thanks, greg k-h
Hi! First, I should say that some of this is really an example, and this abstraction doesn't need to go in as part of this series. I just added it at the end as an example of how RawDevice could/will be implemented, but there is more discussion that needs to happen around Devices (especially around how actual driver implementations and device data stuff work) I think. The main goal of having the RawDevice trait now is that we can start reviewing and upstreaming things that aren't part of the device model but take device references. Otherwise we end up serializing everything too much and it will be difficult to get everything upstream in a reasonable timeframe... Also note that I didn't write any of this code so if you have questions about *why* it was all designed like this, I think Wedson will have to answer that ^^ On 2023/02/24 20:19, Greg Kroah-Hartman wrote: > On Fri, Feb 24, 2023 at 07:53:17PM +0900, Asahi Lina wrote: >> From: Wedson Almeida Filho <wedsonaf@gmail.com> >> >> Add a Device type which represents an owned reference to a generic >> struct device. This minimal implementation just handles reference >> counting and allows the user to get the device name. > > What good is just the device name? I'm all for proper bindings to hook > up properly to the driver core, but this feels like it's not really > doing much of anything. I think we can just drop this, I don't use it. It was just an example of how a RawDevice method might work that is in the downstream tree, and I kept it here. A more practical example would be `of_node()`, which returns the OF node associated with a device and is how the OF abstraction I wrote hooks into the device model. I think I can submit that one pretty soon too, if I'm not mistaken. >> +void *rust_helper_dev_get_drvdata(struct device *dev) >> +{ >> + return dev_get_drvdata(dev); >> +} >> +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata); > > No matching dev_set_drvdata()? What good is getting a random void > pointer if you couldn't set it in the first place? :) I thought something else used this, but looking again the bus drivers just use stuff like platform_{set,get}_drvdata. I'll drop it, I'm not sure why I thought I needed this later... >> +impl Device { >> + /// Creates a new device instance. >> + /// >> + /// # Safety >> + /// >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. >> + pub unsafe fn new(ptr: *mut bindings::device) -> Self { >> + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. >> + unsafe { bindings::get_device(ptr) }; > > You don't check the return value of get_device()? What if it failed > (hint, it can)? Really? I looked at the implementation and I don't see how it can fail, as long as the argument is non-NULL and valid... (which is a precondition of this unsafe function). Did I miss something? >> + // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self` >> + // owns a reference. This is satisfied by the call to `get_device` above. >> + Self { ptr } >> + } >> + >> + /// Creates a new device instance from an existing [`RawDevice`] instance. >> + pub fn from_dev(dev: &dyn RawDevice) -> Self { > > I am a rust newbie, but I don't understand this "RawDevice" here at all. RawDevice is a trait, so this means "a dynamic reference to any type that implements RawDevice". That could be a reference to a bus device outright (like a PlatformDevice), or another Device, or a type-erased dynamic object that is "some device type" but you don't know which (like a reference taken from a Box<dyn RawDevice>). What this means is that you can always create a Device from a reference to anything that is a device (a RawDevice impl). This code path does a get_device(), so it creates a new reference that becomes logically owned by the Device object. Think of it like "clone, and erase the specific device type to a generic device". This is the operation you would use in a kernel abstraction that needs to take "some device", and then grab its own reference and keep it for later, but which does not care about the specific bus type. In particular, the way the bus abstractions work right now downstream, the specific bus device types are not cloneable and only represent borrowed device references that exist for the lifetime of the driver probe callback. The idea is that you grab references to all the bus-specific resources you need within (there is revocability logic to make sure resources can disappear without breaking pointers, failing gracefully if accessed) and do any bus-specific init you need, and then you no longer need to directly touch the bus device after that. This is definitely not going to cover some corner cases, but I haven't had any need to use platform device ops in my GPU driver after probe, so it works for me. But the driver does need to pass around device references and keep them in some inner structures to make things like `dev_warn!()` macros work, and that is what `Device` and `RawDevice` are for: `Device` is an owning reference that can be cloned and constructed out of the original `PlatformDevice` and can outlive it. >> + // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety >> + // requirements. >> + unsafe { Self::new(dev.raw_device()) } >> + } >> +} >> + >> +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference. >> +unsafe impl RawDevice for Device { >> + fn raw_device(&self) -> *mut bindings::device { >> + self.ptr >> + } > > What does this raw device do? What is the relationship to a "real" > device? Maybe it's just my lack of rust knowledge here showing, so any > hints would be appreciated. This means that a Device (an owned reference to a generic struct device) can be used anywhere you need a generic device reference (this sounds tautological but the idea is that specific bus device types can also be used, which is why this layer of indirection exists). In this case it's just an accessor for self.ptr since a Device is just a wrapper around the pointer. > >> +} >> + >> +impl Drop for Device { >> + fn drop(&mut self) { >> + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to >> + // relinquish it now. >> + unsafe { bindings::put_device(self.ptr) }; > > So it is now ok for this to be freed? Yes, because the Rust type system guarantees that Drop only gets called when the Device no longer has an owner. That means there are no references of any kind left and the object is ceasing to exist. Rust also blocks you from manually calling drop() (this is a special case, without it you could drop something more than once). In other words, a Device represents a reference to a struct device. Once the Device gets dropped, there is nobody left to potentially use that reference through the Device by definition. (Of course you can invalidly stash away the raw pointer like those C API issues you found... but that's a different problem). One way to solve the C abstraction issue would be to embed a Device in the RtKit/IoPageTableInner structs (the respective Rust abstraction types), creating it out of the passed RawDevice reference with Device::from_raw() in the constructor for those. That means you'd automatically `get_device()` on construction and `put_device()` when the abstraction objects get dropped, and that would solve all the problems without any extra cleanup or management code. > One meta-comment, why is any of this needed at all? No driver should > ever be dealing with a "raw" struct device pointer at all, right? They > should be calling into subsystems that give it a pointer to a > bus-specific pointer type (gpio, usb, pci, etc.) I'm getting the feeling maybe the name is just bad. Maybe it should be called AbstractDevice or IntoDevice or something like that. It's just "some device". Drivers indeed should never be dealing with actual device pointers, the pointers are for consumption by the kernel crate only. That this is done using trait functions that can technically be called by the driver code is just the way Rust models this: you need *some* trait function to get at the raw pointer (because the kernel crate needs it) and the trait needs to be public (because it's part of the public interface as the abstract device type) and there is no such thing as private trait functions in a public trait in Rust. But the idea is that this doesn't matter: sure, drivers can *get* a raw device pointer but they can't *do* anything with it without unsafe. Technically though, if we wanted to hide this further, we could make it return a newtype wrapper around the raw pointer. Then as long as the inner field is not public, drivers wouldn't even be able to get at the raw pointer but the kernel crate would. I'm not sure if this is worth it, though. It's not like drivers can't work around that with unsafe anyway, once you use unsafe you can do all kinds of crazy stuff like raw memory reinterpretations, copies, and transmutations, which will allow you to cheat the entire type system if you want... but of course the whole idea here is that we make things impossible in safe code and then we look at the unsafe blocks with a magnifying glass during review to make sure they aren't doing anything actually unsafe or crazy. ~~ Lina
On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote: > >> +impl Device { > >> + /// Creates a new device instance. > >> + /// > >> + /// # Safety > >> + /// > >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. > >> + pub unsafe fn new(ptr: *mut bindings::device) -> Self { > >> + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. > >> + unsafe { bindings::get_device(ptr) }; > > > > You don't check the return value of get_device()? What if it failed > > (hint, it can)? > > Really? I looked at the implementation and I don't see how it can fail, > as long as the argument is non-NULL and valid... (which is a > precondition of this unsafe function). Did I miss something? This function has changed a bit over time, but yes, it could fail if someone else just dropped the last reference right before you tried to grab a new one (look at the implementation of kobject_get()). Yes, if you "know" you have a non-zero reference count first, it should never fail, but how do you know this? We have the ability to check the return value of the function, shouldn't that happen? thanks, greg k-h
On 25/02/2023 00.34, Greg Kroah-Hartman wrote: > On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote: >>>> +impl Device { >>>> + /// Creates a new device instance. >>>> + /// >>>> + /// # Safety >>>> + /// >>>> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. >>>> + pub unsafe fn new(ptr: *mut bindings::device) -> Self { >>>> + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. >>>> + unsafe { bindings::get_device(ptr) }; >>> >>> You don't check the return value of get_device()? What if it failed >>> (hint, it can)? >> >> Really? I looked at the implementation and I don't see how it can fail, >> as long as the argument is non-NULL and valid... (which is a >> precondition of this unsafe function). Did I miss something? > > This function has changed a bit over time, but yes, it could fail if > someone else just dropped the last reference right before you tried to > grab a new one (look at the implementation of kobject_get()). > > Yes, if you "know" you have a non-zero reference count first, it should > never fail, but how do you know this? We have the ability to check the > return value of the function, shouldn't that happen? Well, we know this because it is part of the invariant. If we don't uphold invariants, things fall apart... that's why we create safe abstractions that don't let you break those invariants after all! In this particular case though, I don't see what we can usefully do here. `device_get()` is going to be part of Clone impls and things like that which are non-fallible. At most we can assert!() and rust-panic (which is a BUG as far as I know) if it fails... would that be preferable? ~~ Lina
On Sat, Feb 25, 2023 at 12:51:38AM +0900, Asahi Lina wrote: > On 25/02/2023 00.34, Greg Kroah-Hartman wrote: > > On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote: > >>>> +impl Device { > >>>> + /// Creates a new device instance. > >>>> + /// > >>>> + /// # Safety > >>>> + /// > >>>> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. > >>>> + pub unsafe fn new(ptr: *mut bindings::device) -> Self { > >>>> + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. > >>>> + unsafe { bindings::get_device(ptr) }; > >>> > >>> You don't check the return value of get_device()? What if it failed > >>> (hint, it can)? > >> > >> Really? I looked at the implementation and I don't see how it can fail, > >> as long as the argument is non-NULL and valid... (which is a > >> precondition of this unsafe function). Did I miss something? > > > > This function has changed a bit over time, but yes, it could fail if > > someone else just dropped the last reference right before you tried to > > grab a new one (look at the implementation of kobject_get()). > > > > Yes, if you "know" you have a non-zero reference count first, it should > > never fail, but how do you know this? We have the ability to check the > > return value of the function, shouldn't that happen? > > Well, we know this because it is part of the invariant. If we don't > uphold invariants, things fall apart... that's why we create safe > abstractions that don't let you break those invariants after all! > > In this particular case though, I don't see what we can usefully do > here. `device_get()` is going to be part of Clone impls and things like > that which are non-fallible. At most we can assert!() and rust-panic > (which is a BUG as far as I know) if it fails... would that be preferable? That's a good question, I don't know. In thinking about it some more, I think we are ok with this as-is for now. BUT, I want to see any code that is actually using a struct device, and I really want to review the heck out of the platform device api that you must have somewhere, as that might show some other issues around this type of thing that might be lurking. thanks, greg k-h
On 25/02/2023 01.31, Greg Kroah-Hartman wrote: > On Sat, Feb 25, 2023 at 12:51:38AM +0900, Asahi Lina wrote: >> On 25/02/2023 00.34, Greg Kroah-Hartman wrote: >>> On Sat, Feb 25, 2023 at 12:10:47AM +0900, Asahi Lina wrote: >>>>>> +impl Device { >>>>>> + /// Creates a new device instance. >>>>>> + /// >>>>>> + /// # Safety >>>>>> + /// >>>>>> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. >>>>>> + pub unsafe fn new(ptr: *mut bindings::device) -> Self { >>>>>> + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. >>>>>> + unsafe { bindings::get_device(ptr) }; >>>>> >>>>> You don't check the return value of get_device()? What if it failed >>>>> (hint, it can)? >>>> >>>> Really? I looked at the implementation and I don't see how it can fail, >>>> as long as the argument is non-NULL and valid... (which is a >>>> precondition of this unsafe function). Did I miss something? >>> >>> This function has changed a bit over time, but yes, it could fail if >>> someone else just dropped the last reference right before you tried to >>> grab a new one (look at the implementation of kobject_get()). >>> >>> Yes, if you "know" you have a non-zero reference count first, it should >>> never fail, but how do you know this? We have the ability to check the >>> return value of the function, shouldn't that happen? >> >> Well, we know this because it is part of the invariant. If we don't >> uphold invariants, things fall apart... that's why we create safe >> abstractions that don't let you break those invariants after all! >> >> In this particular case though, I don't see what we can usefully do >> here. `device_get()` is going to be part of Clone impls and things like >> that which are non-fallible. At most we can assert!() and rust-panic >> (which is a BUG as far as I know) if it fails... would that be preferable? > > That's a good question, I don't know. In thinking about it some more, > I think we are ok with this as-is for now. > > BUT, I want to see any code that is actually using a struct device, and > I really want to review the heck out of the platform device api that you > must have somewhere, as that might show some other issues around this > type of thing that might be lurking. The platform abstraction (which I didn't write either) is here [1], though that's just pulled straight from RfL right now without proper commit/attribution info and has a few other dependencies still. But the refcounting is actually really boring on that one... there is none! A platform::Device instance only exists during probe and cannot be cloned or stolen, so it only represents a borrowed reference during that time, and obviously the subsystem is guaranteed to hold a reference while the driver probe callback runs (right? I mean, things would be incredibly broken if that weren't the case and probe could randomly lose the reference halfway through). There is definitely going to come a time when this isn't enough, but right now with the device resource model stuff it works, and of course it's really easy to review since it's so simple! More to the point though: I think it's probably okay if some things can't be checked in these initial submissions, but then we check them as more users (in `kernel` and drivers) come in later? As you mentioned there's a chicken-and-egg issue, and also I'd say a mismatch between "what you need to validate certain invariants/design decisions" and "what is reasonable to submit as a single series". But at the end of the day we're going to have to refer back to this code when reviewing further code that uses it inside `kernel` anyway (especially this early on), so it's probably okay to fix anything that we missed or discover is broken then? I certainly don't mind adding a bunch of "fix broken thing" patches at the head of later reviews if we run into issues ^^ [1] https://github.com/Rust-for-Linux/linux/pull/969/commits/33770890aa71a491d81ec2cb2b7a45d953d1b7dd ~~ Lina
Hi Greg, As Lina points out, I wrote this code, so I'll try to answer your questions. Lina, apologies for the delay in participating, I've been distracted with other stuff. On Fri, 24 Feb 2023 at 08:19, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Feb 24, 2023 at 07:53:17PM +0900, Asahi Lina wrote: > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > Add a Device type which represents an owned reference to a generic > > struct device. This minimal implementation just handles reference > > counting and allows the user to get the device name. > > What good is just the device name? I'm all for proper bindings to hook > up properly to the driver core, but this feels like it's not really > doing much of anything. > > Do you have a real user that we can see how this is interacting? > > And what does a driver care about the device name anyway? It should > only be using the dev_*() calls to print that info out to the log, and > never messing around with it in any other format as that's what > userspace expects. I added this for the pl061 gpio driver, which initialises the gpio chip label with the device name: https://elixir.bootlin.com/linux/v6.2.2/source/drivers/gpio/gpio-pl061.c#L333 Someone suggested this driver as good example in some of these earlier interminable threads. The source code for it is in our Rust-for-Linux rust branch. > > Lina: Rewrote commit message, dropped the Amba bits, and squashed in > > simple changes to the core Device code from latter commits in > > rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata > > helper which will be needed by consumers later on anyway. > > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > > Signed-off-by: Asahi Lina <lina@asahilina.net> > > --- > > rust/helpers.c | 13 +++++++++ > > rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 88 insertions(+), 1 deletion(-) > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > index 04b9be46e887..54954fd80c77 100644 > > --- a/rust/helpers.c > > +++ b/rust/helpers.c > > @@ -20,6 +20,7 @@ > > > > #include <linux/bug.h> > > #include <linux/build_bug.h> > > +#include <linux/device.h> > > #include <linux/err.h> > > #include <linux/refcount.h> > > > > @@ -65,6 +66,18 @@ long rust_helper_PTR_ERR(__force const void *ptr) > > } > > EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR); > > > > +void *rust_helper_dev_get_drvdata(struct device *dev) > > +{ > > + return dev_get_drvdata(dev); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata); > > No matching dev_set_drvdata()? What good is getting a random void > pointer if you couldn't set it in the first place? :) > > > +const char *rust_helper_dev_name(const struct device *dev) > > +{ > > + return dev_name(dev); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_dev_name); > > Again, why? What is going to use this? > > And I don't really understand the rules you are putting on the name > string after calling this, more below: > > > + > > /* > > * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type > > * as the Rust `usize` type, so we can use it in contexts where Rust > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > > index 9be021e393ca..e57da622d817 100644 > > --- a/rust/kernel/device.rs > > +++ b/rust/kernel/device.rs > > @@ -4,7 +4,7 @@ > > //! > > //! C header: [`include/linux/device.h`](../../../../include/linux/device.h) > > > > -use crate::bindings; > > +use crate::{bindings, str::CStr}; > > > > /// A raw device. > > /// > > @@ -20,4 +20,78 @@ use crate::bindings; > > pub unsafe trait RawDevice { > > /// Returns the raw `struct device` related to `self`. > > fn raw_device(&self) -> *mut bindings::device; > > + > > + /// Returns the name of the device. > > + fn name(&self) -> &CStr { > > + let ptr = self.raw_device(); > > + > > + // SAFETY: `ptr` is valid because `self` keeps it alive. > > + let name = unsafe { bindings::dev_name(ptr) }; > > + > > + // SAFETY: The name of the device remains valid while it is alive (because the device is > > + // never renamed, per the safety requirement of this trait). This is guaranteed to be the > > + // case because the reference to `self` outlives the one of the returned `CStr` (enforced > > + // by the compiler because of their lifetimes). > > + unsafe { CStr::from_char_ptr(name) } > > Why can the device never be renamed? Devices are renamed all the time, > sometimes when you least expect it (i.e. by userspace). So how is this > considered "safe"? and actually correct? > > Again, maybe seeing a real user of this might make more sense, but > as-is, this feels wrong and not needed at all. This requirement is to allow callers to use the string without having to make a copy of it. If subsystems/buses are not following what the C documentation says, as you point out in another thread, we have a several options: (a) remove access to names altogether, (b) leave things as they are, then those subsystems wouldn't be able to honour the safety requirements of this trait therefore they wouldn't implement it, (c) make a copy of the string, etc. Since we don't have immediate plans to upstream the pl061 driver, I think we should just remove the name functionality for now and revisit it later if the need arises. > > > + } > > +} > > + > > +/// A ref-counted device. > > +/// > > +/// # Invariants > > +/// > > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by > > +/// `self`, and will be decremented when `self` is dropped. > > +pub struct Device { > > + pub(crate) ptr: *mut bindings::device, > > +} > > + > > +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread. > > +unsafe impl Send for Device {} > > + > > +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used > > +// from any thread. > > +unsafe impl Sync for Device {} > > + > > +impl Device { > > + /// Creates a new device instance. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. > > + pub unsafe fn new(ptr: *mut bindings::device) -> Self { > > + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. > > + unsafe { bindings::get_device(ptr) }; > > You don't check the return value of get_device()? What if it failed > (hint, it can)? I think this was already discussed, but anyway: the caller owns a reference to this device, so `get_device` is safe to call and cannot fail. Note that `get_device` calls `kobject_get`, which doesn't fail. I suppose you're hinting at `kobject_get_unless_zero`, which of course can fail but isn't in use here. > > > + // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self` > > + // owns a reference. This is satisfied by the call to `get_device` above. > > + Self { ptr } > > + } > > + > > + /// Creates a new device instance from an existing [`RawDevice`] instance. > > + pub fn from_dev(dev: &dyn RawDevice) -> Self { > > I am a rust newbie, but I don't understand this "RawDevice" here at all. Different buses will have their own Rust "Device" type, for example, pci::Device, amba::Device, platform::Device that wrap their C counterparts pci_dev, amba_device, platform_device. "RawDevice" is a trait for functionality that is common to all devices. It exposes the "struct device" of each bus/subsystem so that functions that work on any "struct device", for example, `clk_get`, `pr_info`. will automatically work on all subsystems. For example, as part writing Rust abstractions for a platform devices, we have a platform::Device type, which is wrapper around `struct platform_device`. It has a bunch of associated functions that do things that are specific to the platform bus. But then they also implement the `RawDevice` trait (by implementing `raw_device` that returns &pdev->dev), which allows drivers to call `clk_get` and the printing functions directly. Let's say `pdev` is a platform device; if we wanted to call `clk_get` in C, we'd do something like: clk = clk_get(&pdev->dev, NULL); In Rust, we'd do: clk = pdev.clk_get(None); (Note that we didn't have to know that pdev had a field whose type is a `struct device` that we could use to call clk_get on; `RawDevice` encoded this information.) Does the intent of the abstraction make sense to you now? > > + // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety > > + // requirements. > > + unsafe { Self::new(dev.raw_device()) } > > + } > > +} > > + > > +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference. > > +unsafe impl RawDevice for Device { > > + fn raw_device(&self) -> *mut bindings::device { > > + self.ptr > > + } > > What does this raw device do? What is the relationship to a "real" > device? Maybe it's just my lack of rust knowledge here showing, so any > hints would be appreciated. > This is the function that returns the `struct device` field embedded in the different devices for each bus. > > +} > > + > > +impl Drop for Device { > > + fn drop(&mut self) { > > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > > + // relinquish it now. > > + unsafe { bindings::put_device(self.ptr) }; > > So it is now ok for this to be freed? > > One meta-comment, why is any of this needed at all? No driver should > ever be dealing with a "raw" struct device pointer at all, right? They > should be calling into subsystems that give it a pointer to a > bus-specific pointer type (gpio, usb, pci, etc.) > > So I'm thinking that adding support for "raw" struct device pointers > feels ripe for abuse in a "the code should not be doing that" type of > thing. > > Unless you are writing a new bus/class in rust? Drivers will not use this directly. They'll benefit from it as I described above to be able to call bus-agnostic functions. Implementers of bus abstractions will use this to say "my device also has a `struct device` in it, and I want it to benefit from all bus-agnostic device manipulation functions". Implementers of abstractions of bus-agnostic device functions will add their abstractions to `RawDevice` instead of bus-specific types so that they work with all devices. Cheers, -Wedson
On Sun, Mar 05, 2023 at 03:39:25AM -0300, Wedson Almeida Filho wrote: > > > + /// Returns the name of the device. > > > + fn name(&self) -> &CStr { > > > + let ptr = self.raw_device(); > > > + > > > + // SAFETY: `ptr` is valid because `self` keeps it alive. > > > + let name = unsafe { bindings::dev_name(ptr) }; > > > + > > > + // SAFETY: The name of the device remains valid while it is alive (because the device is > > > + // never renamed, per the safety requirement of this trait). This is guaranteed to be the > > > + // case because the reference to `self` outlives the one of the returned `CStr` (enforced > > > + // by the compiler because of their lifetimes). > > > + unsafe { CStr::from_char_ptr(name) } > > > > Why can the device never be renamed? Devices are renamed all the time, > > sometimes when you least expect it (i.e. by userspace). So how is this > > considered "safe"? and actually correct? > > > > Again, maybe seeing a real user of this might make more sense, but > > as-is, this feels wrong and not needed at all. > > This requirement is to allow callers to use the string without having > to make a copy of it. > > If subsystems/buses are not following what the C documentation says, > as you point out in another thread, we have a several options: (a) > remove access to names altogether, (b) leave things as they are, then > those subsystems wouldn't be able to honour the safety requirements of > this trait therefore they wouldn't implement it, (c) make a copy of > the string, etc. How about we fix the documentation in the .c code and also drop this as you really don't need it now. Want to send a patch for the driver core code fix? > > > + // owns a reference. This is satisfied by the call to `get_device` above. > > > + Self { ptr } > > > + } > > > + > > > + /// Creates a new device instance from an existing [`RawDevice`] instance. > > > + pub fn from_dev(dev: &dyn RawDevice) -> Self { > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all. > > Different buses will have their own Rust "Device" type, for example, > pci::Device, amba::Device, platform::Device that wrap their C > counterparts pci_dev, amba_device, platform_device. > > "RawDevice" is a trait for functionality that is common to all > devices. It exposes the "struct device" of each bus/subsystem so that > functions that work on any "struct device", for example, `clk_get`, > `pr_info`. will automatically work on all subsystems. Why is this being called "Raw" then? Why not just "Device" to follow along with the naming scheme that the kernel already uses? > For example, as part writing Rust abstractions for a platform devices, > we have a platform::Device type, which is wrapper around `struct > platform_device`. It has a bunch of associated functions that do > things that are specific to the platform bus. But then they also > implement the `RawDevice` trait (by implementing `raw_device` that > returns &pdev->dev), which allows drivers to call `clk_get` and the > printing functions directly. > > Let's say `pdev` is a platform device; if we wanted to call `clk_get` > in C, we'd do something like: > > clk = clk_get(&pdev->dev, NULL); > > In Rust, we'd do: > > clk = pdev.clk_get(None); > > (Note that we didn't have to know that pdev had a field whose type is > a `struct device` that we could use to call clk_get on; `RawDevice` > encoded this information.) > > Does the intent of the abstraction make sense to you now? A bit more, yes. But I want to see some real users before agreeing that it is sane :) thanks, greg k-h
On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Sun, Mar 05, 2023 at 03:39:25AM -0300, Wedson Almeida Filho wrote: > > > > + /// Returns the name of the device. > > > > + fn name(&self) -> &CStr { > > > > + let ptr = self.raw_device(); > > > > + > > > > + // SAFETY: `ptr` is valid because `self` keeps it alive. > > > > + let name = unsafe { bindings::dev_name(ptr) }; > > > > + > > > > + // SAFETY: The name of the device remains valid while it is alive (because the device is > > > > + // never renamed, per the safety requirement of this trait). This is guaranteed to be the > > > > + // case because the reference to `self` outlives the one of the returned `CStr` (enforced > > > > + // by the compiler because of their lifetimes). > > > > + unsafe { CStr::from_char_ptr(name) } > > > > > > Why can the device never be renamed? Devices are renamed all the time, > > > sometimes when you least expect it (i.e. by userspace). So how is this > > > considered "safe"? and actually correct? > > > > > > Again, maybe seeing a real user of this might make more sense, but > > > as-is, this feels wrong and not needed at all. > > > > This requirement is to allow callers to use the string without having > > to make a copy of it. > > > > If subsystems/buses are not following what the C documentation says, > > as you point out in another thread, we have a several options: (a) > > remove access to names altogether, (b) leave things as they are, then > > those subsystems wouldn't be able to honour the safety requirements of > > this trait therefore they wouldn't implement it, (c) make a copy of > > the string, etc. > > How about we fix the documentation in the .c code and also drop this as > you really don't need it now. > > Want to send a patch for the driver core code fix? Sure, will do. > > > > + // owns a reference. This is satisfied by the call to `get_device` above. > > > > + Self { ptr } > > > > + } > > > > + > > > > + /// Creates a new device instance from an existing [`RawDevice`] instance. > > > > + pub fn from_dev(dev: &dyn RawDevice) -> Self { > > > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all. > > > > Different buses will have their own Rust "Device" type, for example, > > pci::Device, amba::Device, platform::Device that wrap their C > > counterparts pci_dev, amba_device, platform_device. > > > > "RawDevice" is a trait for functionality that is common to all > > devices. It exposes the "struct device" of each bus/subsystem so that > > functions that work on any "struct device", for example, `clk_get`, > > `pr_info`. will automatically work on all subsystems. > > Why is this being called "Raw" then? Why not just "Device" to follow > along with the naming scheme that the kernel already uses? Because it gives us access to underlying raw `struct device` pointer, in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not married to the name though, we should probably look for a better one if this one is confusing. Just "Device" is already taken. It's a ref-counted `struct device` (it calls get_device/put_device in the right places automatically, guarantees no dandling pointers); it is meant to be used by code that needs to hold on to devices when they don't care about the bus. (It in fact implements `RawDevice`.) How about `IsDevice`? Then, for example, the platform bus would implement `IsDevice` for `plaform::Device`. > > For example, as part writing Rust abstractions for a platform devices, > > we have a platform::Device type, which is wrapper around `struct > > platform_device`. It has a bunch of associated functions that do > > things that are specific to the platform bus. But then they also > > implement the `RawDevice` trait (by implementing `raw_device` that > > returns &pdev->dev), which allows drivers to call `clk_get` and the > > printing functions directly. > > > > Let's say `pdev` is a platform device; if we wanted to call `clk_get` > > in C, we'd do something like: > > > > clk = clk_get(&pdev->dev, NULL); > > > > In Rust, we'd do: > > > > clk = pdev.clk_get(None); > > > > (Note that we didn't have to know that pdev had a field whose type is > > a `struct device` that we could use to call clk_get on; `RawDevice` > > encoded this information.) > > > > Does the intent of the abstraction make sense to you now? > > A bit more, yes. But I want to see some real users before agreeing that > it is sane :) Fair enough. Cheers, -Wedson
On Thu, Mar 09, 2023 at 01:46:39PM -0300, Wedson Almeida Filho wrote: > On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > > + // owns a reference. This is satisfied by the call to `get_device` above. > > > > > + Self { ptr } > > > > > + } > > > > > + > > > > > + /// Creates a new device instance from an existing [`RawDevice`] instance. > > > > > + pub fn from_dev(dev: &dyn RawDevice) -> Self { > > > > > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all. > > > > > > Different buses will have their own Rust "Device" type, for example, > > > pci::Device, amba::Device, platform::Device that wrap their C > > > counterparts pci_dev, amba_device, platform_device. > > > > > > "RawDevice" is a trait for functionality that is common to all > > > devices. It exposes the "struct device" of each bus/subsystem so that > > > functions that work on any "struct device", for example, `clk_get`, > > > `pr_info`. will automatically work on all subsystems. > > > > Why is this being called "Raw" then? Why not just "Device" to follow > > along with the naming scheme that the kernel already uses? > > Because it gives us access to underlying raw `struct device` pointer, > in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not > married to the name though, we should probably look for a better one > if this one is confusing. > > Just "Device" is already taken. It's a ref-counted `struct device` (it > calls get_device/put_device in the right places automatically, > guarantees no dandling pointers); it is meant to be used by code that > needs to hold on to devices when they don't care about the bus. (It in > fact implements `RawDevice`.) I don't understand, why do you need both of these? Why can't one just do? Why would you need one without the other? I would think that "Device" and "RawDevice" here would be the same thing, that is a way to refer to a "larger" underlying struct device memory chunk in a way that can be passed around without knowing, or caring, what the "real" device type is. > How about `IsDevice`? That sounds like a question, and would return a boolean, not a structure :) > Then, for example, the platform bus would implement `IsDevice` for > `plaform::Device`. I don't really understand that, sorry. thanks, greg k-h
On Thu, 9 Mar 2023 at 14:11, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Mar 09, 2023 at 01:46:39PM -0300, Wedson Almeida Filho wrote: > > On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > + // owns a reference. This is satisfied by the call to `get_device` above. > > > > > > + Self { ptr } > > > > > > + } > > > > > > + > > > > > > + /// Creates a new device instance from an existing [`RawDevice`] instance. > > > > > > + pub fn from_dev(dev: &dyn RawDevice) -> Self { > > > > > > > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all. > > > > > > > > Different buses will have their own Rust "Device" type, for example, > > > > pci::Device, amba::Device, platform::Device that wrap their C > > > > counterparts pci_dev, amba_device, platform_device. > > > > > > > > "RawDevice" is a trait for functionality that is common to all > > > > devices. It exposes the "struct device" of each bus/subsystem so that > > > > functions that work on any "struct device", for example, `clk_get`, > > > > `pr_info`. will automatically work on all subsystems. > > > > > > Why is this being called "Raw" then? Why not just "Device" to follow > > > along with the naming scheme that the kernel already uses? > > > > Because it gives us access to underlying raw `struct device` pointer, > > in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not > > married to the name though, we should probably look for a better one > > if this one is confusing. > > > > Just "Device" is already taken. It's a ref-counted `struct device` (it > > calls get_device/put_device in the right places automatically, > > guarantees no dandling pointers); it is meant to be used by code that > > needs to hold on to devices when they don't care about the bus. (It in > > fact implements `RawDevice`.) > > I don't understand, why do you need both of these? Why can't one just > do? Why would you need one without the other? I would think that > "Device" and "RawDevice" here would be the same thing, that is a way to > refer to a "larger" underlying struct device memory chunk in a way that > can be passed around without knowing, or caring, what the "real" device > type is. `Device` is a struct, it is the Rust abstraction for C's `struct device`. Let's use the platform bus as our running example: we have `platform::Device` as the Rust abstraction for C's `struct platform_device`. Let's use `clk_get`as our running example of a function that takes a `struct device` as argument. If we have a platform device, we can't just call `clk_get` because the types don't match. In C, we access the `dev` field of `struct platform_device` before we call `clk_get` (i.e., we call clk_get(&pdev->dev, ...)), but in Rust we don't want to make the fields of `platform::Device` public, especially because they're fields of a C struct. So as part of `platform::Device` we'd have to implement something like: impl platform::Device { fn get_device(&self) -> &Device { ... } } Then calling `clk_get` would be something like: pdev.get_device().clk_get(...) The problem is that `clk_get` doesn't know that `platform::Device` is a device, that's why we need this `get_device()` call on each bus abstraction of a device, plus on each call to bus-agnostic device functions. Since we're implementing this "adapter" function anyway, we may as well put in a _trait_ and improve how people use it. We say: every struct that is a device should implement the `RawDevice` (or `IsDevice`, or whatever we decide to call it) trait. Then `platform::Device` would still have to implement something like: impl RawDevice for platform::Device { fn get_device(&self) -> &Device { ... } } (Note that we went from `impl X` to `impl RawDevice for X`.) With this trait, users can call `clk_get` on a platform device as follows: pdev.clk_get(...) So we improve the experience of driver developers by having bus abstraction developers add "RawDevice for" to their impl block of the `get_device` function for their device. > > How about `IsDevice`? > > That sounds like a question, and would return a boolean, not a structure :) `RawDevice` is not a struct, it's a trait that is implemented by bus-specific structs. > > Then, for example, the platform bus would implement `IsDevice` for > > `plaform::Device`. > > I don't really understand that, sorry. I tried to explain it above. I think the source of the problem is the distinction between `struct` and `trait`, the latter of course doesn't exist in C. Cheers, -Wedson
On Thu, Mar 9, 2023 at 6:11 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > I don't understand, why do you need both of these? Why can't one just > do? Why would you need one without the other? I would think that > "Device" and "RawDevice" here would be the same thing, that is a way to > refer to a "larger" underlying struct device memory chunk in a way that > can be passed around without knowing, or caring, what the "real" device > type is. > > That sounds like a question, and would return a boolean, not a structure :) > > I don't really understand that, sorry. To complement what Wedson wrote, perhaps this helps a bit, since there are a few `*Device` things around (I will prefix things with the module they are in for clarity): - `device::RawDevice` is a trait, not a struct. This means it is just an interface/property/tag/feature. Then types (like structs) can say "I support/provide/implement this trait". In this case, it requires those types implementing `device::RawDevice` to provide a suitable `raw_device()` method. - Then, you have `device::Device`. This is an actual struct, containing a `struct device *`. It implements `device::RawDevice`, by returning the pointer. - Then there are others like `platform::Device` or `amba::Device`. They are also structs, containing e.g. `struct platform_device *` or anything they may need. They implement `device::RawDevice` too if it makes sense, say, by returning a pointer to the `->dev` in their C struct. So you have two different kinds of entities here. One is the trait, and the others are structs that promise to implement that trait correctly, regardless of how each struct manages to do it. The trait can also then provide extra default functionality for those that have implemented it. For instance, the `clk_get()` method Wedson mentioned can be defined in the trait, and in its body we can call the `raw_device()` to do so. `raw_device()` is available because we are inside the trait, even if each struct implementing the trait provides a different implementation. Traits are sometimes named after the "main" method they contain, e.g. `ToString` may be a trait with a `to_string()` method. Here `device::RawDevice` has a `raw_device()` method, so it makes sense in that way. Wedson was suggesting `device::IsDevice` as an alternative, because `device::Device` is taken for the ref-counted device. Of course, other arrangements of names could be possible. `IsDevice` uses Pascal case, so by convention it would be fairly clear that it would not be a function returning a boolean. But one may expect that it is a trait that implements a `is_device()` method, though. (And if that was the case, which it isn't, one could then indeed expect that such a method would return the boolean you expected). Cheers, Miguel
On Thu, 9 Mar 2023 16:06:06 -0300 Wedson Almeida Filho <wedsonaf@gmail.com> wrote: > On Thu, 9 Mar 2023 at 14:11, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Mar 09, 2023 at 01:46:39PM -0300, Wedson Almeida Filho wrote: > > > On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > + // owns a reference. This is satisfied by the call to `get_device` above. > > > > > > > + Self { ptr } > > > > > > > + } > > > > > > > + > > > > > > > + /// Creates a new device instance from an existing [`RawDevice`] instance. > > > > > > > + pub fn from_dev(dev: &dyn RawDevice) -> Self { > > > > > > > > > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all. > > > > > > > > > > Different buses will have their own Rust "Device" type, for example, > > > > > pci::Device, amba::Device, platform::Device that wrap their C > > > > > counterparts pci_dev, amba_device, platform_device. > > > > > > > > > > "RawDevice" is a trait for functionality that is common to all > > > > > devices. It exposes the "struct device" of each bus/subsystem so that > > > > > functions that work on any "struct device", for example, `clk_get`, > > > > > `pr_info`. will automatically work on all subsystems. > > > > > > > > Why is this being called "Raw" then? Why not just "Device" to follow > > > > along with the naming scheme that the kernel already uses? > > > > > > Because it gives us access to underlying raw `struct device` pointer, > > > in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not > > > married to the name though, we should probably look for a better one > > > if this one is confusing. > > > > > > Just "Device" is already taken. It's a ref-counted `struct device` (it > > > calls get_device/put_device in the right places automatically, > > > guarantees no dandling pointers); it is meant to be used by code that > > > needs to hold on to devices when they don't care about the bus. (It in > > > fact implements `RawDevice`.) > > > > I don't understand, why do you need both of these? Why can't one just > > do? Why would you need one without the other? I would think that > > "Device" and "RawDevice" here would be the same thing, that is a way to > > refer to a "larger" underlying struct device memory chunk in a way that > > can be passed around without knowing, or caring, what the "real" device > > type is. > > `Device` is a struct, it is the Rust abstraction for C's `struct device`. > > Let's use the platform bus as our running example: we have > `platform::Device` as the Rust abstraction for C's `struct > platform_device`. > > Let's use `clk_get`as our running example of a function that takes a > `struct device` as argument. > > If we have a platform device, we can't just call `clk_get` because the > types don't match. In C, we access the `dev` field of `struct > platform_device` before we call `clk_get` (i.e., we call > clk_get(&pdev->dev, ...)), but in Rust we don't want to make the > fields of `platform::Device` public, especially because they're fields > of a C struct. So as part of `platform::Device` we'd have to implement > something like: > > impl platform::Device { > fn get_device(&self) -> &Device { > ... > } > } > > Then calling `clk_get` would be something like: > > pdev.get_device().clk_get(...) Just thinking out loud here, would it work if `platform::Device` implements `Deref<Target = Device>`? Best, Gary
On Fri, 24 Feb 2023 19:53:17 +0900 Asahi Lina <lina@asahilina.net> wrote: > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > Add a Device type which represents an owned reference to a generic > struct device. This minimal implementation just handles reference > counting and allows the user to get the device name. > > Lina: Rewrote commit message, dropped the Amba bits, and squashed in > simple changes to the core Device code from latter commits in > rust-for-linux/rust. Also include the rust_helper_dev_get_drvdata > helper which will be needed by consumers later on anyway. > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Asahi Lina <lina@asahilina.net> > --- > rust/helpers.c | 13 +++++++++ > rust/kernel/device.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/rust/helpers.c b/rust/helpers.c > index 04b9be46e887..54954fd80c77 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -20,6 +20,7 @@ > > #include <linux/bug.h> > #include <linux/build_bug.h> > +#include <linux/device.h> > #include <linux/err.h> > #include <linux/refcount.h> > > @@ -65,6 +66,18 @@ long rust_helper_PTR_ERR(__force const void *ptr) > } > EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR); > > +void *rust_helper_dev_get_drvdata(struct device *dev) > +{ > + return dev_get_drvdata(dev); > +} > +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata); > + > +const char *rust_helper_dev_name(const struct device *dev) > +{ > + return dev_name(dev); > +} > +EXPORT_SYMBOL_GPL(rust_helper_dev_name); > + > /* > * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type > * as the Rust `usize` type, so we can use it in contexts where Rust > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > index 9be021e393ca..e57da622d817 100644 > --- a/rust/kernel/device.rs > +++ b/rust/kernel/device.rs > @@ -4,7 +4,7 @@ > //! > //! C header: [`include/linux/device.h`](../../../../include/linux/device.h) > > -use crate::bindings; > +use crate::{bindings, str::CStr}; > > /// A raw device. > /// > @@ -20,4 +20,78 @@ use crate::bindings; > pub unsafe trait RawDevice { > /// Returns the raw `struct device` related to `self`. > fn raw_device(&self) -> *mut bindings::device; > + > + /// Returns the name of the device. > + fn name(&self) -> &CStr { > + let ptr = self.raw_device(); > + > + // SAFETY: `ptr` is valid because `self` keeps it alive. > + let name = unsafe { bindings::dev_name(ptr) }; > + > + // SAFETY: The name of the device remains valid while it is alive (because the device is > + // never renamed, per the safety requirement of this trait). This is guaranteed to be the > + // case because the reference to `self` outlives the one of the returned `CStr` (enforced > + // by the compiler because of their lifetimes). > + unsafe { CStr::from_char_ptr(name) } > + } > +} > + > +/// A ref-counted device. > +/// > +/// # Invariants > +/// > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by > +/// `self`, and will be decremented when `self` is dropped. > +pub struct Device { > + pub(crate) ptr: *mut bindings::device, > +} > + Shouldn't this be #[repr(transparent)] pub struct Device(Opaque<bindings::device>); ? Best, Gary
On Mon, Mar 13, 2023 at 05:52:02PM +0000, Gary Guo wrote: [...] > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > > index 9be021e393ca..e57da622d817 100644 > > --- a/rust/kernel/device.rs > > +++ b/rust/kernel/device.rs > > @@ -4,7 +4,7 @@ > > //! > > //! C header: [`include/linux/device.h`](../../../../include/linux/device.h) > > > > -use crate::bindings; > > +use crate::{bindings, str::CStr}; > > > > /// A raw device. > > /// > > @@ -20,4 +20,78 @@ use crate::bindings; > > pub unsafe trait RawDevice { > > /// Returns the raw `struct device` related to `self`. > > fn raw_device(&self) -> *mut bindings::device; > > + > > + /// Returns the name of the device. > > + fn name(&self) -> &CStr { > > + let ptr = self.raw_device(); > > + > > + // SAFETY: `ptr` is valid because `self` keeps it alive. > > + let name = unsafe { bindings::dev_name(ptr) }; > > + > > + // SAFETY: The name of the device remains valid while it is alive (because the device is > > + // never renamed, per the safety requirement of this trait). This is guaranteed to be the > > + // case because the reference to `self` outlives the one of the returned `CStr` (enforced > > + // by the compiler because of their lifetimes). > > + unsafe { CStr::from_char_ptr(name) } > > + } > > +} > > + > > +/// A ref-counted device. > > +/// > > +/// # Invariants > > +/// > > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by > > +/// `self`, and will be decremented when `self` is dropped. > > +pub struct Device { > > + pub(crate) ptr: *mut bindings::device, > > +} > > + > > Shouldn't this be > > #[repr(transparent)] > pub struct Device(Opaque<bindings::device>); > > ? I have the same feeling, for `task_struct`, we have #[repr(transparent)] pub struct Task(pub(crate) UnsafeCell<bindings::task_struct>); and pub struct TaskRef<'a> { task: &'a Task, _not_send: PhantomData<*mut ()>, } I wonder whether we should do the similar for `Device`, meaning `Device` is just a tranparent representation for `struct device` and another type (say, `DeviceRef`) is introduced as the reference type, or we can just use `ARef`. Regards, Boqun > > Best, > Gary
diff --git a/rust/helpers.c b/rust/helpers.c index 04b9be46e887..54954fd80c77 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -20,6 +20,7 @@ #include <linux/bug.h> #include <linux/build_bug.h> +#include <linux/device.h> #include <linux/err.h> #include <linux/refcount.h> @@ -65,6 +66,18 @@ long rust_helper_PTR_ERR(__force const void *ptr) } EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR); +void *rust_helper_dev_get_drvdata(struct device *dev) +{ + return dev_get_drvdata(dev); +} +EXPORT_SYMBOL_GPL(rust_helper_dev_get_drvdata); + +const char *rust_helper_dev_name(const struct device *dev) +{ + return dev_name(dev); +} +EXPORT_SYMBOL_GPL(rust_helper_dev_name); + /* * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type * as the Rust `usize` type, so we can use it in contexts where Rust diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index 9be021e393ca..e57da622d817 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -4,7 +4,7 @@ //! //! C header: [`include/linux/device.h`](../../../../include/linux/device.h) -use crate::bindings; +use crate::{bindings, str::CStr}; /// A raw device. /// @@ -20,4 +20,78 @@ use crate::bindings; pub unsafe trait RawDevice { /// Returns the raw `struct device` related to `self`. fn raw_device(&self) -> *mut bindings::device; + + /// Returns the name of the device. + fn name(&self) -> &CStr { + let ptr = self.raw_device(); + + // SAFETY: `ptr` is valid because `self` keeps it alive. + let name = unsafe { bindings::dev_name(ptr) }; + + // SAFETY: The name of the device remains valid while it is alive (because the device is + // never renamed, per the safety requirement of this trait). This is guaranteed to be the + // case because the reference to `self` outlives the one of the returned `CStr` (enforced + // by the compiler because of their lifetimes). + unsafe { CStr::from_char_ptr(name) } + } +} + +/// A ref-counted device. +/// +/// # Invariants +/// +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by +/// `self`, and will be decremented when `self` is dropped. +pub struct Device { + pub(crate) ptr: *mut bindings::device, +} + +// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread. +unsafe impl Send for Device {} + +// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used +// from any thread. +unsafe impl Sync for Device {} + +impl Device { + /// Creates a new device instance. + /// + /// # Safety + /// + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. + pub unsafe fn new(ptr: *mut bindings::device) -> Self { + // SAFETY: By the safety requirements, ptr is valid and its refcounted will be incremented. + unsafe { bindings::get_device(ptr) }; + // INVARIANT: The safety requirements satisfy all but one invariant, which is that `self` + // owns a reference. This is satisfied by the call to `get_device` above. + Self { ptr } + } + + /// Creates a new device instance from an existing [`RawDevice`] instance. + pub fn from_dev(dev: &dyn RawDevice) -> Self { + // SAFETY: The requirements are satisfied by the existence of `RawDevice` and its safety + // requirements. + unsafe { Self::new(dev.raw_device()) } + } +} + +// SAFETY: The device returned by `raw_device` is the one for which we hold a reference. +unsafe impl RawDevice for Device { + fn raw_device(&self) -> *mut bindings::device { + self.ptr + } +} + +impl Drop for Device { + fn drop(&mut self) { + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // relinquish it now. + unsafe { bindings::put_device(self.ptr) }; + } +} + +impl Clone for Device { + fn clone(&self) -> Self { + Device::from_dev(self) + } }