Re: [PATCH] fnic: remove casting vmalloc

[Joe Perches <joe@perches.com>]

On Thu, 2019-07-18 at 21:02 +0200, Vasyl Gomonovych wrote:
> Generated by:  alloc_cast.cocci

Please look deeper than just the cocci output.

> diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c
[]
> @@ -59,8 +59,7 @@ int fnic_debugfs_init(void)
>  						fnic_trace_debugfs_root);
>  
>  	/* Allocate memory to structure */
> -	fc_trc_flag = (struct fc_trace_flag_type *)
> -		vmalloc(sizeof(struct fc_trace_flag_type));
> +	fc_trc_flag = vmalloc(sizeof(struct fc_trace_flag_type));
>  
>  	if (fc_trc_flag) {
>  		fc_trc_flag->fc_row_file = 0;

Not your fault, but this code is not well written.

struct fc_trace_flag_type is:

drivers/scsi/fnic/fnic_debugfs.c:struct fc_trace_flag_type {
drivers/scsi/fnic/fnic_debugfs.c-       u8 fc_row_file;
drivers/scsi/fnic/fnic_debugfs.c-       u8 fc_normal_file;
drivers/scsi/fnic/fnic_debugfs.c-       u8 fnic_trace;
drivers/scsi/fnic/fnic_debugfs.c-       u8 fc_trace;
drivers/scsi/fnic/fnic_debugfs.c-       u8 fc_clear;
drivers/scsi/fnic/fnic_debugfs.c-};

It doesn't require vmalloc as it's just 5 bytes in size.
Might as well do either of allocate it with kmalloc
or because it's a singleton, just create a static.

Maybe:
---
 drivers/scsi/fnic/fnic_debugfs.c | 55 ++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 33 deletions(-)

diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c
index 21991c99db7c..72071bd4c9a1 100644
--- a/drivers/scsi/fnic/fnic_debugfs.c
+++ b/drivers/scsi/fnic/fnic_debugfs.c
@@ -31,15 +31,13 @@ static struct dentry *fnic_fc_rdata_trace_debugfs_file;
 static struct dentry *fnic_fc_trace_enable;
 static struct dentry *fnic_fc_trace_clear;
 
-struct fc_trace_flag_type {
+static struct {
 	u8 fc_row_file;
 	u8 fc_normal_file;
 	u8 fnic_trace;
 	u8 fc_trace;
 	u8 fc_clear;
-};
-
-static struct fc_trace_flag_type *fc_trc_flag;
+} fc_trc_flag;
 
 /*
  * fnic_debugfs_init - Initialize debugfs for fnic debug logging
@@ -52,26 +50,18 @@ static struct fc_trace_flag_type *fc_trc_flag;
  */
 int fnic_debugfs_init(void)
 {
-	int rc = -1;
 	fnic_trace_debugfs_root = debugfs_create_dir("fnic", NULL);
 
 	fnic_stats_debugfs_root = debugfs_create_dir("statistics",
 						fnic_trace_debugfs_root);
 
-	/* Allocate memory to structure */
-	fc_trc_flag = (struct fc_trace_flag_type *)
-		vmalloc(sizeof(struct fc_trace_flag_type));
-
-	if (fc_trc_flag) {
-		fc_trc_flag->fc_row_file = 0;
-		fc_trc_flag->fc_normal_file = 1;
-		fc_trc_flag->fnic_trace = 2;
-		fc_trc_flag->fc_trace = 3;
-		fc_trc_flag->fc_clear = 4;
-	}
+	fc_trc_flag.fc_row_file = 0;
+	fc_trc_flag.fc_normal_file = 1;
+	fc_trc_flag.fnic_trace = 2;
+	fc_trc_flag.fc_trace = 3;
+	fc_trc_flag.fc_clear = 4;
 
-	rc = 0;
-	return rc;
+	return 0;
 }
 
 /*
@@ -89,8 +79,7 @@ void fnic_debugfs_terminate(void)
 	debugfs_remove(fnic_trace_debugfs_root);
 	fnic_trace_debugfs_root = NULL;
 
-	if (fc_trc_flag)
-		vfree(fc_trc_flag);
+	memset(&fc_trc_flag, 0, sizeof(fc_trc_flag));
 }
 
 /*
@@ -121,11 +110,11 @@ static ssize_t fnic_trace_ctrl_read(struct file *filp,
 	u8 *trace_type;
 	len = 0;
 	trace_type = (u8 *)filp->private_data;
-	if (*trace_type == fc_trc_flag->fnic_trace)
+	if (*trace_type == fc_trc_flag.fnic_trace)
 		len = sprintf(buf, "%u\n", fnic_tracing_enabled);
-	else if (*trace_type == fc_trc_flag->fc_trace)
+	else if (*trace_type == fc_trc_flag.fc_trace)
 		len = sprintf(buf, "%u\n", fnic_fc_tracing_enabled);
-	else if (*trace_type == fc_trc_flag->fc_clear)
+	else if (*trace_type == fc_trc_flag.fc_clear)
 		len = sprintf(buf, "%u\n", fnic_fc_trace_cleared);
 	else
 		pr_err("fnic: Cannot read to any debugfs file\n");
@@ -172,11 +161,11 @@ static ssize_t fnic_trace_ctrl_write(struct file *filp,
 	if (ret < 0)
 		return ret;
 
-	if (*trace_type == fc_trc_flag->fnic_trace)
+	if (*trace_type == fc_trc_flag.fnic_trace)
 		fnic_tracing_enabled = val;
-	else if (*trace_type == fc_trc_flag->fc_trace)
+	else if (*trace_type == fc_trc_flag.fc_trace)
 		fnic_fc_tracing_enabled = val;
-	else if (*trace_type == fc_trc_flag->fc_clear)
+	else if (*trace_type == fc_trc_flag.fc_clear)
 		fnic_fc_trace_cleared = val;
 	else
 		pr_err("fnic: cannot write to any debugfs file\n");
@@ -218,7 +207,7 @@ static int fnic_trace_debugfs_open(struct inode *inode,
 	if (!fnic_dbg_prt)
 		return -ENOMEM;
 
-	if (*rdata_ptr == fc_trc_flag->fnic_trace) {
+	if (*rdata_ptr == fc_trc_flag.fnic_trace) {
 		fnic_dbg_prt->buffer = vmalloc(array3_size(3, trace_max_pages,
 							   PAGE_SIZE));
 		if (!fnic_dbg_prt->buffer) {
@@ -347,13 +336,13 @@ void fnic_trace_debugfs_init(void)
 	fnic_trace_enable = debugfs_create_file("tracing_enable",
 					S_IFREG|S_IRUGO|S_IWUSR,
 					fnic_trace_debugfs_root,
-					&(fc_trc_flag->fnic_trace),
+					&fc_trc_flag.fnic_trace,
 					&fnic_trace_ctrl_fops);
 
 	fnic_trace_debugfs_file = debugfs_create_file("trace",
 					S_IFREG|S_IRUGO|S_IWUSR,
 					fnic_trace_debugfs_root,
-					&(fc_trc_flag->fnic_trace),
+					&fc_trc_flag.fnic_trace,
 					&fnic_trace_debugfs_fops);
 }
 
@@ -390,27 +379,27 @@ void fnic_fc_trace_debugfs_init(void)
 	fnic_fc_trace_enable = debugfs_create_file("fc_trace_enable",
 					S_IFREG|S_IRUGO|S_IWUSR,
 					fnic_trace_debugfs_root,
-					&(fc_trc_flag->fc_trace),
+					&fc_trc_flag.fc_trace,
 					&fnic_trace_ctrl_fops);
 
 	fnic_fc_trace_clear = debugfs_create_file("fc_trace_clear",
 					S_IFREG|S_IRUGO|S_IWUSR,
 					fnic_trace_debugfs_root,
-					&(fc_trc_flag->fc_clear),
+					&fc_trc_flag.fc_clear,
 					&fnic_trace_ctrl_fops);
 
 	fnic_fc_rdata_trace_debugfs_file =
 		debugfs_create_file("fc_trace_rdata",
 				    S_IFREG|S_IRUGO|S_IWUSR,
 				    fnic_trace_debugfs_root,
-				    &(fc_trc_flag->fc_normal_file),
+				    &fc_trc_flag.fc_normal_file,
 				    &fnic_trace_debugfs_fops);
 
 	fnic_fc_trace_debugfs_file =
 		debugfs_create_file("fc_trace",
 				    S_IFREG|S_IRUGO|S_IWUSR,
 				    fnic_trace_debugfs_root,
-				    &(fc_trc_flag->fc_row_file),
+				    &fc_trc_flag.fc_row_file,
 				    &fnic_trace_debugfs_fops);
 }