[PATCH V6 0/2] author field in module! macro should be a array

[PATCH V6 0/2] author field in module! macro should be a array

[Guilherme Giacomo Simoes]

In the module! macro, the author field is currently of type String.
Since modules can have multiple authors, this limitation prevents
specifying more than one.

- Change the author field to Option<Vec<String>> to allow creating
  modules with multiple authors.

- rename the field from author to authors to make it explicit that it
  can refer to multiple authors.

- In modules that use the author field, update its value to an array of
  strings, and also rename it from author to authors.

- Change the checkpatch.pl to find poorly formatted arrays in the macro
  module!

---
v6 changes
- Use consistent style in warn messages
- Improved the commit title for checkpatch changes
---

Guilherme Giacomo Simoes (2):
  rust: module: change author to an array
  checkpatch: check format of Vec<String> in modules

 drivers/block/rnull.rs               |  2 +-
 drivers/net/phy/ax88796b_rust.rs     |  2 +-
 drivers/net/phy/qt2025.rs            |  2 +-
 rust/kernel/net/phy.rs               |  4 +--
 rust/kernel/pci.rs                   |  2 +-
 rust/kernel/platform.rs              |  2 +-
 rust/macros/lib.rs                   |  6 ++--
 rust/macros/module.rs                | 12 ++++----
 samples/rust/rust_driver_faux.rs     |  2 +-
 samples/rust/rust_driver_pci.rs      |  2 +-
 samples/rust/rust_driver_platform.rs |  2 +-
 samples/rust/rust_minimal.rs         |  2 +-
 samples/rust/rust_misc_device.rs     |  2 +-
 samples/rust/rust_print_main.rs      |  2 +-
 scripts/checkpatch.pl                | 43 ++++++++++++++++++++++++++++
 15 files changed, 66 insertions(+), 21 deletions(-)

-- 
2.34.1

[PATCH V6 1/2] rust: module: change author to an array

[Guilherme Giacomo Simoes]

In the module! macro, the author field is currently of type String.
Since modules can have multiple authors, this limitation prevents
specifying more than one.
Change the author field to Option<Vec<String>> to allow creating modules
with multiple authors. Additionally, rename the field from author to
authors to make it explicit that it can refer to multiple authors.  In
modules that use the author field, update its value to an array of
strings, and also rename it from author to authors.

Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Link: https://github.com/Rust-for-Linux/linux/issues/244
Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
 drivers/block/rnull.rs               |  2 +-
 drivers/net/phy/ax88796b_rust.rs     |  2 +-
 drivers/net/phy/qt2025.rs            |  2 +-
 rust/kernel/net/phy.rs               |  4 ++--
 rust/kernel/pci.rs                   |  2 +-
 rust/kernel/platform.rs              |  2 +-
 rust/macros/lib.rs                   |  6 +++---
 rust/macros/module.rs                | 12 +++++++-----
 samples/rust/rust_driver_faux.rs     |  2 +-
 samples/rust/rust_driver_pci.rs      |  2 +-
 samples/rust/rust_driver_platform.rs |  2 +-
 samples/rust/rust_minimal.rs         |  2 +-
 samples/rust/rust_misc_device.rs     |  2 +-
 samples/rust/rust_print_main.rs      |  2 +-
 14 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs
index ddf3629d8894..d07e76ae2c13 100644
--- a/drivers/block/rnull.rs
+++ b/drivers/block/rnull.rs
@@ -27,7 +27,7 @@
 module! {
     type: NullBlkModule,
     name: "rnull_mod",
-    author: "Andreas Hindborg",
+    authors: ["Andreas Hindborg"],
     description: "Rust implementation of the C null block driver",
     license: "GPL v2",
 }
diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index 8c7eb009d9fc..bc73ebccc2aa 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -19,7 +19,7 @@
         DeviceId::new_with_driver::<PhyAX88796B>()
     ],
     name: "rust_asix_phy",
-    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
+    authors: ["FUJITA Tomonori <fujita.tomonori@gmail.com>"],
     description: "Rust Asix PHYs driver",
     license: "GPL",
 }
diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
index 1ab065798175..520daeb42089 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -26,7 +26,7 @@
         phy::DeviceId::new_with_driver::<PhyQT2025>(),
     ],
     name: "qt2025_phy",
