| Message ID | 20240220161002.2983334-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/1] driver core: Move fw_devlink stuff to where it belongs | expand |
On Tue, Feb 20, 2024 at 06:10:02PM +0200, Andy Shevchenko wrote: > A few APIs that belong specifically to the fw_devlink APIs > - are exposed to others without need > - prevents device property code to be cleaned up in the future > > Resolve this mess by moving fw_devlink code to where it belongs > and hide from others. > > Fixes: b5d3e2fbcb10 ("device property: Add fwnode_is_ancestor_of() and fwnode_get_next_parent_dev()") > Fixes: 372a67c0c5ef ("driver core: Add fwnode_to_dev() to look up device from fwnode") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
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: > > > > A few APIs that belong specifically to the fw_devlink APIs > > - are exposed to others without need > > - prevents device property code to be cleaned up in the future > > > > Resolve this mess by moving fw_devlink code to where it belongs > > and hide from others. ... > 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.
On Wed, Feb 21, 2024 at 07:47:45AM +0000, Sakari Ailus wrote: > On Tue, Feb 20, 2024 at 06:10:02PM +0200, Andy Shevchenko wrote: > > A few APIs that belong specifically to the fw_devlink APIs > > - are exposed to others without need > > - prevents device property code to be cleaned up in the future > > > > Resolve this mess by moving fw_devlink code to where it belongs > > and hide from others. > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> Thank you!
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: > > > > > > > > A few APIs that belong specifically to the fw_devlink APIs > > > > - are exposed to others without need > > > > - prevents device property code to be cleaned up in the future > > > > > > > > Resolve this mess by moving fw_devlink code to where it belongs > > > > and hide from others. > > ... > > > > 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(). > 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. 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() :) -Saravana
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.
diff --git a/drivers/base/core.c b/drivers/base/core.c index 9828da9b933c..35ccd8bb2c9b 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1871,6 +1871,7 @@ static void fw_devlink_unblock_consumers(struct device *dev) device_links_write_unlock(); } +#define get_dev_from_fwnode(fwnode) get_device((fwnode)->dev) static bool fwnode_init_without_drv(struct fwnode_handle *fwnode) { @@ -1901,6 +1902,63 @@ static bool fwnode_ancestor_init_without_drv(struct fwnode_handle *fwnode) return false; } +/** + * fwnode_is_ancestor_of - Test if @ancestor is ancestor of @child + * @ancestor: Firmware which is tested for being an ancestor + * @child: Firmware which is tested for being the child + * + * A node is considered an ancestor of itself too. + * + * Return: true if @ancestor is an ancestor of @child. Otherwise, returns false. + */ +static bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, + const struct fwnode_handle *child) +{ + struct fwnode_handle *parent; + + if (IS_ERR_OR_NULL(ancestor)) + return false; + + if (child == ancestor) + return true; + + fwnode_for_each_parent_node(child, parent) { + if (parent == ancestor) { + fwnode_handle_put(parent); + return true; + } + } + return false; +} + +/** + * fwnode_get_next_parent_dev - Find device of closest ancestor fwnode + * @fwnode: firmware node + * + * Given a firmware node (@fwnode), this function finds its closest ancestor + * firmware node that has a corresponding struct device and returns that struct + * device. + * + * The caller is responsible for calling put_device() on the returned device + * pointer. + * + * Return: a pointer to the device of the @fwnode's closest ancestor. + */ +static struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode) +{ + struct fwnode_handle *parent; + struct device *dev; + + fwnode_for_each_parent_node(fwnode, parent) { + dev = get_dev_from_fwnode(parent); + if (dev) { + fwnode_handle_put(parent); + return dev; + } + } + return NULL; +} + /** * __fw_devlink_relax_cycles - Relax and mark dependency cycles. * @con: Potential consumer device. diff --git a/drivers/base/property.c b/drivers/base/property.c index a1b01ab42052..afa1bf2b3c5a 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -699,34 +699,6 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode) } EXPORT_SYMBOL_GPL(fwnode_get_next_parent); -/** - * fwnode_get_next_parent_dev - Find device of closest ancestor fwnode - * @fwnode: firmware node - * - * Given a firmware node (@fwnode), this function finds its closest ancestor - * firmware node that has a corresponding struct device and returns that struct - * device. - * - * The caller is responsible for calling put_device() on the returned device - * pointer. - * - * Return: a pointer to the device of the @fwnode's closest ancestor. - */ -struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode) -{ - struct fwnode_handle *parent; - struct device *dev; - - fwnode_for_each_parent_node(fwnode, parent) { - dev = get_dev_from_fwnode(parent); - if (dev) { - fwnode_handle_put(parent); - return dev; - } - } - return NULL; -} - /** * fwnode_count_parents - Return the number of parents a node has * @fwnode: The node the parents of which are to be counted @@ -773,34 +745,6 @@ struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode, } EXPORT_SYMBOL_GPL(fwnode_get_nth_parent); -/** - * fwnode_is_ancestor_of - Test if @ancestor is ancestor of @child - * @ancestor: Firmware which is tested for being an ancestor - * @child: Firmware which is tested for being the child - * - * A node is considered an ancestor of itself too. - * - * Return: true if @ancestor is an ancestor of @child. Otherwise, returns false. - */ -bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child) -{ - struct fwnode_handle *parent; - - if (IS_ERR_OR_NULL(ancestor)) - return false; - - if (child == ancestor) - return true; - - fwnode_for_each_parent_node(child, parent) { - if (parent == ancestor) { - fwnode_handle_put(parent); - return true; - } - } - return false; -} - /** * fwnode_get_next_child_node - Return the next child node handle for a node * @fwnode: Firmware node to find the next child node for. diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h index 0428942093a7..4228c45d5ccc 100644 --- a/include/linux/fwnode.h +++ b/include/linux/fwnode.h @@ -193,7 +193,6 @@ struct fwnode_operations { if (fwnode_has_op(fwnode, op)) \ (fwnode)->ops->op(fwnode, ## __VA_ARGS__); \ } while (false) -#define get_dev_from_fwnode(fwnode) get_device((fwnode)->dev) static inline void fwnode_init(struct fwnode_handle *fwnode, const struct fwnode_operations *ops) diff --git a/include/linux/property.h b/include/linux/property.h index 07fbebc73243..1f0135e24d00 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -150,11 +150,9 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode); for (parent = fwnode_get_parent(fwnode); parent; \ parent = fwnode_get_next_parent(parent)) -struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode); unsigned int fwnode_count_parents(const struct fwnode_handle *fwn); struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn, unsigned int depth); -bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child); struct fwnode_handle *fwnode_get_next_child_node( const struct fwnode_handle *fwnode, struct fwnode_handle *child); struct fwnode_handle *fwnode_get_next_available_child_node(
A few APIs that belong specifically to the fw_devlink APIs - are exposed to others without need - prevents device property code to be cleaned up in the future Resolve this mess by moving fw_devlink code to where it belongs and hide from others. Fixes: b5d3e2fbcb10 ("device property: Add fwnode_is_ancestor_of() and fwnode_get_next_parent_dev()") Fixes: 372a67c0c5ef ("driver core: Add fwnode_to_dev() to look up device from fwnode") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/base/core.c | 58 ++++++++++++++++++++++++++++++++++++++++ drivers/base/property.c | 56 -------------------------------------- include/linux/fwnode.h | 1 - include/linux/property.h | 2 -- 4 files changed, 58 insertions(+), 59 deletions(-)