Message-Id: <D7PYBD1V7Y0W.3TGGTJF6MHY5Z@gmail.com>
Date: Tue, 11 Feb 2025 16:57:46 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Lyude Paul" <lyude@...hat.com>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>
Cc: <linux-kernel@...r.kernel.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 "Danilo Krummrich" <dakr@...nel.org>, "Alexander Lobakin"
 <aleksander.lobakin@...el.com>, "Andy Shevchenko"
 <andriy.shevchenko@...ux.intel.com>, "Bjorn Helgaas" <bhelgaas@...gle.com>,
 "Jonathan Cameron" <Jonathan.Cameron@...wei.com>, "Liam Girdwood"
 <lgirdwood@...il.com>, "Lukas Wunner" <lukas@...ner.de>, "Mark Brown"
 <broonie@...nel.org>, Maíra Canal <mairacanal@...eup.net>,
 "Robin Murphy" <robin.murphy@....com>, "Simona Vetter"
 <simona.vetter@...ll.ch>, "Zijun Hu" <quic_zijuhu@...cinc.com>,
 <linux-usb@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Subject: Re: [PATCH v4 1/9] driver core: add a faux bus for use when a
 simple device/bus is needed

Hi Lyude,

On Tue Feb 11, 2025 at 3:06 PM -05, Lyude Paul wrote:
> On Mon, 2025-02-10 at 10:52 -0500, Kurt Borja wrote:
>> > Modules usually don't need to do the probe callback at all.  I show one
>> > example in this series (the regulator dummy driver), but it can be
>> > easily rewritten to not need that at all.
>> 
>> This is a good point, but from a developer standpoint I would always try
>> to use the probe callback. This API seems to suggest that's the
>> appropiate use.
>> 
>> Also it would be amazing if the probe could reside in an __init section.
>
> IMO I think it is fine without the probe callback, plus we're the ones making
> the API - it can say whatever we want :).

IMO I think you are right.

>
> To be clear though, generally I'm fairly sure the only reason for drivers to
> be using probe() at all is because you want a driver-callback the kernel is
> responsible for executing in response to a new device being added on a bus.
> This doesn't really make sense for a virtual bus, since we're in control of
> adding devices - and thus probe() would just be an unnecessary layer for work
> that can already easily be done from the same call site that added the device.
> So I think it's fine for this to be a special case imo.

I'm still just a newbie in kernel development so keep that in mind.

For me having probe and remove callbacks on "real" hardware devices has
always been about gurantees. Gurantees about lifetimes of resources
assigned by the driver/bus/subsystem (I just started learning rust :p).
Me as a driver, I know I can initialize a bunch of memory, interfaces,
etc. In a safe way, inside the probe because I know everything is
guranteed to be valid until just after the .remove callback.

Of course, this does not apply to this bus because the bus itself and
the single driver are very minimalistic, so we can achieve the same
gurantees in module __init and __exit.

So I agree, the probe probably should just be dropped. However, if the
probe goes then so must faux_device_create_with_groups() because without
a probe the API can't give any gurantees about the lifetime of for
example the drvdata opaque pointer.

I think this is a small problem, because in platform-drivers-x86 for
example, there is a lot of "fake" platform devices that expose a sysfs
interface. So if this devices were made today, they could use this faux
bus, but they would need to create/remove the sysfs groups at the
appropiate time, as to not mess with lifetimes, which is a bit 
bug-prone.

Additionally the current faux device API is very misleading, because
although it gives you a choice to add groups, this groups get added way
before the probe you provide is even executed, and even if it fails. I
believe this could lead to a lot of misbehaviors and bugs.

-- 
 ~ Kurt

Powered by blists - more mailing lists