-    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
+    authors: ["FUJITA Tomonori <fujita.tomonori@gmail.com>"],
     description: "AMCC QT2025 PHY driver",
     license: "GPL",
     firmware: ["qt2025-2.0.3.3.fw"],
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index bb654a28dab3..a59469c785e3 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -790,7 +790,7 @@ const fn as_int(&self) -> u32 {
 ///         DeviceId::new_with_driver::<PhySample>()
 ///     ],
 ///     name: "rust_sample_phy",
-///     author: "Rust for Linux Contributors",
+///     authors: ["Rust for Linux Contributors"],
 ///     description: "Rust sample PHYs driver",
 ///     license: "GPL",
 /// }
@@ -819,7 +819,7 @@ const fn as_int(&self) -> u32 {
 /// module! {
 ///     type: Module,
 ///     name: "rust_sample_phy",
-///     author: "Rust for Linux Contributors",
+///     authors: ["Rust for Linux Contributors"],
 ///     description: "Rust sample PHYs driver",
 ///     license: "GPL",
 /// }
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 4c98b5b9aa1e..f7b2743828ae 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -103,7 +103,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
 /// kernel::module_pci_driver! {
 ///     type: MyDriver,
 ///     name: "Module name",
-///     author: "Author name",
+///     authors: ["Author name"],
 ///     description: "Description",
 ///     license: "GPL v2",
 /// }
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 50e6b0421813..1297f5292ba9 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -101,7 +101,7 @@ fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
 /// kernel::module_platform_driver! {
 ///     type: MyDriver,
 ///     name: "Module name",
-///     author: "Author name",
+///     authors: ["Author name"],
 ///     description: "Description",
 ///     license: "GPL v2",
 /// }
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index d61bc6a56425..7ce1cb891dfb 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -36,7 +36,7 @@
 /// module!{
 ///     type: MyModule,
 ///     name: "my_kernel_module",
-///     author: "Rust for Linux Contributors",
+///     authors: ["Rust for Linux Contributors"],
 ///     description: "My very own kernel module!",
 ///     license: "GPL",
 ///     alias: ["alternate_module_name"],
@@ -69,7 +69,7 @@
 /// module!{
 ///     type: MyDeviceDriverModule,
 ///     name: "my_device_driver_module",
-///     author: "Rust for Linux Contributors",
+///     authors: ["Rust for Linux Contributors"],
 ///     description: "My device driver requires firmware",
 ///     license: "GPL",
 ///     firmware: ["my_device_firmware1.bin", "my_device_firmware2.bin"],
@@ -88,7 +88,7 @@
 /// # Supported argument types
 ///   - `type`: type which implements the [`Module`] trait (required).
 ///   - `name`: ASCII string literal of the name of the kernel module (required).
-///   - `author`: string literal of the author of the kernel module.
+///   - `authors`: array of ASCII string literals of the authors of the kernel module.
 ///   - `description`: string literal of the description of the kernel module.
 ///   - `license`: ASCII string literal of the license of the kernel module (required).
 ///   - `alias`: array of ASCII string literals of the alias names of the kernel module.
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index cdf94f4982df..db16f0af0855 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -94,7 +94,7 @@ struct ModuleInfo {
     type_: String,
     license: String,
     name: String,
-    author: Option<String>,
+    authors: Option<Vec<String>>,
     description: Option<String>,
     alias: Option<Vec<String>>,
     firmware: Option<Vec<String>>,
@@ -107,7 +107,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
         const EXPECTED_KEYS: &[&str] = &[
             "type",
             "name",
-            "author",
+            "authors",
             "description",
             "license",
             "alias",
@@ -135,7 +135,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
             match key.as_str() {
                 "type" => info.type_ = expect_ident(it),
                 "name" => info.name = expect_string_ascii(it),
-                "author" => info.author = Some(expect_string(it)),
+                "authors" => info.authors = Some(expect_string_array(it)),
                 "description" => info.description = Some(expect_string(it)),
                 "license" => info.license = expect_string_ascii(it),
                 "alias" => info.alias = Some(expect_string_array(it)),
@@ -183,8 +183,10 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
     let info = ModuleInfo::parse(&mut it);
 
     let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
-    if let Some(author) = info.author {
-        modinfo.emit("author", &author);
+    if let Some(authors) = info.authors {
+        for author in authors {
+            modinfo.emit("authors", &author);
+        }
     }
     if let Some(description) = info.description {
         modinfo.emit("description", &description);
diff --git a/samples/rust/rust_driver_faux.rs b/samples/rust/rust_driver_faux.rs
index 048c6cb98b29..378bab4b587d 100644
--- a/samples/rust/rust_driver_faux.rs
+++ b/samples/rust/rust_driver_faux.rs
@@ -7,7 +7,7 @@
 module! {
     type: SampleModule,
     name: "rust_faux_driver",
-    author: "Lyude Paul",
+    authors: ["Lyude Paul"],
     description: "Rust faux device sample",
     license: "GPL",
 }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 1fb6e44f3395..364a0660a743 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -104,7 +104,7 @@ fn drop(&mut self) {
 kernel::module_pci_driver! {
     type: SampleDriver,
     name: "rust_driver_pci",
-    author: "Danilo Krummrich",
+    authors: ["Danilo Krummrich"],
     description: "Rust PCI driver",
     license: "GPL v2",
 }
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 8120609e2940..f7a0f1b29d1d 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -43,7 +43,7 @@ fn drop(&mut self) {
 kernel::module_platform_driver! {
     type: SampleDriver,
     name: "rust_driver_platform",
-    author: "Danilo Krummrich",
+    authors: ["Danilo Krummrich"],
     description: "Rust Platform driver",
     license: "GPL v2",
 }
diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs
index 4aaf117bf8e3..1fc7a1be6b6d 100644
--- a/samples/rust/rust_minimal.rs
+++ b/samples/rust/rust_minimal.rs
@@ -7,7 +7,7 @@
 module! {
     type: RustMinimal,
     name: "rust_minimal",
-    author: "Rust for Linux Contributors",
+    authors: ["Rust for Linux Contributors"],
     description: "Rust minimal sample",
     license: "GPL",
 }
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 40ad7266c225..d3785e7c0330 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -116,7 +116,7 @@
 module! {
     type: RustMiscDeviceModule,
     name: "rust_misc_device",
-    author: "Lee Jones",
+    authors: ["Lee Jones"],
     description: "Rust misc device sample",
     license: "GPL",
 }
diff --git a/samples/rust/rust_print_main.rs b/samples/rust/rust_print_main.rs
index 7e8af5f176a3..8ea95e8c2f36 100644
--- a/samples/rust/rust_print_main.rs
+++ b/samples/rust/rust_print_main.rs
@@ -8,7 +8,7 @@
 module! {
     type: RustPrint,
     name: "rust_print",
-    author: "Rust for Linux Contributors",
+    authors: ["Rust for Linux Contributors"],
     description: "Rust printing macros sample",
     license: "GPL",
 }
-- 
2.34.1

[PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Guilherme Giacomo Simoes]

Implement a check to ensure that the author, firmware, and alias fields
of the module! macro are properly formatted.

* If the array contains more than one value, enforce vertical
  formatting.
* If the array contains only one value, it may be formatted on a single
  line

Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
 scripts/checkpatch.pl | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7b28ad331742..54e1893d13aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2775,6 +2775,9 @@ sub process {
 	$realcnt = 0;
 	$linenr = 0;
 	$fixlinenr = -1;
+
+	my %array_parse_module;
+
 	foreach my $line (@lines) {
 		$linenr++;
 		$fixlinenr++;
@@ -3567,6 +3570,46 @@ sub process {
 # ignore non-hunk lines and lines being removed
 		next if (!$hunk_line || $line =~ /^-/);
 
+# check if the field is about author, firmware or alias from module! macro and find malformed arrays
+		my $inline = 0;
+		my $key = "";
+		my $add_line = $line =~ /^\+/;
+
+		if ($line =~ /\b(authors|alias|firmware)\s*:\s*\[/) {
+			$inline = 1;
+			$array_parse_module{$1} = 1;
+		}
+
+		my @keys = keys %array_parse_module;
+		if (@keys) {
+			$key = $keys[0];
+		}
+
+		if ($add_line && $key) {
+			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
+
+			my $counter = () = $line =~ /"/g;
+			my $more_than_one = $counter > 2;
+			if ($more_than_one) {
+				WARN("ARRAY_MODULE_MACRO",
+				     "Prefer each array element on a separate line\n". $herevet);
+			} elsif ($inline && $line !~ /\]/ && $line !~ /,/ && $line =~ /"/) {
+				WARN("ARRAY_MODULE_MACRO",
+				     "Prefer declare ] on the same line\n" . $herevet);
+			} elsif (!$inline && $line =~ /\]/ && $line =~ /\"/) {
+				WARN("ARRAY_MODULE_MACRO",
+				     "Prefer a new line after the last value and before ]\n" . $herevet);
+			} elsif ($inline && $line =~ /,/ && $line !~ /\]/) {
+				WARN("ARRAY_MODULE_MACRO",
+				     "Prefer a new line after [\n" . $herevet);
+			}
+		}
+
+		#END OF ANALYZE FIELD
+		if ($line =~ /\]/) {
+			delete $array_parse_module{$key};
+		}
+
 #trailing whitespace
 		if ($line =~ /^\+.*\015/) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
-- 
2.34.1

Re: [PATCH V6 1/2] rust: module: change author to an array

[Charalampos Mitrodimas]

Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> writes:

> In the module! macro, the author field is currently of type String.
> Since modules can have multiple authors, this limitation prevents
> specifying more than one.
> Change the author field to Option<Vec<String>> to allow creating modules
> with multiple authors. Additionally, rename the field from author to
> authors to make it explicit that it can refer to multiple authors.  In
> modules that use the author field, update its value to an array of
> strings, and also rename it from author to authors.
>
> Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/244
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> ---
>  drivers/block/rnull.rs               |  2 +-
>  drivers/net/phy/ax88796b_rust.rs     |  2 +-
>  drivers/net/phy/qt2025.rs            |  2 +-
>  rust/kernel/net/phy.rs               |  4 ++--
>  rust/kernel/pci.rs                   |  2 +-
>  rust/kernel/platform.rs              |  2 +-
>  rust/macros/lib.rs                   |  6 +++---
>  rust/macros/module.rs                | 12 +++++++-----
>  samples/rust/rust_driver_faux.rs     |  2 +-
>  samples/rust/rust_driver_pci.rs      |  2 +-
>  samples/rust/rust_driver_platform.rs |  2 +-
>  samples/rust/rust_minimal.rs         |  2 +-
>  samples/rust/rust_misc_device.rs     |  2 +-
>  samples/rust/rust_print_main.rs      |  2 +-
>  14 files changed, 23 insertions(+), 21 deletions(-)

Rust side looks good to me. Thanks!

Reviewed-by: Charalampos Mitrodimas <charmitro@posteo.net>

Re: [PATCH V6 0/2] author field in module! macro should be a array

[Andreas Hindborg]

"Guilherme Giacomo Simoes" <trintaeoitogc@gmail.com> writes:

> In the module! macro, the author field is currently of type String.
> Since modules can have multiple authors, this limitation prevents
> specifying more than one.
>
> - Change the author field to Option<Vec<String>> to allow creating
>   modules with multiple authors.
>
> - rename the field from author to authors to make it explicit that it
>   can refer to multiple authors.
>
> - In modules that use the author field, update its value to an array of
>   strings, and also rename it from author to authors.
>
> - Change the checkpatch.pl to find poorly formatted arrays in the macro
>   module!
>
> ---
> v6 changes
> - Use consistent style in warn messages
> - Improved the commit title for checkpatch changes
> ---
>
> Guilherme Giacomo Simoes (2):
>   rust: module: change author to an array
>   checkpatch: check format of Vec<String> in modules
>
>  drivers/block/rnull.rs               |  2 +-
>  drivers/net/phy/ax88796b_rust.rs     |  2 +-
>  drivers/net/phy/qt2025.rs            |  2 +-
>  rust/kernel/net/phy.rs               |  4 +--
>  rust/kernel/pci.rs                   |  2 +-
>  rust/kernel/platform.rs              |  2 +-
>  rust/macros/lib.rs                   |  6 ++--
>  rust/macros/module.rs                | 12 ++++----
>  samples/rust/rust_driver_faux.rs     |  2 +-
>  samples/rust/rust_driver_pci.rs      |  2 +-
>  samples/rust/rust_driver_platform.rs |  2 +-
>  samples/rust/rust_minimal.rs         |  2 +-
>  samples/rust/rust_misc_device.rs     |  2 +-
>  samples/rust/rust_print_main.rs      |  2 +-
>  scripts/checkpatch.pl                | 43 ++++++++++++++++++++++++++++
>  15 files changed, 66 insertions(+), 21 deletions(-)

What is the base commit for this one?


Best regards,
Andreas Hindborg

Re: [PATCH V6 1/2] rust: module: change author to an array

[Andreas Hindborg]

"Guilherme Giacomo Simoes" <trintaeoitogc@gmail.com> writes:

> In the module! macro, the author field is currently of type String.
> Since modules can have multiple authors, this limitation prevents
> specifying more than one.
> Change the author field to Option<Vec<String>> to allow creating modules
> with multiple authors. Additionally, rename the field from author to
> authors to make it explicit that it can refer to multiple authors.  In
> modules that use the author field, update its value to an array of
> strings, and also rename it from author to authors.
>
> Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/244
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Andreas Hindborg]

"Guilherme Giacomo Simoes" <trintaeoitogc@gmail.com> writes:

> Implement a check to ensure that the author, firmware, and alias fields
> of the module! macro are properly formatted.
>
> * If the array contains more than one value, enforce vertical
>   formatting.
> * If the array contains only one value, it may be formatted on a single
>   line
>
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>

Tested-by: Andreas Hindborg <a.hindborg@kernel.org>

Does what it says on the label. However, indentation is not checked and
the following is passing:

    authors: [
        "John Doe",
              "Foo Bar"
    ],

Another thing: I'm wondering if we could have a rust hostprog to do
this check, rather than a perl script?


Best regards,
Andreas Hindborg

Re: [PATCH V6 1/2] rust: module: change author to an array

[Alice Ryhl]

On Sun, Feb 23, 2025 at 6:42 PM Guilherme Giacomo Simoes
<trintaeoitogc@gmail.com> wrote:
>
> In the module! macro, the author field is currently of type String.
> Since modules can have multiple authors, this limitation prevents
> specifying more than one.
> Change the author field to Option<Vec<String>> to allow creating modules
> with multiple authors. Additionally, rename the field from author to
> authors to make it explicit that it can refer to multiple authors.  In
> modules that use the author field, update its value to an array of
> strings, and also rename it from author to authors.
>
> Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/244
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Guilherme Giacomo Simoes]

Andreas Hindborg <a.hindborg@kernel.org> wrote:
> Does what it says on the label. However, indentation is not checked and
> the following is passing:
> 
>     authors: [
>         "John Doe",
>               "Foo Bar"
>     ],
> 
You is right, maybe is good doing this?

> Another thing: I'm wondering if we could have a rust hostprog to do
> this check, rather than a perl script?
hostprog like rustfmt?
About this, Daniel Sedlak say:
"I think we could fight with the code formatting, because when it comes 
to the rust macros, rustfmt is often very confused and we could end up 
with variations like:

	authors: ["author1", "author2",
			"author3"]

or

	authors: [
		   "author1",
		   "author2",
		  ]
"

Thanks,
Guilherme

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Miguel Ojeda]

On Tue, Mar 4, 2025 at 9:51 PM Guilherme Giacomo Simoes
<trintaeoitogc@gmail.com> wrote:
>
> hostprog like rustfmt?

No, Andreas means a script written in Rust, rather than a binary that
comes from the toolchain.

I think it could be a good idea (it would be lovely to write the
checker in Rust -- I also had a checker bot in Python from the old
days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't
need a built kernel, so it would be a disadvantage or at least a
difference w.r.t. the usual `checkpatch.pl`, and we may not be able to
call it from `checkpatch.pl`.

> About this, Daniel Sedlak say:
> "I think we could fight with the code formatting, because when it comes
> to the rust macros, rustfmt is often very confused and we could end up
> with variations like:

Did you check? i.e. is it something we noticed, or something that
generally happens but maybe not in this case? Is there a way to
workaround or disable that (e.g. a `rustfmt` config value)?

Thanks!

Cheers,
Miguel

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Miguel Ojeda]

On Tue, Mar 4, 2025 at 10:00 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I think it could be a good idea (it would be lovely to write the
> checker in Rust -- I also had a checker bot in Python from the old
> days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't
> need a built kernel, so it would be a disadvantage or at least a
> difference w.r.t. the usual `checkpatch.pl`, and we may not be able to
> call it from `checkpatch.pl`.

By "built kernel", I don't mean an entire kernel, but rather having to
call `make` in general, i.e. we could have a target to build just the
script, but still, it is a difference.

Cheers,
Miguel

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, Mar 4, 2025 at 10:00 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> I think it could be a good idea (it would be lovely to write the
>> checker in Rust -- I also had a checker bot in Python from the old
>> days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't
>> need a built kernel, so it would be a disadvantage or at least a
>> difference w.r.t. the usual `checkpatch.pl`, and we may not be able to
>> call it from `checkpatch.pl`.
>
> By "built kernel", I don't mean an entire kernel, but rather having to
> call `make` in general, i.e. we could have a target to build just the
> script, but still, it is a difference.

Right, it needs a bit more tool support than running checkpatch.pl
needs. Perhaps we could move it from checkpatch.pl to the rustfmt make
target?


Best regards,
Andreas Hindborg

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Miguel Ojeda]

On Wed, Mar 5, 2025 at 8:53 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Right, it needs a bit more tool support than running checkpatch.pl
> needs. Perhaps we could move it from checkpatch.pl to the rustfmt make
> target?

That could perhaps be an option for this case, though not sure if it
applies to all cases, i.e. `checkpatch.pl` also checks things that
only make sense to check in a patch and also things that are not
related to formatting.

Perhaps we want an entirely separate thing in `tools/` eventually, or
even out of the kernel tree, so that it can be easily run as a bot
etc. like in the past.

In any case, landing checks here is fine (as long as Joe et al.
agree), they can be moved or removed later if needed.

Cheers,
Miguel

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Wed, Mar 5, 2025 at 8:53 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Right, it needs a bit more tool support than running checkpatch.pl
>> needs. Perhaps we could move it from checkpatch.pl to the rustfmt make
>> target?
>
> That could perhaps be an option for this case, though not sure if it
> applies to all cases, i.e. `checkpatch.pl` also checks things that
> only make sense to check in a patch and also things that are not
> related to formatting.
>
> Perhaps we want an entirely separate thing in `tools/` eventually, or
> even out of the kernel tree, so that it can be easily run as a bot
> etc. like in the past.
>
> In any case, landing checks here is fine (as long as Joe et al.
> agree), they can be moved or removed later if needed.

Absolutely. Just brainstorming at this point.


Best regards,
Andreas Hindborg

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Guilherme Giacomo Simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> No, Andreas means a script written in Rust, rather than a binary that
> comes from the toolchain.
> 
> I think it could be a good idea (it would be lovely to write the
> checker in Rust -- I also had a checker bot in Python from the old
> days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't
> need a built kernel, so it would be a disadvantage or at least a
> difference w.r.t. the usual `checkpatch.pl`, and we may not be able to
> call it from `checkpatch.pl`.
I don't know if I really understand how this would is do.

> Did you check? i.e. is it something we noticed, or something that
> generally happens but maybe not in this case? Is there a way to
> workaround or disable that (e.g. a `rustfmt` config value)?
The rustfmt have a array_width parameter [1], but with this, all arrays in rust
code will have the formatting that we set. (In this case, is 1 per line).

If we set the max_width, for limit the width of line, it seens for me, that
arrays don't are affected.

[1] https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#array_width

Thanks,
Guilherme

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Miguel Ojeda]

On Thu, Mar 6, 2025 at 2:59 AM Guilherme Giacomo Simoes
<trintaeoitogc@gmail.com> wrote:
>
> I don't know if I really understand how this would is do.

Do not worry about it for this series -- it is something for the future.

> The rustfmt have a array_width parameter [1], but with this, all arrays in rust
> code will have the formatting that we set. (In this case, is 1 per line).

Yeah, I was thinking more of something like `skip_macro_invocations`
(but it is unstable) or perhaps one like `format_macro_*` that refers
to arrays within macros or similar (but I don't think it exists yet).
I added the former to our wishlist at
https://github.com/Rust-for-Linux/linux/issues/398, anyway, since I
suspect it could be useful eventually.

In any case, without changing the configuration, i.e. the current
status, what happens in the different cases? e.g. 1 item, 2 items, 10
items...?

Thanks!

Cheers,
Miguel

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Guilherme Giacomo Simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> In any case, without changing the configuration, i.e. the current
> status, what happens in the different cases? e.g. 1 item, 2 items, 10
> items...?
In current config, the formatting seems don't have a effect for arrays. 
i.e. this array: authors: [ "author_1", "author_2", "author_3", "author_4","author_5","author_6","author_7" ]
don't is fixed by rustfmt for no format.

Thanks,
Guilherme

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Guilherme Giacomo Simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> Perhaps we want an entirely separate thing in `tools/` eventually, or
> even out of the kernel tree, so that it can be easily run as a bot
> etc. like in the past.
A out of kernel tree tool, is a good idea for me. But a separated tool in
`tools/`, don't seems a good ideia for me, because, we have things a lot for do
in addition on developing our code.
like: 
	- make LLVM=1 rusttest
	- ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 --kconfig_add CONFIG_RUST=y
	- ./scripts/checkpatch.pl
	... and anothers things more.... 

I don't think that this things is uneccessary, but, if we might reduce the work
of developer, and set he for focus on your code, seem better for me.

This is only my opinion that can be absolutely wrong... 

Thanks,
Guilherme

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Guilherme Giacomo Simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> In any case, landing checks here is fine (as long as Joe et al.
> agree), they can be moved or removed later if needed.
The only thing bad in this check, is when the array is very very big, like:
    authors: [ 
        "author_1"
        "author_2"
        "author_3"
        "author_4"
        "author_5"
        "author_6"
        "author_8"        
		"author_9" 
        "author_10" 
        "author_11" 
        "author_12" 
        "author_13" 
        "author_14" 
        "author_15" 
        "author_16" 
        "author_17" 
    ],


if I set a new author_18, the diff will be:
         "author_15" 
         "author_16" 
         "author_17" 
+        "author_18" 
     ],
And, in this case, if I make a wrong change, like:

         "author_15" 
         "author_16" 
         "author_17" 
+        "author_18" , "author_19"
     ],
the checkpatch don't will throw a warning message, because, he don't can
perceive that he is within of array of author, firmware or alias of module!. 

I couldn't do a better check for this.. 

thoughs? 

Thanks,
Guilherme

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Miguel Ojeda]

On Thu, Mar 6, 2025 at 5:32 PM Guilherme Giacomo Simoes
<trintaeoitogc@gmail.com> wrote:
>
> the checkpatch don't will throw a warning message, because, he don't can
> perceive that he is within of array of author, firmware or alias of module!.

I wouldn't worry about that case -- it is unlikely we even have the
case, and even if we do, just manually checking it is fine. :)

Cheers,
Miguel

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Miguel Ojeda]

On Thu, Mar 6, 2025 at 5:08 PM Guilherme Giacomo Simoes
<trintaeoitogc@gmail.com> wrote:
>
> In current config, the formatting seems don't have a effect for arrays.
> i.e. this array: authors: [ "author_1", "author_2", "author_3", "author_4","author_5","author_6","author_7" ]
> don't is fixed by rustfmt for no format.

So if I understand correctly, `rustfmt` will simply not do anything,
i.e. it will always keep the array as it was -- that sounds good
enough to me! Most people will write it correctly, and for those that
don't, `checkpatch.pl` will likely catch the mistake -- that is all we
need.

Cheers,
Miguel

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Guilherme Giacomo Simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> So if I understand correctly, `rustfmt` will simply not do anything,
> i.e. it will always keep the array as it was -- that sounds good
> enough to me! Most people will write it correctly, and for those that
> don't, `checkpatch.pl` will likely catch the mistake -- that is all we
> need.
Ok, but, how is mentioned by Andreas Hindborg, this scenary is passing:
    authors: [
        "John Doe",
              "Foo Bar"
    ],

Is good make a check for this too and senda a v7

Thanks,
Guilherme

Re: [PATCH V6 2/2] checkpatch: check format of Vec<String> in modules

[Miguel Ojeda]

On Thu, Mar 6, 2025 at 6:18 PM Guilherme Giacomo Simoes
<trintaeoitogc@gmail.com> wrote:
>
> Is good make a check for this too and senda a v7

Sure, the more the check catches, the better (as long as it is not
unreasonably complex).

Cheers,
Miguel