[PATCH v1 0/5] driver core: fw_devlink: Clean up strings and mutex usages

[PATCH v1 0/5] driver core: fw_devlink: Clean up strings and mutex usages

[Andy Shevchenko]

Replace strlen() + kmalloc() + snprintf() by kasprintf() ond other
string handling improvements.

Use guard() / scoped_guard() to handle fw_devlink mutex.

Andy Shevchenko (5):
  driver core: Sort headers
  driver core: Use kasprintf() instead of fixed buffer formatting
  driver core: Use guards for simple mutex locks
  driver core: Make use of returned value of dev_err_probe()
  driver core: Use 2-argument strscpy()

 drivers/base/core.c | 142 ++++++++++++++++++++------------------------
 1 file changed, 64 insertions(+), 78 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/5] driver core: Sort headers

[Andy Shevchenko]

Sort the headers in alphabetic order in order to ease
the maintenance for this part.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 144cb8c201fb..6b9d3d255135 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -9,29 +9,29 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/blkdev.h>
 #include <linux/cpufreq.h>
 #include <linux/device.h>
+#include <linux/dma-map-ops.h> /* for dma_default_coherent */
 #include <linux/err.h>
 #include <linux/fwnode.h>
 #include <linux/init.h>
+#include <linux/kdev_t.h>
 #include <linux/kstrtox.h>
 #include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/kdev_t.h>
+#include <linux/mutex.h>
+#include <linux/netdevice.h>
 #include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/blkdev.h>
-#include <linux/mutex.h>
 #include <linux/pm_runtime.h>
-#include <linux/netdevice.h>
 #include <linux/rcupdate.h>
-#include <linux/sched/signal.h>
 #include <linux/sched/mm.h>
+#include <linux/sched/signal.h>
+#include <linux/slab.h>
 #include <linux/string_helpers.h>
 #include <linux/swiotlb.h>
 #include <linux/sysfs.h>
-#include <linux/dma-map-ops.h> /* for dma_default_coherent */
 
 #include "base.h"
 #include "physical_location.h"
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 2/5] driver core: Use kasprintf() instead of fixed buffer formatting

[Andy Shevchenko]

Improve readability and maintainability by replacing a hardcoded string
allocation and formatting by the use of the kasprintf() helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/core.c | 70 +++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 38 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6b9d3d255135..f5b5b6bcdf35 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -10,6 +10,7 @@
 
 #include <linux/acpi.h>
 #include <linux/blkdev.h>
+#include <linux/cleanup.h>
 #include <linux/cpufreq.h>
 #include <linux/device.h>
 #include <linux/dma-map-ops.h> /* for dma_default_coherent */
