[PATCH 0/2] staging: vme_user: Replace printk's & cleanup log messages

[PATCH 0/2] staging: vme_user: Replace printk's & cleanup log messages

[Soumya Negi]

Staging driver vme_user has a bunch of printk() calls in vme.c which
triggers checkpatch warnings. Remove all printk's and change them to
the appropriate logging mechanism i.e pr_err()/pr_warn(), or 
dev_err()/dev_warn().

Also, cleanup the messages further by using __func__ in the string 
instead of function names.

This patchset fixes all checkpatch warnings like:
 
    WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then
             dev_err(dev, ... then pr_err(...  to printk(KERN_ERR ...
&
    WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
             this function's name, in a string

Patches must be applied in order.

Soumya Negi (2):
  staging: vme_user: Replace printk() with pr_*(),dev_*()
  staging: vme_user: Use __func__ instead of function name

 drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
 1 file changed, 94 insertions(+), 81 deletions(-)

-- 
2.42.0

[PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Soumya Negi]

vme.c uses printk() to log messages. To improve and standardize message
formatting, use logging mechanisms pr_err()/pr_warn() and
dev_err()/dev_warn() instead. Retain the printk log levels of the
messages during replacement.

Issue found by checkpatch.pl

Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
---
 drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
 1 file changed, 94 insertions(+), 81 deletions(-)

diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
index 6519a7c994a0..e8c2c1e77b7d 100644
--- a/drivers/staging/vme_user/vme.c
+++ b/drivers/staging/vme_user/vme.c
@@ -9,6 +9,8 @@
  * Copyright 2004 Motorola Inc.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/init.h>
 #include <linux/export.h>
 #include <linux/mm.h>
@@ -62,7 +64,7 @@ static struct vme_bridge *find_bridge(struct vme_resource *resource)
 		return list_entry(resource->entry, struct vme_lm_resource,
 			list)->parent;
 	default:
-		printk(KERN_ERR "Unknown resource type\n");
+		pr_err("Unknown resource type\n");
 		return NULL;
 	}
 }
@@ -84,24 +86,25 @@ void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
 	struct vme_bridge *bridge;
 
 	if (!resource) {
-		printk(KERN_ERR "No resource\n");
+		pr_err("No resource\n");
 		return NULL;
 	}
 
 	bridge = find_bridge(resource);
 	if (!bridge) {
-		printk(KERN_ERR "Can't find bridge\n");
+		pr_err("Can't find bridge\n");
 		return NULL;
 	}
 
 	if (!bridge->parent) {
-		printk(KERN_ERR "Dev entry NULL for bridge %s\n", bridge->name);
+		pr_err("Dev entry NULL for bridge %s\n", bridge->name);
 		return NULL;
 	}
 
 	if (!bridge->alloc_consistent) {
-		printk(KERN_ERR "alloc_consistent not supported by bridge %s\n",
-		       bridge->name);
+		dev_err(bridge->parent,
+			"alloc_consistent not supported by bridge %s\n",
+			bridge->name);
 		return NULL;
 	}
 
@@ -124,24 +127,25 @@ void vme_free_consistent(struct vme_resource *resource, size_t size,
 	struct vme_bridge *bridge;
 
 	if (!resource) {
-		printk(KERN_ERR "No resource\n");
+		pr_err("No resource\n");
 		return;
 	}
 
 	bridge = find_bridge(resource);
 	if (!bridge) {
-		printk(KERN_ERR "Can't find bridge\n");
+		pr_err("Can't find bridge\n");
 		return;
 	}
 
 	if (!bridge->parent) {
-		printk(KERN_ERR "Dev entry NULL for bridge %s\n", bridge->name);
+		pr_err("Dev entry NULL for bridge %s\n", bridge->name);
 		return;
 	}
 
 	if (!bridge->free_consistent) {
-		printk(KERN_ERR "free_consistent not supported by bridge %s\n",
-		       bridge->name);
+		dev_err(bridge->parent,
+			"free_consistent not supported by bridge %s\n",
+			bridge->name);
 		return;
 	}
 
@@ -184,7 +188,7 @@ size_t vme_get_size(struct vme_resource *resource)
 	case VME_DMA:
 		return 0;
 	default:
-		printk(KERN_ERR "Unknown resource type\n");
+		pr_err("Unknown resource type\n");
 		return 0;
 	}
 }
@@ -225,7 +229,7 @@ int vme_check_window(u32 aspace, unsigned long long vme_base,
 		/* User Defined */
 		break;
 	default:
-		printk(KERN_ERR "Invalid address space\n");
+		pr_err("Invalid address space\n");
 		retval = -EINVAL;
 		break;
 	}
