Message ID | 20230131130841.318301-1-yakoyoku@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp2740956wrn; Tue, 31 Jan 2023 05:09:44 -0800 (PST) X-Google-Smtp-Source: AK7set9+H2NBBk1UnFcGVYHPBcA6qkV2S8l4RHzfFVIuBNsBein2IryS/xWQLuH4kIM15+LzeipA X-Received: by 2002:a17:906:ad8d:b0:7c0:be5d:59a9 with SMTP id la13-20020a170906ad8d00b007c0be5d59a9mr3028454ejb.20.1675170584597; Tue, 31 Jan 2023 05:09:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1675170584; cv=none; d=google.com; s=arc-20160816; b=rwd7YkiN7X6IQvDHg33Nmu6om1h0voKRMOSVZLpAPAANAvfX1HhKgcI2L3BMa0ZJ0e cvRGw3sYdglzGess030bbj1FXm7NJh+2lmDA8VSsydY9vggWuIigOXkgiNHe3Mj0cSju wembhqDb57LR00kpq8CHSeL2kwkIEn7ZgwARwP0U0iQejgemk4LTFvKLeUcpxAkAuR8/ 2XFUdTIvxYmp3q6LgaMoTsnkVb+Na+YcSEjq+UYcGfENxfYs61SwSII68iebGRL8YrzZ 6Ja1PTc6DoTe/5lvP/2LbOvKoEjUf1DsL7VyySA/5ccvrPS6CiL2gAA89T0WhGE/+OD6 M23w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=5vxGDcuE5ADlwFzUbFDPZd16x22UTWZptpHGJiMgcac=; b=ck+/CmpeQtEjvVdDm86zFf6J6gT+ifuf20uwYAgvAZEgBra+/G2hTGF606DQR+dxRq zY9ayX3aw7ARwsQYlWp0Ua18HSOjQDEXe54Mb8R00pQecFsMCb5jjq+M2jbS1zdoeRnK W1KkoAiQegmUliAJXUcFPHMEfqfecQsI/yBvVoPyN7lqEp9NCTiJgEL+mse1fv7gKPVu PhtgGDl/00RfjSWtHhBH9trufv/+8ATR9Hh23iBuPdV//t2BMGDPRsaCyUkVkwHJD/W1 VnGrVXm6kZmSL89aiqT0bVZc/eiFtCZTjh6kPQBEtMMCQ5uuDiivPT0z8+VPCLbXTCNf FKNA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=GGhSlkCq; 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=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j24-20020a170906105800b008788321c866si17911918ejj.864.2023.01.31.05.09.20; Tue, 31 Jan 2023 05:09:44 -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=@gmail.com header.s=20210112 header.b=GGhSlkCq; 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=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231996AbjAaNI5 (ORCPT <rfc822;maxin.john@gmail.com> + 99 others); Tue, 31 Jan 2023 08:08:57 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44436 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231473AbjAaNI4 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 31 Jan 2023 08:08:56 -0500 Received: from mail-oa1-x29.google.com (mail-oa1-x29.google.com [IPv6:2001:4860:4864:20::29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 962138A73; Tue, 31 Jan 2023 05:08:49 -0800 (PST) Received: by mail-oa1-x29.google.com with SMTP id 586e51a60fabf-142b72a728fso19195859fac.9; Tue, 31 Jan 2023 05:08:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=5vxGDcuE5ADlwFzUbFDPZd16x22UTWZptpHGJiMgcac=; b=GGhSlkCqG9nBEEeAex6K3a7EAFyUp4RwNFkYdvohfBEglz0IMFuA4wsRJ0mgiXIQcV nE24AmBFn71YXufi6C16QS4Fws0FKEFNvxKK7H/xJjUXfhA5epOfsPPY2MBMDLbmVkq8 5lmgHx35n38FeiPAxo4UC1aGpqbSjZk36hJrx7gt01BY0vizqG4ypXEDGQO22QFSGpD1 35M/onbqoNIzo5ipXjeiwJby4xL1+wXcD75KhybgVIGHu2YPYlaHSKamlCejqKZvF5F3 HVYupofiLDWB77fbQ1Z3K7scA6ArjCaheCTYgxZmiZK6q4qiPo3fyZEaSaEaGRMx/Fhz A87w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=5vxGDcuE5ADlwFzUbFDPZd16x22UTWZptpHGJiMgcac=; b=xhaxseqJWL9soWKpfAVXc4fOfbw9sDwR/X8xms1MZzFbtz4jU+Ie4ijttsU6deUjn5 5m94XI1HOHnJVXz/n6y9mXXgecnjnIzyiIp9jp6XNVBMTiqStI/ihmHt3m+cAEtcMGAC HDZ7P9ebKUcaoLb4XnPPj3txv0OC0V/LAPOIh0XPub5aU0OxwwMjDMqodQ6jspgJSRgw Il9GwLz4pziFc1VtdgzfktnzE2idiXe7uvPC27hfCREOPj7kkjl94SPgz/npBvXOtaof BZ95temojfStVvHAYcLFcYmM00adI/00uQTXiDNL95ZXXXKeOcZDMMbm+3cwccO2D9IK VKbg== X-Gm-Message-State: AO0yUKW05+xLEcSqoTSNOofgjVTyFMD4+0K0DGSMkjv2388FVkRJohv5 fYeUSZm0zIFbsTf1w3GpXGuZ504/NuM= X-Received: by 2002:a05:6870:6593:b0:163:4004:c7de with SMTP id fp19-20020a056870659300b001634004c7demr12835858oab.36.1675170528642; Tue, 31 Jan 2023 05:08:48 -0800 (PST) Received: from tx3000mach.io (static.220.238.itcsa.net. [190.15.220.238]) by smtp.gmail.com with ESMTPSA id g5-20020a05687054c500b0014866eb34cesm6441483oan.48.2023.01.31.05.08.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Jan 2023 05:08:48 -0800 (PST) From: Martin Rodriguez Reboredo <yakoyoku@gmail.com> To: linux-kernel@vger.kernel.org Cc: rust-for-linux@vger.kernel.org, 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> Subject: [PATCH] rust: add this_module macro Date: Tue, 31 Jan 2023 10:08:41 -0300 Message-Id: <20230131130841.318301-1-yakoyoku@gmail.com> X-Mailer: git-send-email 2.39.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,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?1756543670769201739?= X-GMAIL-MSGID: =?utf-8?q?1756543670769201739?= |
Series |
rust: add this_module macro
|
|
Commit Message
Martin Rodriguez Reboredo
Jan. 31, 2023, 1:08 p.m. UTC
Adds a Rust equivalent to the handy THIS_MODULE macro from C.
Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
---
rust/kernel/lib.rs | 12 ++++++++++++
1 file changed, 12 insertions(+)
Comments
On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote: > Adds a Rust equivalent to the handy THIS_MODULE macro from C. > > Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > --- > rust/kernel/lib.rs | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index e0b0e953907d..afb6b0390426 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -80,6 +80,18 @@ impl ThisModule { > } > } > > +/// Returns the current module. > +#[macro_export] > +macro_rules! this_module { > + () => { > + if cfg!(MODULE) { > + Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) }) > + } else { > + None > + } > + }; > +} While this is handy, what exactly will it be used for? The C wrappers/shim/whatever should probably handle this for you already when you save this pointer into a structure right? Surely you aren't trying to increment your own module's reference count, right? That just doesn't work :) thanks, greg k-h
On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote: >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote: >> Adds a Rust equivalent to the handy THIS_MODULE macro from C. >> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> >> --- >> rust/kernel/lib.rs | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >> index e0b0e953907d..afb6b0390426 100644 >> --- a/rust/kernel/lib.rs >> +++ b/rust/kernel/lib.rs >> @@ -80,6 +80,18 @@ impl ThisModule { >> } >> } >> >> +/// Returns the current module. >> +#[macro_export] >> +macro_rules! this_module { >> + () => { >> + if cfg!(MODULE) { >> + Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) }) >> + } else { >> + None >> + } >> + }; >> +} > >While this is handy, what exactly will it be used for? The C >wrappers/shim/whatever should probably handle this for you already when >you save this pointer into a structure right? > >Surely you aren't trying to increment your own module's reference count, >right? That just doesn't work :) > >thanks, > >greg k-h This was meant for setting the owner field of a file_operations struct or the cra_owner field of crypto_alg and many other structs. I know that increfing a module without a good reason is dead dumb, so I'm not trying to send things in a downwards spiral. @@@ And yes, I should have mentioned that in the commit message, but I let slip that detail.
On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote: > On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote: > >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote: > >> Adds a Rust equivalent to the handy THIS_MODULE macro from C. > >> > >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > >> --- > >> rust/kernel/lib.rs | 12 ++++++++++++ > >> 1 file changed, 12 insertions(+) > >> > >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > >> index e0b0e953907d..afb6b0390426 100644 > >> --- a/rust/kernel/lib.rs > >> +++ b/rust/kernel/lib.rs > >> @@ -80,6 +80,18 @@ impl ThisModule { > >> } > >> } > >> > >> +/// Returns the current module. > >> +#[macro_export] > >> +macro_rules! this_module { > >> + () => { > >> + if cfg!(MODULE) { > >> + Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) }) > >> + } else { > >> + None > >> + } > >> + }; > >> +} > > > >While this is handy, what exactly will it be used for? The C > >wrappers/shim/whatever should probably handle this for you already when > >you save this pointer into a structure right? > > > >Surely you aren't trying to increment your own module's reference count, > >right? That just doesn't work :) > > > >thanks, > > > >greg k-h > > This was meant for setting the owner field of a file_operations struct > or the cra_owner field of crypto_alg and many other structs. But shouldn't the macro kernel::declare_file_operations() do this for you automagically? You should never have to manually say "this module!" to any structure or function call if we do things right. Yes, many "old school" structures in the kernel do this, but we have learned from the 1990's, see the fun wrappers around simple things like usb_register_driver(); as an example of how the driver author themselves should never see a module pointer anywhere. > I know that increfing a module without a good reason is dead dumb, so > I'm not trying to send things in a downwards spiral. @@@ That's good, but let's not add housekeeping requirements when we do not have to do so if at all possible please. thanks, greg k-h
On Tue, Jan 31, 2023 at 04:15:51PM +0100, Greg KH wrote: >On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote: >> On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote: >> >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote: >> >> Adds a Rust equivalent to the handy THIS_MODULE macro from C. >> >> >> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> >> >> --- >> >> rust/kernel/lib.rs | 12 ++++++++++++ >> >> 1 file changed, 12 insertions(+) >> >> >> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >> >> index e0b0e953907d..afb6b0390426 100644 >> >> --- a/rust/kernel/lib.rs >> >> +++ b/rust/kernel/lib.rs >> >> @@ -80,6 +80,18 @@ impl ThisModule { >> >> } >> >> } >> >> >> >> +/// Returns the current module. >> >> +#[macro_export] >> >> +macro_rules! this_module { >> >> + () => { >> >> + if cfg!(MODULE) { >> >> + Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) }) >> >> + } else { >> >> + None >> >> + } >> >> + }; >> >> +} >> > >> >While this is handy, what exactly will it be used for? The C >> >wrappers/shim/whatever should probably handle this for you already when >> >you save this pointer into a structure right? >> > >> >Surely you aren't trying to increment your own module's reference count, >> >right? That just doesn't work :) >> > >> >thanks, >> > >> >greg k-h >> >> This was meant for setting the owner field of a file_operations struct >> or the cra_owner field of crypto_alg and many other structs. > >But shouldn't the macro kernel::declare_file_operations() do this for >you automagically? You should never have to manually say "this module!" >to any structure or function call if we do things right. > >Yes, many "old school" structures in the kernel do this, but we have >learned from the 1990's, see the fun wrappers around simple things like >usb_register_driver(); as an example of how the driver author themselves >should never see a module pointer anywhere. > >> I know that increfing a module without a good reason is dead dumb, so >> I'm not trying to send things in a downwards spiral. @@@ > >That's good, but let's not add housekeeping requirements when we do not >have to do so if at all possible please. > >thanks, > >greg k-h *kicks can*, at least I can take some ideas out of this, anyways, thanks for your reviews.
On Tue, Jan 31, 2023 at 01:07:28PM -0300, Martin Rodriguez Reboredo wrote: > On Tue, Jan 31, 2023 at 04:15:51PM +0100, Greg KH wrote: > >On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote: > >> On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote: > >> >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote: > >> >> Adds a Rust equivalent to the handy THIS_MODULE macro from C. > >> >> > >> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > >> >> --- > >> >> rust/kernel/lib.rs | 12 ++++++++++++ > >> >> 1 file changed, 12 insertions(+) > >> >> > >> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > >> >> index e0b0e953907d..afb6b0390426 100644 > >> >> --- a/rust/kernel/lib.rs > >> >> +++ b/rust/kernel/lib.rs > >> >> @@ -80,6 +80,18 @@ impl ThisModule { > >> >> } > >> >> } > >> >> > >> >> +/// Returns the current module. > >> >> +#[macro_export] > >> >> +macro_rules! this_module { > >> >> + () => { > >> >> + if cfg!(MODULE) { > >> >> + Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) }) > >> >> + } else { > >> >> + None > >> >> + } > >> >> + }; > >> >> +} > >> > > >> >While this is handy, what exactly will it be used for? The C > >> >wrappers/shim/whatever should probably handle this for you already when > >> >you save this pointer into a structure right? > >> > > >> >Surely you aren't trying to increment your own module's reference count, > >> >right? That just doesn't work :) > >> > > >> >thanks, > >> > > >> >greg k-h > >> > >> This was meant for setting the owner field of a file_operations struct > >> or the cra_owner field of crypto_alg and many other structs. > > > >But shouldn't the macro kernel::declare_file_operations() do this for > >you automagically? You should never have to manually say "this module!" > >to any structure or function call if we do things right. > > > >Yes, many "old school" structures in the kernel do this, but we have > >learned from the 1990's, see the fun wrappers around simple things like > >usb_register_driver(); as an example of how the driver author themselves > >should never see a module pointer anywhere. > > > >> I know that increfing a module without a good reason is dead dumb, so > >> I'm not trying to send things in a downwards spiral. @@@ > > > >That's good, but let's not add housekeeping requirements when we do not > >have to do so if at all possible please. > > > >thanks, > > > >greg k-h > > *kicks can*, at least I can take some ideas out of this, anyways, thanks > for your reviews. I don't mean to reject this out-of-hand, it's just that without a real user, it's impossible to review this and say "this is ok" instead of "perhaps you should do it this other way"? Right now the rust framework is just that, a framework. Perhaps we should not be adding anything else to it until there is a real user of it? Otherwise this will keep coming up again and again. Treat this like any other kernel feature/addition, you can't add apis until you submit a user for it at the same time. That's one way we have been able to evolve and maintain the kernel source tree for so long. Without an api user, we have no way to know how it's being used or if it's even being used at all. thanks, greg k-h
On Tue, Jan 31, 2023 at 5:59 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > Right now the rust framework is just that, a framework. Perhaps we > should not be adding anything else to it until there is a real user of > it? Otherwise this will keep coming up again and again. > > Treat this like any other kernel feature/addition, you can't add apis > until you submit a user for it at the same time. That's one way we have > been able to evolve and maintain the kernel source tree for so long. > Without an api user, we have no way to know how it's being used or if > it's even being used at all. Agreed. For this patch, it came independently, so I cannot speak about its users. For other patch series we have sent, the users are out-of-tree, but they want to come in-tree as soon as possible. We are coordinating with them to prioritize the submission of the things they will depend on. Since the goal is to submit things piece by piece so that they can be properly reviewed, what we have been doing to ameliorate things is to provide enough details, examples and documentation for each function, type, etc. so that it is hopefully clear how they will be used. If there are some cases where it may not be clear, we can also link to the upcoming users. Cheers, Miguel
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e0b0e953907d..afb6b0390426 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -80,6 +80,18 @@ impl ThisModule { } } +/// Returns the current module. +#[macro_export] +macro_rules! this_module { + () => { + if cfg!(MODULE) { + Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) }) + } else { + None + } + }; +} + #[cfg(not(any(testlib, test)))] #[panic_handler] fn panic(info: &core::panic::PanicInfo<'_>) -> ! {