@@ -563,24 +564,11 @@ static struct class devlink_class = {
 
 static int devlink_add_symlinks(struct device *dev)
 {
+	char *buf_con __free(kfree) = NULL, *buf_sup __free(kfree) = NULL;
 	int ret;
-	size_t len;
 	struct device_link *link = to_devlink(dev);
 	struct device *sup = link->supplier;
 	struct device *con = link->consumer;
-	char *buf;
-
-	len = max(strlen(dev_bus_name(sup)) + strlen(dev_name(sup)),
-		  strlen(dev_bus_name(con)) + strlen(dev_name(con)));
-	len += strlen(":");
-	/*
-	 * we kzalloc() memory for symlink name of both supplier and
-	 * consumer, so explicitly take into account both prefix.
-	 */
-	len += max(strlen("supplier:"), strlen("consumer:")) + 1;
-	buf = kzalloc(len, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
 
 	ret = sysfs_create_link(&link->link_dev.kobj, &sup->kobj, "supplier");
 	if (ret)
@@ -590,58 +578,64 @@ static int devlink_add_symlinks(struct device *dev)
 	if (ret)
 		goto err_con;
 
-	snprintf(buf, len, "consumer:%s:%s", dev_bus_name(con), dev_name(con));
-	ret = sysfs_create_link(&sup->kobj, &link->link_dev.kobj, buf);
+	buf_con = kasprintf(GFP_KERNEL, "consumer:%s:%s", dev_bus_name(con), dev_name(con));
+	if (!buf_con) {
+		ret = -ENOMEM;
+		goto err_con_dev;
+	}
+
+	ret = sysfs_create_link(&sup->kobj, &link->link_dev.kobj, buf_con);
 	if (ret)
 		goto err_con_dev;
 
-	snprintf(buf, len, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
-	ret = sysfs_create_link(&con->kobj, &link->link_dev.kobj, buf);
+	buf_sup = kasprintf(GFP_KERNEL, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
+	if (!buf_sup) {
+		ret = -ENOMEM;
+		goto err_sup_dev;
+	}
+
+	ret = sysfs_create_link(&con->kobj, &link->link_dev.kobj, buf_sup);
 	if (ret)
 		goto err_sup_dev;
 
 	goto out;
 
 err_sup_dev:
-	snprintf(buf, len, "consumer:%s:%s", dev_bus_name(con), dev_name(con));
-	sysfs_remove_link(&sup->kobj, buf);
+	sysfs_remove_link(&sup->kobj, buf_con);
 err_con_dev:
 	sysfs_remove_link(&link->link_dev.kobj, "consumer");
 err_con:
 	sysfs_remove_link(&link->link_dev.kobj, "supplier");
 out:
-	kfree(buf);
 	return ret;
 }
 
 static void devlink_remove_symlinks(struct device *dev)
 {
+	char *buf_con __free(kfree) = NULL, *buf_sup __free(kfree) = NULL;
 	struct device_link *link = to_devlink(dev);
-	size_t len;
 	struct device *sup = link->supplier;
 	struct device *con = link->consumer;
-	char *buf;
 
 	sysfs_remove_link(&link->link_dev.kobj, "consumer");
 	sysfs_remove_link(&link->link_dev.kobj, "supplier");
 
-	len = max(strlen(dev_bus_name(sup)) + strlen(dev_name(sup)),
-		  strlen(dev_bus_name(con)) + strlen(dev_name(con)));
-	len += strlen(":");
-	len += max(strlen("supplier:"), strlen("consumer:")) + 1;
-	buf = kzalloc(len, GFP_KERNEL);
-	if (!buf) {
-		WARN(1, "Unable to properly free device link symlinks!\n");
-		return;
+	if (device_is_registered(con)) {
+		buf_sup = kasprintf(GFP_KERNEL, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
+		if (!buf_sup)
+			goto out;
+		sysfs_remove_link(&con->kobj, buf_sup);
 	}
 
-	if (device_is_registered(con)) {
-		snprintf(buf, len, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
-		sysfs_remove_link(&con->kobj, buf);
-	}
-	snprintf(buf, len, "consumer:%s:%s", dev_bus_name(con), dev_name(con));
-	sysfs_remove_link(&sup->kobj, buf);
-	kfree(buf);
+	buf_con = kasprintf(GFP_KERNEL, "consumer:%s:%s", dev_bus_name(con), dev_name(con));
+	if (!buf_con)
+		goto out;
+	sysfs_remove_link(&sup->kobj, buf_con);
+
+	return;
+
+out:
+	WARN(1, "Unable to properly free device link symlinks!\n");
 }
 
 static struct class_interface devlink_class_intf = {
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 3/5] driver core: Use guards for simple mutex locks

[Andy Shevchenko]

Guards can help to make the code more readable. So use it wherever they
do so.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/core.c | 50 ++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index f5b5b6bcdf35..bff5e53ca0ce 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -98,12 +98,9 @@ static int __fwnode_link_add(struct fwnode_handle *con,
 int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
 		    u8 flags)
 {
-	int ret;
+	guard(mutex)(&fwnode_link_lock);
 
-	mutex_lock(&fwnode_link_lock);
-	ret = __fwnode_link_add(con, sup, flags);
-	mutex_unlock(&fwnode_link_lock);
-	return ret;
+	return __fwnode_link_add(con, sup, flags);
 }
 
 /**
@@ -144,10 +141,10 @@ static void fwnode_links_purge_suppliers(struct fwnode_handle *fwnode)
 {
 	struct fwnode_link *link, *tmp;
 
-	mutex_lock(&fwnode_link_lock);
+	guard(mutex)(&fwnode_link_lock);
+
 	list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook)
 		__fwnode_link_del(link);
-	mutex_unlock(&fwnode_link_lock);
 }
 
 /**
@@ -160,10 +157,10 @@ static void fwnode_links_purge_consumers(struct fwnode_handle *fwnode)
 {
 	struct fwnode_link *link, *tmp;
 
-	mutex_lock(&fwnode_link_lock);
+	guard(mutex)(&fwnode_link_lock);
+
 	list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook)
 		__fwnode_link_del(link);
-	mutex_unlock(&fwnode_link_lock);
 }
 
 /**
@@ -1059,20 +1056,16 @@ int device_links_check_suppliers(struct device *dev)
 	 * Device waiting for supplier to become available is not allowed to
 	 * probe.
 	 */
-	mutex_lock(&fwnode_link_lock);
-	sup_fw = fwnode_links_check_suppliers(dev->fwnode);
-	if (sup_fw) {
-		if (!dev_is_best_effort(dev)) {
-			fwnode_ret = -EPROBE_DEFER;
-			dev_err_probe(dev, -EPROBE_DEFER,
-				    "wait for supplier %pfwf\n", sup_fw);
-		} else {
-			fwnode_ret = -EAGAIN;
+	scoped_guard(mutex, &fwnode_link_lock) {
+		sup_fw = fwnode_links_check_suppliers(dev->fwnode);
+		if (sup_fw) {
+			if (dev_is_best_effort(dev))
+				fwnode_ret = -EAGAIN;
+			else
+				return dev_err_probe(dev, -EPROBE_DEFER,
+						     "wait for supplier %pfwf\n", sup_fw);
 		}
 	}
-	mutex_unlock(&fwnode_link_lock);
-	if (fwnode_ret == -EPROBE_DEFER)
-		return fwnode_ret;
 
 	device_links_write_lock();
 
@@ -1247,9 +1240,8 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
 	bool val;
 
 	device_lock(dev);
-	mutex_lock(&fwnode_link_lock);
-	val = !!fwnode_links_check_suppliers(dev->fwnode);
-	mutex_unlock(&fwnode_link_lock);
+	scoped_guard(mutex, &fwnode_link_lock)
+		val = !!fwnode_links_check_suppliers(dev->fwnode);
 	device_unlock(dev);
 	return sysfs_emit(buf, "%u\n", val);
 }
@@ -1322,13 +1314,15 @@ void device_links_driver_bound(struct device *dev)
 	 */
 	if (dev->fwnode && dev->fwnode->dev == dev) {
 		struct fwnode_handle *child;
+
 		fwnode_links_purge_suppliers(dev->fwnode);
-		mutex_lock(&fwnode_link_lock);
+
+		guard(mutex)(&fwnode_link_lock);
+
 		fwnode_for_each_available_child_node(dev->fwnode, child)
 			__fw_devlink_pickup_dangling_consumers(child,
 							       dev->fwnode);
 		__fw_devlink_link_to_consumers(dev);
-		mutex_unlock(&fwnode_link_lock);
 	}
 	device_remove_file(dev, &dev_attr_waiting_for_supplier);
 
@@ -2337,10 +2331,10 @@ static void fw_devlink_link_device(struct device *dev)
 
 	fw_devlink_parse_fwtree(fwnode);
 
-	mutex_lock(&fwnode_link_lock);
+	guard(mutex)(&fwnode_link_lock);
+
 	__fw_devlink_link_to_consumers(dev);
 	__fw_devlink_link_to_suppliers(dev, fwnode);
-	mutex_unlock(&fwnode_link_lock);
 }
 
 /* Device links support end. */
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 4/5] driver core: Make use of returned value of dev_err_probe()

[Andy Shevchenko]

Instead of assigning ret explicitly to the same value that is supplied
to dev_err_probe(), make use  of returned value of the latter.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index bff5e53ca0ce..1e5d7f9bc4f4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1084,10 +1084,8 @@ int device_links_check_suppliers(struct device *dev)
 			}
 
 			device_links_missing_supplier(dev);
-			dev_err_probe(dev, -EPROBE_DEFER,
-				      "supplier %s not ready\n",
-				      dev_name(link->supplier));
-			ret = -EPROBE_DEFER;
+			ret = dev_err_probe(dev, -EPROBE_DEFER,
+					    "supplier %s not ready\n", dev_name(link->supplier));
 			break;
 		}
 		WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 5/5] driver core: Use 2-argument strscpy()

[Andy Shevchenko]

From: Andy Shevchenko <andy.shevchenko@gmail.com>

Use 2-argument strscpy(), which is not only shorter but also provides
an additional check that destination buffer is an array.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 1e5d7f9bc4f4..2a31dc0ed21b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4867,7 +4867,7 @@ set_dev_info(const struct device *dev, struct dev_printk_info *dev_info)
 	else
 		return;
 
-	strscpy(dev_info->subsystem, subsys, sizeof(dev_info->subsystem));
+	strscpy(dev_info->subsystem, subsys);
 
 	/*
 	 * Add device identifier DEVICE=:
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/5] driver core: Sort headers

[quic_zijuhu]

On 8/21/2024 11:48 PM, Andy Shevchenko wrote:
> Sort the headers in alphabetic order in order to ease
> the maintenance for this part.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 144cb8c201fb..6b9d3d255135 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -9,29 +9,29 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/blkdev.h>
>  #include <linux/cpufreq.h>
>  #include <linux/device.h>
> +#include <linux/dma-map-ops.h> /* for dma_default_coherent */
>  #include <linux/err.h>
>  #include <linux/fwnode.h>
>  #include <linux/init.h>
> +#include <linux/kdev_t.h>
>  #include <linux/kstrtox.h>
>  #include <linux/module.h>
> -#include <linux/slab.h>
> -#include <linux/kdev_t.h>
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
>  #include <linux/notifier.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> -#include <linux/blkdev.h>
> -#include <linux/mutex.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/netdevice.h>
>  #include <linux/rcupdate.h>
> -#include <linux/sched/signal.h>
>  #include <linux/sched/mm.h>
> +#include <linux/sched/signal.h>
> +#include <linux/slab.h>
>  #include <linux/string_helpers.h>
>  #include <linux/swiotlb.h>
>  #include <linux/sysfs.h>
> -#include <linux/dma-map-ops.h> /* for dma_default_coherent */
>  
>  #include "base.h"
>  #include "physical_location.h"

Hi Andy,

i don't think it is good idea to sort headers by alphabetic order.

why ?

1) header's dependency is not related to its file (name|path), their
dependency are related to # includes order.

2) it maybe be easy to cause build error.

3) header's path or name maybe be related to subsystem, it is not good
to sort one subsystem's headers before the other.


