Re: [PATCH 3/4] rust: configfs: introduce rust support for configfs

[Andreas Hindborg <a.hindborg@kernel.org>]

"Fiona Behrens" <me@kloenk.dev> writes:

> Andreas Hindborg <a.hindborg@kernel.org> writes:
>

[cut]

>> +/// A `configfs` subsystem.
>> +///
>> +/// This is the top level entrypoint for a `configfs` hierarchy. Embed a field
>> +/// of this type into a struct and implement [`HasSubsystem`] for the struct
>> +/// with the [`kernel::impl_has_subsystem`] macro. Instantiate the subsystem with
>> +/// [`Subsystem::register`].
>> +///
>> +/// A [`Subsystem`] is also a [`Group`], and implementing [`HasSubsystem`] for a
>> +/// type will automatically implement [`HasGroup`] for the type.
>
> Rustdoc is unhappy about this (and other lines where `HasGroup` is used)
> as `HasGroup` is private and therefore rustdoc cannot resolve this link.

Right, I'll remove this link.

>
>> +#[pin_data(PinnedDrop)]
>> +pub struct Subsystem<DATA> {
>
> I would favour this as PascalCase (so `Subsystem<Data>`), as this is a
> generic type and not a generic const (I always see all uppercase for
> const generics).

Yea, that makes sense. I somehow thought all generics had to be
screaming snake.


[cut]

>> +
>> +#[pinned_drop]
>> +impl<DATA> PinnedDrop for Subsystem<DATA> {
>> +    fn drop(self: Pin<&mut Self>) {
>> +        // SAFETY: We registered `self.subsystem` in the initializer returned by `Self::new`.
>> +        unsafe { bindings::configfs_unregister_subsystem(self.subsystemget()) };
>
> I see other users (e.g. gpio-virtuser[0]) to also destroy the mutex, is
> that a required action?
>
> [0]: https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpio/gpio-virtuser.c#L1841

Referring to C null_blk, they do not destroy the mutex when tearing
down. I am not sure what the correct thing to do is. Perhaps lockdep
wants it destroyed? I'll investigate.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/block/null_blk/main.c?h=v6.14-rc1#n2105


[cut]

>> +
>> +impl<PAR, CHLD, CPTR, PCPTR> GroupOperationsVTable<PAR, CHLD, CPTR, PCPTR>
>> +where
>> +    PAR: GroupOperations<Child = CHLD, ChildPointer = CPTR, PinChildPointer = PCPTR>,
>> +    CPTR: InPlaceInit<Group<CHLD>, PinnedSelf = PCPTR>,
>> +    PCPTR: ForeignOwnable<PointedTo = Group<CHLD>>,
>> +{
>
> I usualy favour having struct and then impls for the functions direcly
> above each other. Here you have th `get_group_data` function between
> that, maybe it makes sense to move that function either further up or
> down.

I agree, I'll move it 👍
>
> So far this is all that I did direcly see.

Thanks!


Best regards,
Andreas Hindborg