Message-ID: <fa831c54-1998-4cea-95fd-e6ced90974e2@beagleboard.org>
Date: Tue, 29 Apr 2025 22:26:17 +0530
From: Ayush Singh <ayush@...gleboard.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Danilo Krummrich <dakr@...nel.org>,
 Jason Kridner <jkridner@...gleboard.org>,
 Deepak Khatri <lorforlinux@...gleboard.org>,
 Robert Nelson <robertcnelson@...gleboard.org>, Dhruva Gole <d-gole@...com>,
 Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, "Rafael J. Wysocki" <rafael@...nel.org>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
 Herve Codina <herve.codina@...tlin.com>,
 Luca Ceresoli <luca.ceresoli@...tlin.com>
Subject: Re: [PATCH v2] rust: kernel: device: Add
 devm_of_platform_populate/depopulate

On 4/29/25 22:09, Greg Kroah-Hartman wrote:

> On Tue, Apr 29, 2025 at 09:37:04PM +0530, Ayush Singh wrote:
>> On 4/29/25 20:27, Greg Kroah-Hartman wrote:
>>
>>> On Tue, Apr 29, 2025 at 04:44:54PM +0200, Danilo Krummrich wrote:
>>>> On Tue, Apr 29, 2025 at 04:37:49PM +0200, Greg Kroah-Hartman wrote:
>>>>> On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
>>>>>> On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
>>>>>>> +    /// Remove devices populated from device tree
>>>>>>> +    pub fn devm_of_platform_depopulate(&self) {
>>>>>>> +        // SAFETY: self is valid bound Device reference
>>>>>>> +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
>>>>>>> +    }
>>>>>>> +}
>>>>>> One additional question regarding devm_of_platform_depopulate(). This function
>>>>>> is only used once throughout the whole kernel (in [1]), and at a first glance
>>>>>> the usage there seems unnecessary.
>>>>>>
>>>>>> In your upcoming driver you call devm_of_platform_depopulate() from a fallible
>>>>>> path [2].
>>>>>>
>>>>>> So, I think we should change devm_of_platform_depopulate() to return an error
>>>>>> instead of WARN(ret).
>>>>>>
>>>>>> If [1] needs it for some subtle reason I don't see, then I think we can still
>>>>>> call it from there as
>>>>>>
>>>>>> 	WARN(devm_of_platform_depopulate())
>>>>>>
>>>>>> [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
>>>>>> [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71
>>>>> Ugh, no, we should just delete this function entirely if only one driver
>>>>> is using it.  That implies it's not really needed at all.
>>>> Ayush's driver calls {de}populate() from a sysfs store path [2]; not sure what
>>>> it's doing semantically or if this is a valid use-case though.
>>> That's going to be rough, and full of tricky corner-cases and probably
>>> shouldn't be doing that at all :)
>>>
>>> So let's hold off on this entirely until we see a real user that can
>>> actually pass review.  Trying to do system configuration like this in
>>> sysfs is a much larger discussion than just adding rust bindings.
>>>
>>> (hint, configfs is for system configuration, not sysfs...)
>>>
>>> Anyway, worst case, you just "open code" the single function call that
>>> this one binding was trying to "wrap".  which is what I think the
>>> in-kernel user should be doing now.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Well, I don't really want to convert this discussion to addon board
>> connector setup discussions. So I will try to keep things as short as
>> possible here while linking to all the other discussions for the same.
>>
>> For starters, what the driver does is as follows:
>>
>> 1. Provide 3 sysfs entries:
>>
>>      - New cape: Can write the name of the cape (I have not settled on the
>> naming convention yet). This name is then used to load appropriate overlay
>> from `/lib/firmware/` and populate all the devices. The overlay is applied
>> to the connector node. Only one cape overlay can be used at a time.
>>
>>      - Current cape: Just a ro entry to get the name of any active cape.
>>
>>      - Delete cape: Remove cape overlay and registered devices.
> That's great, but I don't think that's what sysfs is good for, we can
> discuss that later when you submit your driver for review.
>
> Again, look at configfs please, that's for "configuring" things.  sysfs
> is for basic device properties and some tunables, but is NOT a major api
> interface that requires a lot of logic like loading an overlay would
> require.
>
> Also, circumventing the "normal" device tree overlay interface and
> discussion isn't ok either, this needs to work for all types of devices,
> not just for "capes" like you have here.  To accept something like this
> would be going around all of those other maintainers with their strong
> views of how things should be done.

Umm, can you please explain how exactly I am circumventing "normal" 
devicetree overlay interface and discussion here. If I submit spec or 
devicetree compiler changes, nobody responds. And when I submit the full 
driver, it ends up with it not being the way upstream wants. And I have 
not seen any of Herve and Luca's subsystem specific patches for addon 
board + connector setups accepted yet. So if there is a place where I 
can discuss things regarding the addon board + connector setup more 
before writing full implementations, I would really love that.

I have added Herve and Luca here as well, if they want to add something.

> Anyway, we can discuss that when you submit your code for review, but
> for now, I don't want to take this binding either as I think the whole
> function should just be removed from the kernel anyway :)
>
> thanks,
>
> greg k-h


Best Regards,

Ayush Singh

Powered by blists - more mailing lists