For header's order, my points is that:

1) sort by their dependency.
   #include <b_header.h>
   #include <a_header.h>
   if
   a_header.h:
   #include <b_header.h>

2) all #include <> block before all #include "" block.

3) sort headers related to source file at the last.

   prefix_xyz.c:

   #include <>
   .....
   #include <prefix_xyz.h>   // it is the last if it is exposed.

   #include "internal_header.h"
   ....

4)
sort relevant header together as far as possible, for example, they
belong to the same subsystem.

Re: [PATCH v1 0/5] driver core: fw_devlink: Clean up strings and mutex usages

[Greg Kroah-Hartman]

On Wed, Aug 21, 2024 at 06:48:18PM +0300, Andy Shevchenko wrote:
> Replace strlen() + kmalloc() + snprintf() by kasprintf() ond other
> string handling improvements.
> 
> Use guard() / scoped_guard() to handle fw_devlink mutex.

Nice clean ups, thanks!

Re: [PATCH v1 1/5] driver core: Sort headers

[Andy Shevchenko]

On Thu, Aug 22, 2024 at 11:30:07AM +0800, quic_zijuhu wrote:
> On 8/21/2024 11:48 PM, Andy Shevchenko wrote:
> > Sort the headers in alphabetic order in order to ease
> > the maintenance for this part.

