Received-SPF: pass (google.com: domain of linux-kernel+bounces-129451-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Andreas Hindborg <nmi@metaspace.dk>
To: Benno Lossin <benno.lossin@proton.me>
Cc: Jens Axboe <axboe@kernel.dk>,  Christoph Hellwig <hch@lst.de>,  Keith
 Busch <kbusch@kernel.org>,  Damien Le Moal <Damien.LeMoal@wdc.com>,  Bart
 Van Assche <bvanassche@acm.org>,  Hannes Reinecke <hare@suse.de>,
  "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,  Andreas
 Hindborg <a.hindborg@samsung.com>,  Niklas Cassel <Niklas.Cassel@wdc.com>,
  Greg KH <gregkh@linuxfoundation.org>,  Matthew Wilcox
 <willy@infradead.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>,  Alice Ryhl <aliceryhl@google.com>,
  Chaitanya Kulkarni <chaitanyak@nvidia.com>,  Luis Chamberlain
 <mcgrof@kernel.org>,  Yexuan Yang <1182282462@bupt.edu.cn>,  Sergio
 =?utf-8?Q?Gonz=C3=A1lez?= Collado <sergio.collado@gmail.com>,  Joel
 Granados
 <j.granados@samsung.com>,  "Pankaj Raghav (Samsung)"
 <kernel@pankajraghav.com>,  Daniel Gomez <da.gomez@samsung.com>,  open
 list <linux-kernel@vger.kernel.org>,  "rust-for-linux@vger.kernel.org"
 <rust-for-linux@vger.kernel.org>,  "lsf-pc@lists.linux-foundation.org"
 <lsf-pc@lists.linux-foundation.org>,  "gost.dev@samsung.com"
 <gost.dev@samsung.com>
Subject: Re: [RFC PATCH 4/5] rust: block: add rnull, Rust null_blk
 implementation
In-Reply-To: <1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me> (Benno Lossin's
	message of "Tue, 02 Apr 2024 22:35:32 +0000")
References: <20240313110515.70088-1-nmi@metaspace.dk>
	<20240313110515.70088-5-nmi@metaspace.dk>
	<QqpNcEOxhslSB7-34znxmQK_prPJfe2GT0ejWLesj-Dlse1ueCacbzsJOM0LK3YmgQsUWAR58ZFPPh1MUCliionIXrvLNsOqTS_Ee3bXEuQ=@proton.me>
	<87msqc3p0e.fsf@metaspace.dk>
	<1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me>
User-Agent: mu4e 1.12.2; emacs 29.3
Date: Wed, 03 Apr 2024 11:47:18 +0200
Message-ID: <87edbmsrq1.fsf@metaspace.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Benno Lossin <benno.lossin@proton.me> writes:

[...]

>
>
> So I did some digging and there are multiple things at play. I am going
> to explain the second error first, since that one might be a problem
> with `pin_init`:
> - the `params` extension of the `module!` macro creates constants with
>    snake case names.
> - your `QueueData` struct has the same name as a field.
> - `pin_init!` generates `let $field_name =3D ...` statements for each
>    field you initialize
>
> Now when you define a constant in Rust, you are able to pattern-match
> with that constant, eg:
>
>      const FOO: u8 =3D 0;
>=20=20=20=20=20=20
>      fn main() {
>          match 10 {
>              FOO =3D> println!("foo"),
>              _ =3D> {}
>          }
>      }
>
> So when you do `let FOO =3D x;`, then it interprets `FOO` as the constant.
> This is still true if the constant has a snake case name.
> Since the expression in the `pin_init!` macro has type
> `DropGuard<$field_type>`, we get the error "expected
> `DropGuard<IRQMode>`, found `__rnull_mod_irq_mode`".

Thanks for the analysis!

So in this expanded code:

1   {
2       unsafe { ::core::ptr::write(&raw mut ((*slot).irq_mode), irq_mode) =
};
3   }
4   let irq_mode =3D unsafe {
5       $crate::init::__internal::DropGuard::new(&raw mut ((*slot).irq_mode=
))
6   };

the `irq_mode` on line 2 will refer to the correct thing, but the one on
line 6 will be a pattern match against a constant? That is really
surprising to me. Can we make the let binding in line 6 be `let
irq_mode_pin_init` or something similar?

>
> Now to the first error, this is a problem with the parameter handling of
> `module`. By the same argument above, your let binding in line 104:
>
>      let irq_mode =3D (*irq_mode.read()).try_into()?;
>
> Tries to pattern-match the `irq_mode` constant with the right
> expression. Since you use the `try_into` function, it tries to search
> for a `TryInto` implementation for the type of `irq_mode` which is
> generated by the `module!` macro. The type is named
> __rnull_mod_irq_mode.
>
>
> Now what to do about this. For the second error (the one related to
> `pin_init`), I could create a patch that fixes it by adding the suffix
> `_guard` to those let bindings, preventing the issue. Not sure if we
> need that though, since it will not get rid of the first issue.

I think that is a good idea =F0=9F=91=8D

>
> For the first issue, I think there is no other way than to use a
> different name for either the field or the constant. Since constants are
> usually named using screaming snake case, I think it should be renamed.
> I believe your reason for using a snake case name is that these names
> are used directly as the names for the parameters when loading the
> module and there the convention is to use snake case, right?
> In that case I think we could expect people to write the screaming snake
> case name in rust and have it automatically be lower-cased by the
> `module!` macro when it creates the names that the parameters are shown
> with.

I was thinking about putting the parameters in a separate name space,
but making them screaming snake case is also a good idea. So it would
be `module_parameters::IRQ_MODE` to access the parameter with the name
`irq_mode` exposed towards the user. Developers can always opt in to bringi=
ng
the symbols into scope with a `use`.

Best regards,
Andreas