Re: [PATCH v1 1/1] driver core: Move fw_devlink stuff to where it belongs

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Saravana Kannan <saravanak@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Scally <djrscally@gmail.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Len Brown <lenb@kernel.org>
Subject: Re: [PATCH v1 1/1] driver core: Move fw_devlink stuff to where it belongs
Date: Fri, 23 Feb 2024 16:53:09 +0200	[thread overview]
Message-ID: <ZdixVesIli2D943J@smile.fi.intel.com> (raw)
In-Reply-To: <CAGETcx9akN315asPP=e8xieHqs7gKXvHP-BwRxD=vCBuhh8_Jw@mail.gmail.com>

On Thu, Feb 22, 2024 at 04:54:39PM -0800, Saravana Kannan wrote:
> On Wed, Feb 21, 2024 at 4:49 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Feb 21, 2024 at 02:48:52PM +0200, Andy Shevchenko wrote:
> > > On Tue, Feb 20, 2024 at 06:08:56PM -0800, Saravana Kannan wrote:
> > > > On Tue, Feb 20, 2024 at 8:10 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > The rest of the functions here are related to parents and children of
> > > > a fwnode. So, why is this function considered to be in the wrong
> > > > place?
> > >
> > > When devlink was added it made a few fields in struct fwnode_handle.
> > > These fields have no common grounds with device properties. In particular
> > > struct device pointer is solely for devlinks and shouldn't be used with
> > > them. Hence this patch. TL;DR: they semantically do _not_ belong to
> > > the device property APIs.
> 
> But fwnode_is_ancestor_of() uses none of those new fields and seems
> like a very reasonable API to provide. I understand if you want to
> make the "device link only" argument for fwnode_get_next_parent_dev().

Nobody is using that except devlink. That API can be and should be hidden
(we do not add APIs without users). On itself it's useless.

> > On top of that for the 4+ years no new users appeared, so exporting them was
> > a clear mistake. Hence Fixes tags.
> 
> We have plenty of APIs in .h files (not the same as export to modules)
> -- that have only 1 user. And even some with no users. You don't move
> a string function out of lib/string.c just because there's only one
> user of that function. We put functions in C files that make sense.

You understand that this is a weak argument, right? There are generic
functions, there are more specific ones. Here we are talking about niche
APIs without (other / new) users for all this time!

Now about generic one (as string.h is a bad example here), yes we do move
functions from the headers out if we have only one static user. We even target
killing some generic functions (strlcpy() is one and strlcat() is rumored to
follow same destiny.

> I think Fixes is a bit of an overkill. It's not a bug. Fixes get
> propagated to LTS. This is certainly not LTS worthy. I'm not going to
> NACK or push back on this patch for these reasons, but just letting
> you know that you come off as unreasonably grumpy when you do these
> things even for fwnode_is_ancestor_of() :)

I can drop Fixes it won't affect much as as I said there is no (other) user for
it anyway for all these years.

TL;DR: I will drop Fixes for the next version and that's it. I'm not okay of
leaving functions in the header and be exported just for the sake of exporting.

-- 
With Best Regards,
Andy Shevchenko