...

> i don't think it is good idea to sort headers by alphabetic order.

I strongly disagree on this on several points:

- the header dependencies has to be resolved on each header by applying IWYU
  (Include What You Use) principle:

    in this case we don't care what is needed for each header in question

- the end developer shouldn't care about header dependencies changes as
  the project is evolving:

    it's way out of human being capacity to follow _all_ the changes in the Linux
    kernel headers

- it's much easier to maintain the inclusion block when it's sorted (to avoid
  dups, or to see in a fast manner what's already included):

    we are writing code for humans, and not for the machines (leave the
    optimisation task to the compiler in many cases)

- overall it makes the development process much easier as a whole:

    I do not believe there is a single person in the world who may tell you
    the correct order of inclusion to any, even simple, Linux kernel driver

> why ?
> 
> 1) header's dependency is not related to its file (name|path), their
> dependency are related to # includes order.

That's not true. More precisely we are working hard to make it not true (and
it's not a Plan 9 OS where as far as I know the idea was that developer knows
the exact order).

> 2) it maybe be easy to cause build error.

Yes, and again we are trying to avoid this by enforcing IWYU principle.

> 3) header's path or name maybe be related to subsystem, it is not good
> to sort one subsystem's headers before the other.

There is a grouping approach which makes this easier to get. See IIO subsystem
as a prime example for IWYU implementation in the Linux kernel.

> For header's order, my points is that:
> 
> 1) sort by their dependency.

See above. No way, it's completely impractical.

>    #include <b_header.h>
>    #include <a_header.h>
>    if
>    a_header.h:
>    #include <b_header.h>
> 
> 2) all #include <> block before all #include "" block.
> 
> 3) sort headers related to source file at the last.
> 
>    prefix_xyz.c:
> 
>    #include <>
>    .....
>    #include <prefix_xyz.h>   // it is the last if it is exposed.
> 
>    #include "internal_header.h"
>    ....
> 
> 4)
> sort relevant header together as far as possible, for example, they
> belong to the same subsystem.

-- 
With Best Regards,
Andy Shevchenko