@@ -288,14 +292,14 @@ struct vme_resource *vme_slave_request(struct vme_dev *vdev, u32 address,
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		goto err_bus;
 	}
 
 	/* Loop through slave resources */
 	list_for_each_entry(slave_image, &bridge->slave_resources, list) {
 		if (!slave_image) {
-			printk(KERN_ERR "Registered NULL Slave resource\n");
+			dev_err(bridge->parent, "Registered NULL Slave resource\n");
 			continue;
 		}
 
@@ -362,20 +366,20 @@ int vme_slave_set(struct vme_resource *resource, int enabled,
 	int retval;
 
 	if (resource->type != VME_SLAVE) {
-		printk(KERN_ERR "Not a slave resource\n");
+		pr_err("Not a slave resource\n");
 		return -EINVAL;
 	}
 
 	image = list_entry(resource->entry, struct vme_slave_resource, list);
 
 	if (!bridge->slave_set) {
-		printk(KERN_ERR "Function not supported\n");
+		dev_err(bridge->parent, "Function not supported\n");
 		return -ENOSYS;
 	}
 
 	if (!(((image->address_attr & aspace) == aspace) &&
 	      ((image->cycle_attr & cycle) == cycle))) {
-		printk(KERN_ERR "Invalid attributes\n");
+		pr_err("Invalid attributes\n");
 		return -EINVAL;
 	}
 
@@ -411,14 +415,14 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
 	struct vme_slave_resource *image;
 
 	if (resource->type != VME_SLAVE) {
-		printk(KERN_ERR "Not a slave resource\n");
+		pr_err("Not a slave resource\n");
 		return -EINVAL;
 	}
 
 	image = list_entry(resource->entry, struct vme_slave_resource, list);
 
 	if (!bridge->slave_get) {
-		printk(KERN_ERR "vme_slave_get not supported\n");
+		dev_err(bridge->parent, "vme_slave_get not supported\n");
 		return -EINVAL;
 	}
 
@@ -438,21 +442,21 @@ void vme_slave_free(struct vme_resource *resource)
 	struct vme_slave_resource *slave_image;
 
 	if (resource->type != VME_SLAVE) {
-		printk(KERN_ERR "Not a slave resource\n");
+		pr_err("Not a slave resource\n");
 		return;
 	}
 
 	slave_image = list_entry(resource->entry, struct vme_slave_resource,
 				 list);
 	if (!slave_image) {
-		printk(KERN_ERR "Can't find slave resource\n");
+		pr_err("Can't find slave resource\n");
 		return;
 	}
 
 	/* Unlock image */
 	mutex_lock(&slave_image->mtx);
 	if (slave_image->locked == 0)
-		printk(KERN_ERR "Image is already free\n");
+		pr_err("Image is already free\n");
 
 	slave_image->locked = 0;
 	mutex_unlock(&slave_image->mtx);
@@ -484,14 +488,14 @@ struct vme_resource *vme_master_request(struct vme_dev *vdev, u32 address,
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		goto err_bus;
 	}
 
 	/* Loop through master resources */
 	list_for_each_entry(master_image, &bridge->master_resources, list) {
 		if (!master_image) {
-			printk(KERN_WARNING "Registered NULL master resource\n");
+			dev_warn(bridge->parent, "Registered NULL master resource\n");
 			continue;
 		}
 
@@ -511,7 +515,7 @@ struct vme_resource *vme_master_request(struct vme_dev *vdev, u32 address,
 
 	/* Check to see if we found a resource */
 	if (!allocated_image) {
-		printk(KERN_ERR "Can't find a suitable resource\n");
+		dev_err(&vdev->dev, "Can't find a suitable resource\n");
 		goto err_image;
 	}
 
@@ -561,21 +565,21 @@ int vme_master_set(struct vme_resource *resource, int enabled,
 	int retval;
 
 	if (resource->type != VME_MASTER) {
-		printk(KERN_ERR "Not a master resource\n");
+		pr_err("Not a master resource\n");
 		return -EINVAL;
 	}
 
 	image = list_entry(resource->entry, struct vme_master_resource, list);
 
 	if (!bridge->master_set) {
-		printk(KERN_WARNING "vme_master_set not supported\n");
+		dev_warn(bridge->parent, "vme_master_set not supported\n");
 		return -EINVAL;
 	}
 
 	if (!(((image->address_attr & aspace) == aspace) &&
 	      ((image->cycle_attr & cycle) == cycle) &&
 	      ((image->width_attr & dwidth) == dwidth))) {
-		printk(KERN_WARNING "Invalid attributes\n");
+		pr_warn("Invalid attributes\n");
 		return -EINVAL;
 	}
 
@@ -611,14 +615,14 @@ int vme_master_get(struct vme_resource *resource, int *enabled,
 	struct vme_master_resource *image;
 
 	if (resource->type != VME_MASTER) {
-		printk(KERN_ERR "Not a master resource\n");
+		pr_err("Not a master resource\n");
 		return -EINVAL;
 	}
 
 	image = list_entry(resource->entry, struct vme_master_resource, list);
 
 	if (!bridge->master_get) {
-		printk(KERN_WARNING "%s not supported\n", __func__);
+		dev_warn(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
@@ -650,12 +654,12 @@ ssize_t vme_master_read(struct vme_resource *resource, void *buf, size_t count,
 	size_t length;
 
 	if (!bridge->master_read) {
-		printk(KERN_WARNING "Reading from resource not supported\n");
+		dev_warn(bridge->parent, "Reading from resource not supported\n");
 		return -EINVAL;
 	}
 
 	if (resource->type != VME_MASTER) {
-		printk(KERN_ERR "Not a master resource\n");
+		pr_err("Not a master resource\n");
 		return -EINVAL;
 	}
 
@@ -664,7 +668,7 @@ ssize_t vme_master_read(struct vme_resource *resource, void *buf, size_t count,
 	length = vme_get_size(resource);
 
 	if (offset > length) {
-		printk(KERN_WARNING "Invalid Offset\n");
+		pr_warn("Invalid Offset\n");
 		return -EFAULT;
 	}
 
@@ -698,12 +702,12 @@ ssize_t vme_master_write(struct vme_resource *resource, void *buf,
 	size_t length;
 
 	if (!bridge->master_write) {
-		printk(KERN_WARNING "Writing to resource not supported\n");
+		dev_warn(bridge->parent, "Writing to resource not supported\n");
 		return -EINVAL;
 	}
 
 	if (resource->type != VME_MASTER) {
-		printk(KERN_ERR "Not a master resource\n");
+		pr_err("Not a master resource\n");
 		return -EINVAL;
 	}
 
@@ -712,7 +716,7 @@ ssize_t vme_master_write(struct vme_resource *resource, void *buf,
 	length = vme_get_size(resource);
 
 	if (offset > length) {
-		printk(KERN_WARNING "Invalid Offset\n");
+		pr_warn("Invalid Offset\n");
 		return -EFAULT;
 	}
 
@@ -749,12 +753,12 @@ unsigned int vme_master_rmw(struct vme_resource *resource, unsigned int mask,
 	struct vme_master_resource *image;
 
 	if (!bridge->master_rmw) {
-		printk(KERN_WARNING "Writing to resource not supported\n");
+		dev_warn(bridge->parent, "Writing to resource not supported\n");
 		return -EINVAL;
 	}
 
 	if (resource->type != VME_MASTER) {
-		printk(KERN_ERR "Not a master resource\n");
+		pr_err("Not a master resource\n");
 		return -EINVAL;
 	}
 
@@ -812,21 +816,21 @@ void vme_master_free(struct vme_resource *resource)
 	struct vme_master_resource *master_image;
 
 	if (resource->type != VME_MASTER) {
-		printk(KERN_ERR "Not a master resource\n");
+		pr_err("Not a master resource\n");
 		return;
 	}
 
 	master_image = list_entry(resource->entry, struct vme_master_resource,
 				  list);
 	if (!master_image) {
-		printk(KERN_ERR "Can't find master resource\n");
+		pr_err("Can't find master resource\n");
 		return;
 	}
 
 	/* Unlock image */
 	spin_lock(&master_image->lock);
 	if (master_image->locked == 0)
-		printk(KERN_ERR "Image is already free\n");
+		pr_err("Image is already free\n");
 
 	master_image->locked = 0;
 	spin_unlock(&master_image->lock);
@@ -854,18 +858,19 @@ struct vme_resource *vme_dma_request(struct vme_dev *vdev, u32 route)
 	struct vme_resource *resource = NULL;
 
 	/* XXX Not checking resource attributes */
-	printk(KERN_ERR "No VME resource Attribute tests done\n");
+	pr_err("No VME resource Attribute tests done\n");
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		goto err_bus;
 	}
 
 	/* Loop through DMA resources */
 	list_for_each_entry(dma_ctrlr, &bridge->dma_resources, list) {
 		if (!dma_ctrlr) {
-			printk(KERN_ERR "Registered NULL DMA resource\n");
+			dev_err(bridge->parent,
+				"Registered NULL DMA resource\n");
 			continue;
 		}
 
@@ -920,7 +925,7 @@ struct vme_dma_list *vme_new_dma_list(struct vme_resource *resource)
 	struct vme_dma_list *dma_list;
 
 	if (resource->type != VME_DMA) {
-		printk(KERN_ERR "Not a DMA resource\n");
+		pr_err("Not a DMA resource\n");
 		return NULL;
 	}
 
@@ -1098,12 +1103,13 @@ int vme_dma_list_add(struct vme_dma_list *list, struct vme_dma_attr *src,
 	int retval;
 
 	if (!bridge->dma_list_add) {
-		printk(KERN_WARNING "Link List DMA generation not supported\n");
+		dev_warn(bridge->parent,
+			 "Link List DMA generation not supported\n");
 		return -EINVAL;
 	}
 
 	if (!mutex_trylock(&list->mtx)) {
-		printk(KERN_ERR "Link List already submitted\n");
+		pr_err("Link List already submitted\n");
 		return -EINVAL;
 	}
 
@@ -1131,7 +1137,8 @@ int vme_dma_list_exec(struct vme_dma_list *list)
 	int retval;
 
 	if (!bridge->dma_list_exec) {
-		printk(KERN_ERR "Link List DMA execution not supported\n");
+		dev_err(bridge->parent,
+			"Link List DMA execution not supported\n");
 		return -EINVAL;
 	}
 
@@ -1160,12 +1167,13 @@ int vme_dma_list_free(struct vme_dma_list *list)
 	int retval;
 
 	if (!bridge->dma_list_empty) {
-		printk(KERN_WARNING "Emptying of Link Lists not supported\n");
+		dev_warn(bridge->parent,
+			 "Emptying of Link Lists not supported\n");
 		return -EINVAL;
 	}
 
 	if (!mutex_trylock(&list->mtx)) {
-		printk(KERN_ERR "Link List in use\n");
+		pr_err("Link List in use\n");
 		return -EBUSY;
 	}
 
@@ -1175,7 +1183,7 @@ int vme_dma_list_free(struct vme_dma_list *list)
 	 */
 	retval = bridge->dma_list_empty(list);
 	if (retval) {
-		printk(KERN_ERR "Unable to empty link-list entries\n");
+		pr_err("Unable to empty link-list entries\n");
 		mutex_unlock(&list->mtx);
 		return retval;
 	}
@@ -1200,19 +1208,19 @@ int vme_dma_free(struct vme_resource *resource)
 	struct vme_dma_resource *ctrlr;
 
 	if (resource->type != VME_DMA) {
-		printk(KERN_ERR "Not a DMA resource\n");
+		pr_err("Not a DMA resource\n");
 		return -EINVAL;
 	}
 
 	ctrlr = list_entry(resource->entry, struct vme_dma_resource, list);
 
 	if (!mutex_trylock(&ctrlr->mtx)) {
-		printk(KERN_ERR "Resource busy, can't free\n");
+		pr_err("Resource busy, can't free\n");
 		return -EBUSY;
 	}
 
 	if (!(list_empty(&ctrlr->pending) && list_empty(&ctrlr->running))) {
-		printk(KERN_WARNING "Resource still processing transfers\n");
+		pr_warn("Resource still processing transfers\n");
 		mutex_unlock(&ctrlr->mtx);
 		return -EBUSY;
 	}
@@ -1290,8 +1298,9 @@ void vme_irq_handler(struct vme_bridge *bridge, int level, int statid)
 	if (call)
 		call(level, statid, priv_data);
 	else
-		printk(KERN_WARNING "Spurious VME interrupt, level:%x, vector:%x\n",
-		       level, statid);
+		dev_warn(bridge->parent,
+			 "Spurious VME interrupt, level:%x, vector:%x\n", level,
+			 statid);
 }
 EXPORT_SYMBOL(vme_irq_handler);
 
@@ -1319,17 +1328,18 @@ int vme_irq_request(struct vme_dev *vdev, int level, int statid,
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		return -EINVAL;
 	}
 
 	if ((level < 1) || (level > 7)) {
-		printk(KERN_ERR "Invalid interrupt level\n");
+		pr_err("Invalid interrupt level\n");
 		return -EINVAL;
 	}
 
 	if (!bridge->irq_set) {
-		printk(KERN_ERR "Configuring interrupts not supported\n");
+		dev_err(bridge->parent,
+			"Configuring interrupts not supported\n");
 		return -EINVAL;
 	}
 
@@ -1337,7 +1347,7 @@ int vme_irq_request(struct vme_dev *vdev, int level, int statid,
 
 	if (bridge->irq[level - 1].callback[statid].func) {
 		mutex_unlock(&bridge->irq_mtx);
-		printk(KERN_WARNING "VME Interrupt already taken\n");
+		dev_warn(bridge->parent, "VME Interrupt already taken\n");
 		return -EBUSY;
 	}
 
@@ -1368,17 +1378,18 @@ void vme_irq_free(struct vme_dev *vdev, int level, int statid)
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		return;
 	}
 
 	if ((level < 1) || (level > 7)) {
-		printk(KERN_ERR "Invalid interrupt level\n");
+		pr_err("Invalid interrupt level\n");
 		return;
 	}
 
 	if (!bridge->irq_set) {
-		printk(KERN_ERR "Configuring interrupts not supported\n");
+		dev_err(bridge->parent,
+			"Configuring interrupts not supported\n");
 		return;
 	}
 
@@ -1415,17 +1426,18 @@ int vme_irq_generate(struct vme_dev *vdev, int level, int statid)
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		return -EINVAL;
 	}
 
 	if ((level < 1) || (level > 7)) {
-		printk(KERN_WARNING "Invalid interrupt level\n");
+		pr_warn("Invalid interrupt level\n");
 		return -EINVAL;
 	}
 
 	if (!bridge->irq_generate) {
-		printk(KERN_WARNING "Interrupt generation not supported\n");
+		dev_warn(bridge->parent,
+			 "Interrupt generation not supported\n");
 		return -EINVAL;
 	}
 
@@ -1452,14 +1464,15 @@ struct vme_resource *vme_lm_request(struct vme_dev *vdev)
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		goto err_bus;
 	}
 
 	/* Loop through LM resources */
 	list_for_each_entry(lm, &bridge->lm_resources, list) {
 		if (!lm) {
-			printk(KERN_ERR "Registered NULL Location Monitor resource\n");
+			dev_err(bridge->parent,
+				"Registered NULL Location Monitor resource\n");
 			continue;
 		}
 
@@ -1514,7 +1527,7 @@ int vme_lm_count(struct vme_resource *resource)
 	struct vme_lm_resource *lm;
 
 	if (resource->type != VME_LM) {
-		printk(KERN_ERR "Not a Location Monitor resource\n");
+		pr_err("Not a Location Monitor resource\n");
 		return -EINVAL;
 	}
 
@@ -1545,14 +1558,14 @@ int vme_lm_set(struct vme_resource *resource, unsigned long long lm_base,
 	struct vme_lm_resource *lm;
 
 	if (resource->type != VME_LM) {
-		printk(KERN_ERR "Not a Location Monitor resource\n");
+		pr_err("Not a Location Monitor resource\n");
 		return -EINVAL;
 	}
 
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_set) {
-		printk(KERN_ERR "vme_lm_set not supported\n");
+		dev_err(bridge->parent, "vme_lm_set not supported\n");
 		return -EINVAL;
 	}
 
@@ -1581,14 +1594,14 @@ int vme_lm_get(struct vme_resource *resource, unsigned long long *lm_base,
 	struct vme_lm_resource *lm;
 
 	if (resource->type != VME_LM) {
-		printk(KERN_ERR "Not a Location Monitor resource\n");
+		pr_err("Not a Location Monitor resource\n");
 		return -EINVAL;
 	}
 
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_get) {
-		printk(KERN_ERR "vme_lm_get not supported\n");
+		dev_err(bridge->parent, "vme_lm_get not supported\n");
 		return -EINVAL;
 	}
 
@@ -1618,14 +1631,14 @@ int vme_lm_attach(struct vme_resource *resource, int monitor,
 	struct vme_lm_resource *lm;
 
 	if (resource->type != VME_LM) {
-		printk(KERN_ERR "Not a Location Monitor resource\n");
+		pr_err("Not a Location Monitor resource\n");
 		return -EINVAL;
 	}
 
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_attach) {
-		printk(KERN_ERR "vme_lm_attach not supported\n");
+		dev_err(bridge->parent, "vme_lm_attach not supported\n");
 		return -EINVAL;
 	}
 
@@ -1651,14 +1664,14 @@ int vme_lm_detach(struct vme_resource *resource, int monitor)
 	struct vme_lm_resource *lm;
 
 	if (resource->type != VME_LM) {
-		printk(KERN_ERR "Not a Location Monitor resource\n");
+		pr_err("Not a Location Monitor resource\n");
 		return -EINVAL;
 	}
 
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_detach) {
-		printk(KERN_ERR "vme_lm_detach not supported\n");
+		dev_err(bridge->parent, "vme_lm_detach not supported\n");
 		return -EINVAL;
 	}
 
@@ -1683,7 +1696,7 @@ void vme_lm_free(struct vme_resource *resource)
 	struct vme_lm_resource *lm;
 
 	if (resource->type != VME_LM) {
-		printk(KERN_ERR "Not a Location Monitor resource\n");
+		pr_err("Not a Location Monitor resource\n");
 		return;
 	}
 
@@ -1720,12 +1733,12 @@ int vme_slot_num(struct vme_dev *vdev)
 
 	bridge = vdev->bridge;
 	if (!bridge) {
-		printk(KERN_ERR "Can't find VME bus\n");
+		pr_err("Can't find VME bus\n");
 		return -EINVAL;
 	}
 
 	if (!bridge->slot_get) {
-		printk(KERN_WARNING "vme_slot_num not supported\n");
+		dev_warn(bridge->parent, "vme_slot_num not supported\n");
 		return -EINVAL;
 	}
 
-- 
2.42.0

[PATCH 2/2] staging: vme_user: Use __func__ instead of function name

[Soumya Negi]

Replace function names in message strings with __func__ to fix
all checkpatch warnings like:

    WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
             this function's name, in a string

Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
---
 drivers/staging/vme_user/vme.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
index e8c2c1e77b7d..11c1df12b657 100644
--- a/drivers/staging/vme_user/vme.c
+++ b/drivers/staging/vme_user/vme.c
@@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
 	image = list_entry(resource->entry, struct vme_slave_resource, list);
 
 	if (!bridge->slave_get) {
-		dev_err(bridge->parent, "vme_slave_get not supported\n");
+		dev_err(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
@@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
 	image = list_entry(resource->entry, struct vme_master_resource, list);
 
 	if (!bridge->master_set) {
-		dev_warn(bridge->parent, "vme_master_set not supported\n");
+		dev_warn(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
@@ -1565,7 +1565,7 @@ int vme_lm_set(struct vme_resource *resource, unsigned long long lm_base,
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_set) {
-		dev_err(bridge->parent, "vme_lm_set not supported\n");
+		dev_err(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
@@ -1601,7 +1601,7 @@ int vme_lm_get(struct vme_resource *resource, unsigned long long *lm_base,
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_get) {
-		dev_err(bridge->parent, "vme_lm_get not supported\n");
+		dev_err(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
@@ -1638,7 +1638,7 @@ int vme_lm_attach(struct vme_resource *resource, int monitor,
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_attach) {
-		dev_err(bridge->parent, "vme_lm_attach not supported\n");
+		dev_err(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
@@ -1671,7 +1671,7 @@ int vme_lm_detach(struct vme_resource *resource, int monitor)
 	lm = list_entry(resource->entry, struct vme_lm_resource, list);
 
 	if (!bridge->lm_detach) {
-		dev_err(bridge->parent, "vme_lm_detach not supported\n");
+		dev_err(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
@@ -1738,7 +1738,7 @@ int vme_slot_num(struct vme_dev *vdev)
 	}
 
 	if (!bridge->slot_get) {
-		dev_warn(bridge->parent, "vme_slot_num not supported\n");
+		dev_warn(bridge->parent, "%s not supported\n", __func__);
 		return -EINVAL;
 	}
 
-- 
2.42.0

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Julia Lawall]

On Tue, 17 Oct 2023, Soumya Negi wrote:

> vme.c uses printk() to log messages. To improve and standardize message
> formatting, use logging mechanisms pr_err()/pr_warn() and
> dev_err()/dev_warn() instead. Retain the printk log levels of the
> messages during replacement.

It's not possible to use the dev_ functions more often?  The pr_functions
don't give context information, and the message don't seem to give much
context information either.

julia


>
> Issue found by checkpatch.pl
>
> Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> ---
>  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
>  1 file changed, 94 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> index 6519a7c994a0..e8c2c1e77b7d 100644
> --- a/drivers/staging/vme_user/vme.c
> +++ b/drivers/staging/vme_user/vme.c
> @@ -9,6 +9,8 @@
>   * Copyright 2004 Motorola Inc.
>   */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/init.h>
>  #include <linux/export.h>
>  #include <linux/mm.h>
> @@ -62,7 +64,7 @@ static struct vme_bridge *find_bridge(struct vme_resource *resource)
>  		return list_entry(resource->entry, struct vme_lm_resource,
>  			list)->parent;
>  	default:
> -		printk(KERN_ERR "Unknown resource type\n");
> +		pr_err("Unknown resource type\n");
>  		return NULL;
>  	}
>  }
> @@ -84,24 +86,25 @@ void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
>  	struct vme_bridge *bridge;
>
>  	if (!resource) {
> -		printk(KERN_ERR "No resource\n");
> +		pr_err("No resource\n");
>  		return NULL;
>  	}
>
>  	bridge = find_bridge(resource);
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find bridge\n");
> +		pr_err("Can't find bridge\n");
>  		return NULL;
>  	}
>
>  	if (!bridge->parent) {
> -		printk(KERN_ERR "Dev entry NULL for bridge %s\n", bridge->name);
> +		pr_err("Dev entry NULL for bridge %s\n", bridge->name);
>  		return NULL;
>  	}
>
>  	if (!bridge->alloc_consistent) {
> -		printk(KERN_ERR "alloc_consistent not supported by bridge %s\n",
> -		       bridge->name);
> +		dev_err(bridge->parent,
> +			"alloc_consistent not supported by bridge %s\n",
> +			bridge->name);
>  		return NULL;
>  	}
>
> @@ -124,24 +127,25 @@ void vme_free_consistent(struct vme_resource *resource, size_t size,
>  	struct vme_bridge *bridge;
>
>  	if (!resource) {
> -		printk(KERN_ERR "No resource\n");
> +		pr_err("No resource\n");
>  		return;
>  	}
>
>  	bridge = find_bridge(resource);
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find bridge\n");
> +		pr_err("Can't find bridge\n");
>  		return;
>  	}
>
>  	if (!bridge->parent) {
> -		printk(KERN_ERR "Dev entry NULL for bridge %s\n", bridge->name);
> +		pr_err("Dev entry NULL for bridge %s\n", bridge->name);
>  		return;
>  	}
>
>  	if (!bridge->free_consistent) {
> -		printk(KERN_ERR "free_consistent not supported by bridge %s\n",
> -		       bridge->name);
> +		dev_err(bridge->parent,
> +			"free_consistent not supported by bridge %s\n",
> +			bridge->name);
>  		return;
>  	}
>
> @@ -184,7 +188,7 @@ size_t vme_get_size(struct vme_resource *resource)
>  	case VME_DMA:
>  		return 0;
>  	default:
> -		printk(KERN_ERR "Unknown resource type\n");
> +		pr_err("Unknown resource type\n");
>  		return 0;
>  	}
>  }
> @@ -225,7 +229,7 @@ int vme_check_window(u32 aspace, unsigned long long vme_base,
>  		/* User Defined */
>  		break;
>  	default:
> -		printk(KERN_ERR "Invalid address space\n");
> +		pr_err("Invalid address space\n");
>  		retval = -EINVAL;
>  		break;
>  	}
> @@ -288,14 +292,14 @@ struct vme_resource *vme_slave_request(struct vme_dev *vdev, u32 address,
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		goto err_bus;
>  	}
>
>  	/* Loop through slave resources */
>  	list_for_each_entry(slave_image, &bridge->slave_resources, list) {
>  		if (!slave_image) {
> -			printk(KERN_ERR "Registered NULL Slave resource\n");
> +			dev_err(bridge->parent, "Registered NULL Slave resource\n");
>  			continue;
>  		}
>
> @@ -362,20 +366,20 @@ int vme_slave_set(struct vme_resource *resource, int enabled,
>  	int retval;
>
>  	if (resource->type != VME_SLAVE) {
> -		printk(KERN_ERR "Not a slave resource\n");
> +		pr_err("Not a slave resource\n");
>  		return -EINVAL;
>  	}
>
>  	image = list_entry(resource->entry, struct vme_slave_resource, list);
>
>  	if (!bridge->slave_set) {
> -		printk(KERN_ERR "Function not supported\n");
> +		dev_err(bridge->parent, "Function not supported\n");
>  		return -ENOSYS;
>  	}
>
>  	if (!(((image->address_attr & aspace) == aspace) &&
>  	      ((image->cycle_attr & cycle) == cycle))) {
> -		printk(KERN_ERR "Invalid attributes\n");
> +		pr_err("Invalid attributes\n");
>  		return -EINVAL;
>  	}
>
> @@ -411,14 +415,14 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
>  	struct vme_slave_resource *image;
>
>  	if (resource->type != VME_SLAVE) {
> -		printk(KERN_ERR "Not a slave resource\n");
> +		pr_err("Not a slave resource\n");
>  		return -EINVAL;
>  	}
>
>  	image = list_entry(resource->entry, struct vme_slave_resource, list);
>
>  	if (!bridge->slave_get) {
> -		printk(KERN_ERR "vme_slave_get not supported\n");
> +		dev_err(bridge->parent, "vme_slave_get not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -438,21 +442,21 @@ void vme_slave_free(struct vme_resource *resource)
>  	struct vme_slave_resource *slave_image;
>
>  	if (resource->type != VME_SLAVE) {
> -		printk(KERN_ERR "Not a slave resource\n");
> +		pr_err("Not a slave resource\n");
>  		return;
>  	}
>
>  	slave_image = list_entry(resource->entry, struct vme_slave_resource,
>  				 list);
>  	if (!slave_image) {
> -		printk(KERN_ERR "Can't find slave resource\n");
> +		pr_err("Can't find slave resource\n");
>  		return;
>  	}
>
>  	/* Unlock image */
>  	mutex_lock(&slave_image->mtx);
>  	if (slave_image->locked == 0)
> -		printk(KERN_ERR "Image is already free\n");
> +		pr_err("Image is already free\n");
>
>  	slave_image->locked = 0;
>  	mutex_unlock(&slave_image->mtx);
> @@ -484,14 +488,14 @@ struct vme_resource *vme_master_request(struct vme_dev *vdev, u32 address,
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		goto err_bus;
>  	}
>
>  	/* Loop through master resources */
>  	list_for_each_entry(master_image, &bridge->master_resources, list) {
>  		if (!master_image) {
> -			printk(KERN_WARNING "Registered NULL master resource\n");
> +			dev_warn(bridge->parent, "Registered NULL master resource\n");
>  			continue;
>  		}
>
> @@ -511,7 +515,7 @@ struct vme_resource *vme_master_request(struct vme_dev *vdev, u32 address,
>
>  	/* Check to see if we found a resource */
>  	if (!allocated_image) {
> -		printk(KERN_ERR "Can't find a suitable resource\n");
> +		dev_err(&vdev->dev, "Can't find a suitable resource\n");
>  		goto err_image;
>  	}
>
> @@ -561,21 +565,21 @@ int vme_master_set(struct vme_resource *resource, int enabled,
>  	int retval;
>
>  	if (resource->type != VME_MASTER) {
> -		printk(KERN_ERR "Not a master resource\n");
> +		pr_err("Not a master resource\n");
>  		return -EINVAL;
>  	}
>
>  	image = list_entry(resource->entry, struct vme_master_resource, list);
>
>  	if (!bridge->master_set) {
> -		printk(KERN_WARNING "vme_master_set not supported\n");
> +		dev_warn(bridge->parent, "vme_master_set not supported\n");
>  		return -EINVAL;
>  	}
>
>  	if (!(((image->address_attr & aspace) == aspace) &&
>  	      ((image->cycle_attr & cycle) == cycle) &&
>  	      ((image->width_attr & dwidth) == dwidth))) {
> -		printk(KERN_WARNING "Invalid attributes\n");
> +		pr_warn("Invalid attributes\n");
>  		return -EINVAL;
>  	}
>
> @@ -611,14 +615,14 @@ int vme_master_get(struct vme_resource *resource, int *enabled,
>  	struct vme_master_resource *image;
>
>  	if (resource->type != VME_MASTER) {
> -		printk(KERN_ERR "Not a master resource\n");
> +		pr_err("Not a master resource\n");
>  		return -EINVAL;
>  	}
>
>  	image = list_entry(resource->entry, struct vme_master_resource, list);
>
>  	if (!bridge->master_get) {
> -		printk(KERN_WARNING "%s not supported\n", __func__);
> +		dev_warn(bridge->parent, "%s not supported\n", __func__);
>  		return -EINVAL;
>  	}
>
> @@ -650,12 +654,12 @@ ssize_t vme_master_read(struct vme_resource *resource, void *buf, size_t count,
>  	size_t length;
>
>  	if (!bridge->master_read) {
> -		printk(KERN_WARNING "Reading from resource not supported\n");
> +		dev_warn(bridge->parent, "Reading from resource not supported\n");
>  		return -EINVAL;
>  	}
>
>  	if (resource->type != VME_MASTER) {
> -		printk(KERN_ERR "Not a master resource\n");
> +		pr_err("Not a master resource\n");
>  		return -EINVAL;
>  	}
>
> @@ -664,7 +668,7 @@ ssize_t vme_master_read(struct vme_resource *resource, void *buf, size_t count,
>  	length = vme_get_size(resource);
>
>  	if (offset > length) {
> -		printk(KERN_WARNING "Invalid Offset\n");
> +		pr_warn("Invalid Offset\n");
>  		return -EFAULT;
>  	}
>
> @@ -698,12 +702,12 @@ ssize_t vme_master_write(struct vme_resource *resource, void *buf,
>  	size_t length;
>
>  	if (!bridge->master_write) {
> -		printk(KERN_WARNING "Writing to resource not supported\n");
> +		dev_warn(bridge->parent, "Writing to resource not supported\n");
>  		return -EINVAL;
>  	}
>
>  	if (resource->type != VME_MASTER) {
> -		printk(KERN_ERR "Not a master resource\n");
> +		pr_err("Not a master resource\n");
>  		return -EINVAL;
>  	}
>
> @@ -712,7 +716,7 @@ ssize_t vme_master_write(struct vme_resource *resource, void *buf,
>  	length = vme_get_size(resource);
>
>  	if (offset > length) {
> -		printk(KERN_WARNING "Invalid Offset\n");
> +		pr_warn("Invalid Offset\n");
>  		return -EFAULT;
>  	}
>
> @@ -749,12 +753,12 @@ unsigned int vme_master_rmw(struct vme_resource *resource, unsigned int mask,
>  	struct vme_master_resource *image;
>
>  	if (!bridge->master_rmw) {
> -		printk(KERN_WARNING "Writing to resource not supported\n");
> +		dev_warn(bridge->parent, "Writing to resource not supported\n");
>  		return -EINVAL;
>  	}
>
>  	if (resource->type != VME_MASTER) {
> -		printk(KERN_ERR "Not a master resource\n");
> +		pr_err("Not a master resource\n");
>  		return -EINVAL;
>  	}
>
> @@ -812,21 +816,21 @@ void vme_master_free(struct vme_resource *resource)
>  	struct vme_master_resource *master_image;
>
>  	if (resource->type != VME_MASTER) {
> -		printk(KERN_ERR "Not a master resource\n");
> +		pr_err("Not a master resource\n");
>  		return;
>  	}
>
>  	master_image = list_entry(resource->entry, struct vme_master_resource,
>  				  list);
>  	if (!master_image) {
> -		printk(KERN_ERR "Can't find master resource\n");
> +		pr_err("Can't find master resource\n");
>  		return;
>  	}
>
>  	/* Unlock image */
>  	spin_lock(&master_image->lock);
>  	if (master_image->locked == 0)
> -		printk(KERN_ERR "Image is already free\n");
> +		pr_err("Image is already free\n");
>
>  	master_image->locked = 0;
>  	spin_unlock(&master_image->lock);
> @@ -854,18 +858,19 @@ struct vme_resource *vme_dma_request(struct vme_dev *vdev, u32 route)
>  	struct vme_resource *resource = NULL;
>
>  	/* XXX Not checking resource attributes */
> -	printk(KERN_ERR "No VME resource Attribute tests done\n");
> +	pr_err("No VME resource Attribute tests done\n");
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		goto err_bus;
>  	}
>
>  	/* Loop through DMA resources */
>  	list_for_each_entry(dma_ctrlr, &bridge->dma_resources, list) {
>  		if (!dma_ctrlr) {
> -			printk(KERN_ERR "Registered NULL DMA resource\n");
> +			dev_err(bridge->parent,
> +				"Registered NULL DMA resource\n");
>  			continue;
>  		}
>
> @@ -920,7 +925,7 @@ struct vme_dma_list *vme_new_dma_list(struct vme_resource *resource)
>  	struct vme_dma_list *dma_list;
>
>  	if (resource->type != VME_DMA) {
> -		printk(KERN_ERR "Not a DMA resource\n");
> +		pr_err("Not a DMA resource\n");
>  		return NULL;
>  	}
>
> @@ -1098,12 +1103,13 @@ int vme_dma_list_add(struct vme_dma_list *list, struct vme_dma_attr *src,
>  	int retval;
>
>  	if (!bridge->dma_list_add) {
> -		printk(KERN_WARNING "Link List DMA generation not supported\n");
> +		dev_warn(bridge->parent,
> +			 "Link List DMA generation not supported\n");
>  		return -EINVAL;
>  	}
>
>  	if (!mutex_trylock(&list->mtx)) {
> -		printk(KERN_ERR "Link List already submitted\n");
> +		pr_err("Link List already submitted\n");
>  		return -EINVAL;
>  	}
>
> @@ -1131,7 +1137,8 @@ int vme_dma_list_exec(struct vme_dma_list *list)
>  	int retval;
>
>  	if (!bridge->dma_list_exec) {
> -		printk(KERN_ERR "Link List DMA execution not supported\n");
> +		dev_err(bridge->parent,
> +			"Link List DMA execution not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -1160,12 +1167,13 @@ int vme_dma_list_free(struct vme_dma_list *list)
>  	int retval;
>
>  	if (!bridge->dma_list_empty) {
> -		printk(KERN_WARNING "Emptying of Link Lists not supported\n");
> +		dev_warn(bridge->parent,
> +			 "Emptying of Link Lists not supported\n");
>  		return -EINVAL;
>  	}
>
>  	if (!mutex_trylock(&list->mtx)) {
> -		printk(KERN_ERR "Link List in use\n");
> +		pr_err("Link List in use\n");
>  		return -EBUSY;
>  	}
>
> @@ -1175,7 +1183,7 @@ int vme_dma_list_free(struct vme_dma_list *list)
>  	 */
>  	retval = bridge->dma_list_empty(list);
>  	if (retval) {
> -		printk(KERN_ERR "Unable to empty link-list entries\n");
> +		pr_err("Unable to empty link-list entries\n");
>  		mutex_unlock(&list->mtx);
>  		return retval;
>  	}
> @@ -1200,19 +1208,19 @@ int vme_dma_free(struct vme_resource *resource)
>  	struct vme_dma_resource *ctrlr;
>
>  	if (resource->type != VME_DMA) {
> -		printk(KERN_ERR "Not a DMA resource\n");
> +		pr_err("Not a DMA resource\n");
>  		return -EINVAL;
>  	}
>
>  	ctrlr = list_entry(resource->entry, struct vme_dma_resource, list);
>
>  	if (!mutex_trylock(&ctrlr->mtx)) {
> -		printk(KERN_ERR "Resource busy, can't free\n");
> +		pr_err("Resource busy, can't free\n");
>  		return -EBUSY;
>  	}
>
>  	if (!(list_empty(&ctrlr->pending) && list_empty(&ctrlr->running))) {
> -		printk(KERN_WARNING "Resource still processing transfers\n");
> +		pr_warn("Resource still processing transfers\n");
>  		mutex_unlock(&ctrlr->mtx);
>  		return -EBUSY;
>  	}
> @@ -1290,8 +1298,9 @@ void vme_irq_handler(struct vme_bridge *bridge, int level, int statid)
>  	if (call)
>  		call(level, statid, priv_data);
>  	else
> -		printk(KERN_WARNING "Spurious VME interrupt, level:%x, vector:%x\n",
> -		       level, statid);
> +		dev_warn(bridge->parent,
> +			 "Spurious VME interrupt, level:%x, vector:%x\n", level,
> +			 statid);
>  }
>  EXPORT_SYMBOL(vme_irq_handler);
>
> @@ -1319,17 +1328,18 @@ int vme_irq_request(struct vme_dev *vdev, int level, int statid,
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		return -EINVAL;
>  	}
>
>  	if ((level < 1) || (level > 7)) {
> -		printk(KERN_ERR "Invalid interrupt level\n");
> +		pr_err("Invalid interrupt level\n");
>  		return -EINVAL;
>  	}
>
>  	if (!bridge->irq_set) {
> -		printk(KERN_ERR "Configuring interrupts not supported\n");
> +		dev_err(bridge->parent,
> +			"Configuring interrupts not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -1337,7 +1347,7 @@ int vme_irq_request(struct vme_dev *vdev, int level, int statid,
>
>  	if (bridge->irq[level - 1].callback[statid].func) {
>  		mutex_unlock(&bridge->irq_mtx);
> -		printk(KERN_WARNING "VME Interrupt already taken\n");
> +		dev_warn(bridge->parent, "VME Interrupt already taken\n");
>  		return -EBUSY;
>  	}
>
> @@ -1368,17 +1378,18 @@ void vme_irq_free(struct vme_dev *vdev, int level, int statid)
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		return;
>  	}
>
>  	if ((level < 1) || (level > 7)) {
> -		printk(KERN_ERR "Invalid interrupt level\n");
> +		pr_err("Invalid interrupt level\n");
>  		return;
>  	}
>
>  	if (!bridge->irq_set) {
> -		printk(KERN_ERR "Configuring interrupts not supported\n");
> +		dev_err(bridge->parent,
> +			"Configuring interrupts not supported\n");
>  		return;
>  	}
>
> @@ -1415,17 +1426,18 @@ int vme_irq_generate(struct vme_dev *vdev, int level, int statid)
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		return -EINVAL;
>  	}
>
>  	if ((level < 1) || (level > 7)) {
> -		printk(KERN_WARNING "Invalid interrupt level\n");
> +		pr_warn("Invalid interrupt level\n");
>  		return -EINVAL;
>  	}
>
>  	if (!bridge->irq_generate) {
> -		printk(KERN_WARNING "Interrupt generation not supported\n");
> +		dev_warn(bridge->parent,
> +			 "Interrupt generation not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -1452,14 +1464,15 @@ struct vme_resource *vme_lm_request(struct vme_dev *vdev)
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		goto err_bus;
>  	}
>
>  	/* Loop through LM resources */
>  	list_for_each_entry(lm, &bridge->lm_resources, list) {
>  		if (!lm) {
> -			printk(KERN_ERR "Registered NULL Location Monitor resource\n");
> +			dev_err(bridge->parent,
> +				"Registered NULL Location Monitor resource\n");
>  			continue;
>  		}
>
> @@ -1514,7 +1527,7 @@ int vme_lm_count(struct vme_resource *resource)
>  	struct vme_lm_resource *lm;
>
>  	if (resource->type != VME_LM) {
> -		printk(KERN_ERR "Not a Location Monitor resource\n");
> +		pr_err("Not a Location Monitor resource\n");
>  		return -EINVAL;
>  	}
>
> @@ -1545,14 +1558,14 @@ int vme_lm_set(struct vme_resource *resource, unsigned long long lm_base,
>  	struct vme_lm_resource *lm;
>
>  	if (resource->type != VME_LM) {
> -		printk(KERN_ERR "Not a Location Monitor resource\n");
> +		pr_err("Not a Location Monitor resource\n");
>  		return -EINVAL;
>  	}
>
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
>  	if (!bridge->lm_set) {
> -		printk(KERN_ERR "vme_lm_set not supported\n");
> +		dev_err(bridge->parent, "vme_lm_set not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -1581,14 +1594,14 @@ int vme_lm_get(struct vme_resource *resource, unsigned long long *lm_base,
>  	struct vme_lm_resource *lm;
>
>  	if (resource->type != VME_LM) {
> -		printk(KERN_ERR "Not a Location Monitor resource\n");
> +		pr_err("Not a Location Monitor resource\n");
>  		return -EINVAL;
>  	}
>
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
>  	if (!bridge->lm_get) {
> -		printk(KERN_ERR "vme_lm_get not supported\n");
> +		dev_err(bridge->parent, "vme_lm_get not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -1618,14 +1631,14 @@ int vme_lm_attach(struct vme_resource *resource, int monitor,
>  	struct vme_lm_resource *lm;
>
>  	if (resource->type != VME_LM) {
> -		printk(KERN_ERR "Not a Location Monitor resource\n");
> +		pr_err("Not a Location Monitor resource\n");
>  		return -EINVAL;
>  	}
>
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
>  	if (!bridge->lm_attach) {
> -		printk(KERN_ERR "vme_lm_attach not supported\n");
> +		dev_err(bridge->parent, "vme_lm_attach not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -1651,14 +1664,14 @@ int vme_lm_detach(struct vme_resource *resource, int monitor)
>  	struct vme_lm_resource *lm;
>
>  	if (resource->type != VME_LM) {
> -		printk(KERN_ERR "Not a Location Monitor resource\n");
> +		pr_err("Not a Location Monitor resource\n");
>  		return -EINVAL;
>  	}
>
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
>  	if (!bridge->lm_detach) {
> -		printk(KERN_ERR "vme_lm_detach not supported\n");
> +		dev_err(bridge->parent, "vme_lm_detach not supported\n");
>  		return -EINVAL;
>  	}
>
> @@ -1683,7 +1696,7 @@ void vme_lm_free(struct vme_resource *resource)
>  	struct vme_lm_resource *lm;
>
>  	if (resource->type != VME_LM) {
> -		printk(KERN_ERR "Not a Location Monitor resource\n");
> +		pr_err("Not a Location Monitor resource\n");
>  		return;
>  	}
>
> @@ -1720,12 +1733,12 @@ int vme_slot_num(struct vme_dev *vdev)
>
>  	bridge = vdev->bridge;
>  	if (!bridge) {
> -		printk(KERN_ERR "Can't find VME bus\n");
> +		pr_err("Can't find VME bus\n");
>  		return -EINVAL;
>  	}
>
>  	if (!bridge->slot_get) {
> -		printk(KERN_WARNING "vme_slot_num not supported\n");
> +		dev_warn(bridge->parent, "vme_slot_num not supported\n");
>  		return -EINVAL;
>  	}
>
> --
> 2.42.0
>
>
>

Re: [PATCH 0/2] staging: vme_user: Replace printk's & cleanup log messages

[Julia Lawall]

On Tue, 17 Oct 2023, Soumya Negi wrote:

> Staging driver vme_user has a bunch of printk() calls in vme.c which
> triggers checkpatch warnings. Remove all printk's and change them to
> the appropriate logging mechanism i.e pr_err()/pr_warn(), or
> dev_err()/dev_warn().
>
> Also, cleanup the messages further by using __func__ in the string
> instead of function names.

"cleanup" is not actually a verb.  It's a noun.  The verb would be clean
up.

julia

>
> This patchset fixes all checkpatch warnings like:
>
>     WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then
>              dev_err(dev, ... then pr_err(...  to printk(KERN_ERR ...
> &
>     WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
>              this function's name, in a string
>
> Patches must be applied in order.
>
> Soumya Negi (2):
>   staging: vme_user: Replace printk() with pr_*(),dev_*()
>   staging: vme_user: Use __func__ instead of function name
>
>  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
>  1 file changed, 94 insertions(+), 81 deletions(-)
>
> --
> 2.42.0
>
>
>

Re: [PATCH 0/2] staging: vme_user: Replace printk's & cleanup log messages

[Soumya Negi]

On Wed, Oct 18, 2023 at 07:49:22AM +0200, Julia Lawall wrote:
> 
> 
> On Tue, 17 Oct 2023, Soumya Negi wrote:
> 
> > Staging driver vme_user has a bunch of printk() calls in vme.c which
> > triggers checkpatch warnings. Remove all printk's and change them to
> > the appropriate logging mechanism i.e pr_err()/pr_warn(), or
> > dev_err()/dev_warn().
> >
> > Also, cleanup the messages further by using __func__ in the string
> > instead of function names.
> 
> "cleanup" is not actually a verb.  It's a noun.  The verb would be clean
> up.
> 
> julia
>

Hi Julia,

Will update the commit log.

Tanks,
Soumya

> >
> > This patchset fixes all checkpatch warnings like:
> >
> >     WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then
> >              dev_err(dev, ... then pr_err(...  to printk(KERN_ERR ...
> > &
> >     WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
> >              this function's name, in a string
> >
> > Patches must be applied in order.
> >
> > Soumya Negi (2):
> >   staging: vme_user: Replace printk() with pr_*(),dev_*()
> >   staging: vme_user: Use __func__ instead of function name
> >
> >  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
> >  1 file changed, 94 insertions(+), 81 deletions(-)
> >
> > --
> > 2.42.0
> >
> >
> >

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Soumya Negi]

Hi Julia,

On Wed, Oct 18, 2023 at 07:47:29AM +0200, Julia Lawall wrote:
> 
> 
> On Tue, 17 Oct 2023, Soumya Negi wrote:
> 
> > vme.c uses printk() to log messages. To improve and standardize message
> > formatting, use logging mechanisms pr_err()/pr_warn() and
> > dev_err()/dev_warn() instead. Retain the printk log levels of the
> > messages during replacement.
> 
> It's not possible to use the dev_ functions more often?  The pr_functions
> don't give context information, and the message don't seem to give much
> context information either.
> 
> julia
> 

Yes, I think there can be more dev_*() functions.

Most of the dev_ that can be added are error/warning messages where the bridge's
'struct device' instance is accessible & can be fed to dev_ fns. Although I went
through the VME subsystem docs, I wasn't sure if the bridge's context made sense
to be printed with those messages. So I wrote them as pr_ functions.

Should I add them and send a v2 for review? 

Regards,
Soumya

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Julia Lawall]

On Tue, 17 Oct 2023, Soumya Negi wrote:

> Hi Julia,
>
> On Wed, Oct 18, 2023 at 07:47:29AM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 17 Oct 2023, Soumya Negi wrote:
> >
> > > vme.c uses printk() to log messages. To improve and standardize message
> > > formatting, use logging mechanisms pr_err()/pr_warn() and
> > > dev_err()/dev_warn() instead. Retain the printk log levels of the
> > > messages during replacement.
> >
> > It's not possible to use the dev_ functions more often?  The pr_functions
> > don't give context information, and the message don't seem to give much
> > context information either.
> >
> > julia
> >
>
> Yes, I think there can be more dev_*() functions.
>
> Most of the dev_ that can be added are error/warning messages where the bridge's
> 'struct device' instance is accessible & can be fed to dev_ fns. Although I went
> through the VME subsystem docs, I wasn't sure if the bridge's context made sense
> to be printed with those messages. So I wrote them as pr_ functions.
>
> Should I add them and send a v2 for review?

Maybe wait a bit and see if someone else has a comment on this issue.

julia

>
> Regards,
> Soumya
>
>

Re: [PATCH 2/2] staging: vme_user: Use __func__ instead of function name

[Andi Shyti]

Hi Soumya,

On Tue, Oct 17, 2023 at 09:36:33PM -0700, Soumya Negi wrote:
> Replace function names in message strings with __func__ to fix
> all checkpatch warnings like:
> 
>     WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
>              this function's name, in a string
> 
> Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> ---
>  drivers/staging/vme_user/vme.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> index e8c2c1e77b7d..11c1df12b657 100644
> --- a/drivers/staging/vme_user/vme.c
> +++ b/drivers/staging/vme_user/vme.c
> @@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
>  	image = list_entry(resource->entry, struct vme_slave_resource, list);
>  
>  	if (!bridge->slave_get) {
> -		dev_err(bridge->parent, "vme_slave_get not supported\n");
> +		dev_err(bridge->parent, "%s not supported\n", __func__);
>  		return -EINVAL;
>  	}
>  
> @@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
>  	image = list_entry(resource->entry, struct vme_master_resource, list);
>  
>  	if (!bridge->master_set) {
> -		dev_warn(bridge->parent, "vme_master_set not supported\n");
> +		dev_warn(bridge->parent, "%s not supported\n", __func__);

I wouldn't disagree if you made this dev_err() instead of
dev_warn(). The reasoning behind is that if it's a warning you
should not fail. But beacuse you are returning -EINVAL it means
that you are failing, therefore you should use dev_err().

Others might object that the change I'm suggesting sohuld go in a
different patch, which is also OK.

>  		return -EINVAL;

... or, if you want to keep the dev_warn(), whou can consider
removing the "return -EINVAL;". But this is an evaluation you
should make in a different patch and mainly evaluate if it's
OK to remove the error here.

>  	}
>  
> @@ -1565,7 +1565,7 @@ int vme_lm_set(struct vme_resource *resource, unsigned long long lm_base,
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>  
>  	if (!bridge->lm_set) {
> -		dev_err(bridge->parent, "vme_lm_set not supported\n");
> +		dev_err(bridge->parent, "%s not supported\n", __func__);
>  		return -EINVAL;
>  	}
>  
> @@ -1601,7 +1601,7 @@ int vme_lm_get(struct vme_resource *resource, unsigned long long *lm_base,
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>  
>  	if (!bridge->lm_get) {
> -		dev_err(bridge->parent, "vme_lm_get not supported\n");
> +		dev_err(bridge->parent, "%s not supported\n", __func__);
>  		return -EINVAL;
>  	}
>  
> @@ -1638,7 +1638,7 @@ int vme_lm_attach(struct vme_resource *resource, int monitor,
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>  
>  	if (!bridge->lm_attach) {
> -		dev_err(bridge->parent, "vme_lm_attach not supported\n");
> +		dev_err(bridge->parent, "%s not supported\n", __func__);
>  		return -EINVAL;
>  	}
>  
> @@ -1671,7 +1671,7 @@ int vme_lm_detach(struct vme_resource *resource, int monitor)
>  	lm = list_entry(resource->entry, struct vme_lm_resource, list);
>  
>  	if (!bridge->lm_detach) {
> -		dev_err(bridge->parent, "vme_lm_detach not supported\n");
> +		dev_err(bridge->parent, "%s not supported\n", __func__);
>  		return -EINVAL;
>  	}
>  
> @@ -1738,7 +1738,7 @@ int vme_slot_num(struct vme_dev *vdev)
>  	}
>  
>  	if (!bridge->slot_get) {
> -		dev_warn(bridge->parent, "vme_slot_num not supported\n");
> +		dev_warn(bridge->parent, "%s not supported\n", __func__);
>  		return -EINVAL;
>  	}

Nothing wrong with the patch itself. But imagine if we end up in
one of those printouts and, as a user, you read something like:

   ... vme_slot_num not supported

The message itself doesn't say much to the user. The perfect fix
would be to re-write all these error messages with a proper
meaningful sentence, like, e.g.:

   Can't retrieve the CS/CSR slot id

(don't even know if it's fully correct). Anyway, I understand
you don't have much time for such fine changes, so whatever you
decide to do:

Acked-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Greg Kroah-Hartman]

On Tue, Oct 17, 2023 at 09:36:32PM -0700, Soumya Negi wrote:
> vme.c uses printk() to log messages. To improve and standardize message
> formatting, use logging mechanisms pr_err()/pr_warn() and
> dev_err()/dev_warn() instead. Retain the printk log levels of the
> messages during replacement.
> 
> Issue found by checkpatch.pl
> 
> Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> ---
>  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
>  1 file changed, 94 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> index 6519a7c994a0..e8c2c1e77b7d 100644
> --- a/drivers/staging/vme_user/vme.c
> +++ b/drivers/staging/vme_user/vme.c
> @@ -9,6 +9,8 @@
>   * Copyright 2004 Motorola Inc.
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

No, this is a driver, as others have pointed out, always use dev_*()
calls instead.

Yes, that means that sometimes you do need to propagate the proper
device to the function, but that's ok, it's the correct solution here.

thanks,

greg k-h

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Soumya Negi]

Hi Greg,

On Wed, Oct 18, 2023 at 03:26:07PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 17, 2023 at 09:36:32PM -0700, Soumya Negi wrote:
> > vme.c uses printk() to log messages. To improve and standardize message
> > formatting, use logging mechanisms pr_err()/pr_warn() and
> > dev_err()/dev_warn() instead. Retain the printk log levels of the
> > messages during replacement.
> > 
> > Issue found by checkpatch.pl
> > 
> > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > ---
> >  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
> >  1 file changed, 94 insertions(+), 81 deletions(-)
> > 
> > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > index 6519a7c994a0..e8c2c1e77b7d 100644
> > --- a/drivers/staging/vme_user/vme.c
> > +++ b/drivers/staging/vme_user/vme.c
> > @@ -9,6 +9,8 @@
> >   * Copyright 2004 Motorola Inc.
> >   */
> >  
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> No, this is a driver, as others have pointed out, always use dev_*()
> calls instead.

Some of the pr_ fns can be dev_, but I don't think all can.
e.g. device NULL-check error messages

Also, there are portions of the driver where we have no access to
any 'struct device' to feed into dev_.

> Yes, that means that sometimes you do need to propagate the proper
> device to the function, but that's ok, it's the correct solution here.

Got it. In v2, I'll make pr_*() to dev_*() wherever possible. In most cases, 
the VME bridge device is accessible or if not we can get it using find_bridge().

Thanks,
Soumya
> thanks,
> 
> greg k-h

Re: [PATCH 2/2] staging: vme_user: Use __func__ instead of function name

[Soumya Negi]

Hi Andi,

On Wed, Oct 18, 2023 at 01:00:01PM +0200, Andi Shyti wrote:
> Hi Soumya,
> 
> On Tue, Oct 17, 2023 at 09:36:33PM -0700, Soumya Negi wrote:
> > Replace function names in message strings with __func__ to fix
> > all checkpatch warnings like:
> > 
> >     WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
> >              this function's name, in a string
> > 
> > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > ---
> >  drivers/staging/vme_user/vme.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > index e8c2c1e77b7d..11c1df12b657 100644
> > --- a/drivers/staging/vme_user/vme.c
> > +++ b/drivers/staging/vme_user/vme.c
> > @@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
> >  	image = list_entry(resource->entry, struct vme_slave_resource, list);
> >  
> >  	if (!bridge->slave_get) {
> > -		dev_err(bridge->parent, "vme_slave_get not supported\n");
> > +		dev_err(bridge->parent, "%s not supported\n", __func__);
> >  		return -EINVAL;
> >  	}
> >  
> > @@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
> >  	image = list_entry(resource->entry, struct vme_master_resource, list);
> >  
> >  	if (!bridge->master_set) {
> > -		dev_warn(bridge->parent, "vme_master_set not supported\n");
> > +		dev_warn(bridge->parent, "%s not supported\n", __func__);
> 
> I wouldn't disagree if you made this dev_err() instead of
> dev_warn(). The reasoning behind is that if it's a warning you
> should not fail. But beacuse you are returning -EINVAL it means
> that you are failing, therefore you should use dev_err().
> 
> Others might object that the change I'm suggesting sohuld go in a
> different patch, which is also OK.
> 
> >  		return -EINVAL;
> 
> ... or, if you want to keep the dev_warn(), whou can consider
> removing the "return -EINVAL;". But this is an evaluation you
> should make in a different patch and mainly evaluate if it's
> OK to remove the error here.

I think it should be dev_err() too. The driver has inconsistently used
warn and err log levels in similar functions as well. But I was planning
to tackle those in a standalone patch after the printk's are gone. Or do
you think it should be part of this patchset?

> >  	if (!bridge->slot_get) {
> > -		dev_warn(bridge->parent, "vme_slot_num not supported\n");
> > +		dev_warn(bridge->parent, "%s not supported\n", __func__);
> >  		return -EINVAL;
> >  	}
> 
> Nothing wrong with the patch itself. But imagine if we end up in
> one of those printouts and, as a user, you read something like:
> 
>    ... vme_slot_num not supported
> 
> The message itself doesn't say much to the user. The perfect fix
> would be to re-write all these error messages with a proper
> meaningful sentence, like, e.g.:
> 
>    Can't retrieve the CS/CSR slot id
>
> (don't even know if it's fully correct). Anyway, I understand
> you don't have much time for such fine changes, so whatever you
> decide to do:

Got it. Thanks for the patch suggestion. Although yes, since I still have project
starter tasks pending for my outreachy application(will have to prioritize                           
them) I'm not sure if I will be able to get this done right away. Will try to
though.

> Acked-by: Andi Shyti <andi.shyti@linux.intel.com> 
> 
> Andi

Thanks for the ack. Since I'll have to revise and resend this patch as v2, 
should I not add your ack tag as the patch will be reviewed again? Just 
wondering what the etiquette is.

Though normally are patches supposed to be resent as new versions when adding
ack tags?

Regards,
Soumya

Re: [PATCH 2/2] staging: vme_user: Use __func__ instead of function name

[Andi Shyti]

Hi Soumya,

> > On Tue, Oct 17, 2023 at 09:36:33PM -0700, Soumya Negi wrote:
> > > Replace function names in message strings with __func__ to fix
> > > all checkpatch warnings like:
> > > 
> > >     WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
> > >              this function's name, in a string
> > > 
> > > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > > ---
> > >  drivers/staging/vme_user/vme.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > index e8c2c1e77b7d..11c1df12b657 100644
> > > --- a/drivers/staging/vme_user/vme.c
> > > +++ b/drivers/staging/vme_user/vme.c
> > > @@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
> > >  	image = list_entry(resource->entry, struct vme_slave_resource, list);
> > >  
> > >  	if (!bridge->slave_get) {
> > > -		dev_err(bridge->parent, "vme_slave_get not supported\n");
> > > +		dev_err(bridge->parent, "%s not supported\n", __func__);
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > @@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
> > >  	image = list_entry(resource->entry, struct vme_master_resource, list);
> > >  
> > >  	if (!bridge->master_set) {
> > > -		dev_warn(bridge->parent, "vme_master_set not supported\n");
> > > +		dev_warn(bridge->parent, "%s not supported\n", __func__);
> > 
> > I wouldn't disagree if you made this dev_err() instead of
> > dev_warn(). The reasoning behind is that if it's a warning you
> > should not fail. But beacuse you are returning -EINVAL it means
> > that you are failing, therefore you should use dev_err().
> > 
> > Others might object that the change I'm suggesting sohuld go in a
> > different patch, which is also OK.
> > 
> > >  		return -EINVAL;
> > 
> > ... or, if you want to keep the dev_warn(), whou can consider
> > removing the "return -EINVAL;". But this is an evaluation you
> > should make in a different patch and mainly evaluate if it's
> > OK to remove the error here.
> 
> I think it should be dev_err() too. The driver has inconsistently used
> warn and err log levels in similar functions as well. But I was planning
> to tackle those in a standalone patch after the printk's are gone. Or do
> you think it should be part of this patchset?

OK... if you already noticed this and you are saying there are
more cases, then it's better if you send a different patch.

> > >  	if (!bridge->slot_get) {
> > > -		dev_warn(bridge->parent, "vme_slot_num not supported\n");
> > > +		dev_warn(bridge->parent, "%s not supported\n", __func__);
> > >  		return -EINVAL;
> > >  	}
> > 
> > Nothing wrong with the patch itself. But imagine if we end up in
> > one of those printouts and, as a user, you read something like:
> > 
> >    ... vme_slot_num not supported
> > 
> > The message itself doesn't say much to the user. The perfect fix
> > would be to re-write all these error messages with a proper
> > meaningful sentence, like, e.g.:
> > 
> >    Can't retrieve the CS/CSR slot id
> >
> > (don't even know if it's fully correct). Anyway, I understand
> > you don't have much time for such fine changes, so whatever you
> > decide to do:
> 
> Got it. Thanks for the patch suggestion. Although yes, since I still have project
> starter tasks pending for my outreachy application(will have to prioritize                           
> them) I'm not sure if I will be able to get this done right away. Will try to
> though.

yeah... nevermind.

> > Acked-by: Andi Shyti <andi.shyti@linux.intel.com> 
> > 
> > Andi
> 
> Thanks for the ack. Since I'll have to revise and resend this patch as v2, 
> should I not add your ack tag as the patch will be reviewed again? Just 
> wondering what the etiquette is.

You can keep my tag, unless you are making drastic changes.

Nevertheless, the patch might be reviewed again.

> Though normally are patches supposed to be resent as new versions when adding
> ack tags?

If someones acks or reviews the patches you don't need to resend
just for adding the tags. There are tools doing it and
maintainers take care of that.

But, on the other hand, if you are going to resend anyway, for
other reasons a new version, then you should add the tags
yourself, otherwise they might be lost.

If the new version changes considerably, then you might consider
not adding any tags.

Andi

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Greg Kroah-Hartman]

On Wed, Oct 18, 2023 at 12:38:56PM -0700, Soumya Negi wrote:
> Hi Greg,
> 
> On Wed, Oct 18, 2023 at 03:26:07PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Oct 17, 2023 at 09:36:32PM -0700, Soumya Negi wrote:
> > > vme.c uses printk() to log messages. To improve and standardize message
> > > formatting, use logging mechanisms pr_err()/pr_warn() and
> > > dev_err()/dev_warn() instead. Retain the printk log levels of the
> > > messages during replacement.
> > > 
> > > Issue found by checkpatch.pl
> > > 
> > > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > > ---
> > >  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
> > >  1 file changed, 94 insertions(+), 81 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > index 6519a7c994a0..e8c2c1e77b7d 100644
> > > --- a/drivers/staging/vme_user/vme.c
> > > +++ b/drivers/staging/vme_user/vme.c
> > > @@ -9,6 +9,8 @@
> > >   * Copyright 2004 Motorola Inc.
> > >   */
> > >  
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > 
> > No, this is a driver, as others have pointed out, always use dev_*()
> > calls instead.
> 
> Some of the pr_ fns can be dev_, but I don't think all can.
> e.g. device NULL-check error messages

I would argue that those are pointless and can be removed and also the
check is probably not needed either.

> Also, there are portions of the driver where we have no access to
> any 'struct device' to feed into dev_.

Then you can fix that :)

thanks,

greg k-h

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Soumya Negi]

On Thu, Oct 19, 2023 at 05:34:01PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 18, 2023 at 12:38:56PM -0700, Soumya Negi wrote:
> > Hi Greg,
> > 
> > On Wed, Oct 18, 2023 at 03:26:07PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Oct 17, 2023 at 09:36:32PM -0700, Soumya Negi wrote:
> > > > vme.c uses printk() to log messages. To improve and standardize message
> > > > formatting, use logging mechanisms pr_err()/pr_warn() and
> > > > dev_err()/dev_warn() instead. Retain the printk log levels of the
> > > > messages during replacement.
> > > > 
> > > > Issue found by checkpatch.pl
> > > > 
> > > > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > > > ---
> > > >  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
> > > >  1 file changed, 94 insertions(+), 81 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > > index 6519a7c994a0..e8c2c1e77b7d 100644
> > > > --- a/drivers/staging/vme_user/vme.c
> > > > +++ b/drivers/staging/vme_user/vme.c
> > > > @@ -9,6 +9,8 @@
> > > >   * Copyright 2004 Motorola Inc.
> > > >   */
> > > >  
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > 
> > > No, this is a driver, as others have pointed out, always use dev_*()
> > > calls instead.
> > 
> > Some of the pr_ fns can be dev_, but I don't think all can.
> > e.g. device NULL-check error messages
> 
> I would argue that those are pointless and can be removed and also the
> check is probably not needed either.

Got it. The pr_() in find_bridge() can't be converted to dev_ so I'll remove 
the message entirely in another patch.

I understand that the device-NULL checks should be done on the caller's side. 
Since empty devices would mean something went wrong, would it be better to
put in an assertion(..WARN_ON) when removing the check? 

> > Also, there are portions of the driver where we have no access to
> > any 'struct device' to feed into dev_.
> 
> Then you can fix that :)

Will do this. 

Regards,
Soumya

> thanks,
> 
> greg k-h

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Greg Kroah-Hartman]

On Thu, Oct 19, 2023 at 12:06:18PM -0700, Soumya Negi wrote:
> On Thu, Oct 19, 2023 at 05:34:01PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Oct 18, 2023 at 12:38:56PM -0700, Soumya Negi wrote:
> > > Hi Greg,
> > > 
> > > On Wed, Oct 18, 2023 at 03:26:07PM +0200, Greg Kroah-Hartman wrote:
> > > > On Tue, Oct 17, 2023 at 09:36:32PM -0700, Soumya Negi wrote:
> > > > > vme.c uses printk() to log messages. To improve and standardize message
> > > > > formatting, use logging mechanisms pr_err()/pr_warn() and
> > > > > dev_err()/dev_warn() instead. Retain the printk log levels of the
> > > > > messages during replacement.
> > > > > 
> > > > > Issue found by checkpatch.pl
> > > > > 
> > > > > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > > > > ---
> > > > >  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
> > > > >  1 file changed, 94 insertions(+), 81 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > > > index 6519a7c994a0..e8c2c1e77b7d 100644
> > > > > --- a/drivers/staging/vme_user/vme.c
> > > > > +++ b/drivers/staging/vme_user/vme.c
> > > > > @@ -9,6 +9,8 @@
> > > > >   * Copyright 2004 Motorola Inc.
> > > > >   */
> > > > >  
> > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > 
> > > > No, this is a driver, as others have pointed out, always use dev_*()
> > > > calls instead.
> > > 
> > > Some of the pr_ fns can be dev_, but I don't think all can.
> > > e.g. device NULL-check error messages
> > 
> > I would argue that those are pointless and can be removed and also the
> > check is probably not needed either.
> 
> Got it. The pr_() in find_bridge() can't be converted to dev_ so I'll remove 
> the message entirely in another patch.
> 
> I understand that the device-NULL checks should be done on the caller's side. 
> Since empty devices would mean something went wrong, would it be better to
> put in an assertion(..WARN_ON) when removing the check? 

WARN_ON() means "I have no idea what can happen here so I give up",
which is not a good idea in kernel development.  If that every hits,
then your machine will reboot as the huge majority of all Linux systems
in the world run with panic-on-warn enabled.

If it is impossible for something to happen (i.e. you control all
callers) then just do not check for it.  If it happens, you will get a
NULL-dereference which is the same as a WARN_ON() in a way.

No new WARN_ON() should ever be added to the kernel, especially in a
driver.  Handle the condition if it is possible to be hit.  If it can
never be hit, don't even check it.

thanks,

greg k-h

Re: [PATCH 1/2] staging: vme_user: Replace printk() with pr_*(),dev_*()

[Soumya Negi]

On Thu, Oct 19, 2023 at 09:42:26PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 19, 2023 at 12:06:18PM -0700, Soumya Negi wrote:
> > On Thu, Oct 19, 2023 at 05:34:01PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Oct 18, 2023 at 12:38:56PM -0700, Soumya Negi wrote:
> > > > Hi Greg,
> > > > 
> > > > On Wed, Oct 18, 2023 at 03:26:07PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Tue, Oct 17, 2023 at 09:36:32PM -0700, Soumya Negi wrote:
> > > > > > vme.c uses printk() to log messages. To improve and standardize message
> > > > > > formatting, use logging mechanisms pr_err()/pr_warn() and
> > > > > > dev_err()/dev_warn() instead. Retain the printk log levels of the
> > > > > > messages during replacement.
> > > > > > 
> > > > > > Issue found by checkpatch.pl
> > > > > > 
> > > > > > Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
> > > > > > ---
> > > > > >  drivers/staging/vme_user/vme.c | 175 ++++++++++++++++++---------------
> > > > > >  1 file changed, 94 insertions(+), 81 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > > > > index 6519a7c994a0..e8c2c1e77b7d 100644
> > > > > > --- a/drivers/staging/vme_user/vme.c
> > > > > > +++ b/drivers/staging/vme_user/vme.c
> > > > > > @@ -9,6 +9,8 @@
> > > > > >   * Copyright 2004 Motorola Inc.
> > > > > >   */
> > > > > >  
> > > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > > 
> > > > > No, this is a driver, as others have pointed out, always use dev_*()
> > > > > calls instead.
> > > > 
> > > > Some of the pr_ fns can be dev_, but I don't think all can.
> > > > e.g. device NULL-check error messages
> > > 
> > > I would argue that those are pointless and can be removed and also the
> > > check is probably not needed either.
> > 
> > Got it. The pr_() in find_bridge() can't be converted to dev_ so I'll remove 
> > the message entirely in another patch.
> > 
> > I understand that the device-NULL checks should be done on the caller's side. 
> > Since empty devices would mean something went wrong, would it be better to
> > put in an assertion(..WARN_ON) when removing the check? 
> 
> WARN_ON() means "I have no idea what can happen here so I give up",
> which is not a good idea in kernel development.  If that every hits,
> then your machine will reboot as the huge majority of all Linux systems
> in the world run with panic-on-warn enabled.
> 
> If it is impossible for something to happen (i.e. you control all
> callers) then just do not check for it.  If it happens, you will get a
> NULL-dereference which is the same as a WARN_ON() in a way.
> 
> No new WARN_ON() should ever be added to the kernel, especially in a
> driver.  Handle the condition if it is possible to be hit.  If it can
> never be hit, don't even check it.
> 
> thanks,
> 
> greg k-h

Hi Greg,

Thank you for explaining in detail. I'll remove the device NULL-checks
completely.

Regards,
Soumya