Re: [PATCH][next] scsi: fnic: Replace vmalloc() + memset() with vzalloc() and use array_size()
Reviewed-by: Satish Kharat On 6/15/20, 3:49 PM, "Gustavo A. R. Silva" wrote: Use vzalloc() instead of the vmalloc() and memset. Also, use array_size() instead of the open-coded version. This issue was found with the help of Coccinelle and, audited and fixed manually. Addresses-KSPP-ID: https://github.com/KSPP/linux/issues/83 Signed-off-by: Gustavo A. R. Silva --- drivers/scsi/fnic/fnic_trace.c | 16 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c index 9d52d83161ed..be266d1611bb 100644 --- a/drivers/scsi/fnic/fnic_trace.c +++ b/drivers/scsi/fnic/fnic_trace.c @@ -488,7 +488,7 @@ int fnic_trace_buf_init(void) } fnic_trace_entries.page_offset = - vmalloc(array_size(fnic_max_trace_entries, + vzalloc(array_size(fnic_max_trace_entries, sizeof(unsigned long))); if (!fnic_trace_entries.page_offset) { printk(KERN_ERR PFX "Failed to allocate memory for" @@ -500,8 +500,6 @@ int fnic_trace_buf_init(void) err = -ENOMEM; goto err_fnic_trace_buf_init; } - memset((void *)fnic_trace_entries.page_offset, 0, - (fnic_max_trace_entries * sizeof(unsigned long))); fnic_trace_entries.wr_idx = fnic_trace_entries.rd_idx = 0; fnic_buf_head = fnic_trace_buf_p; @@ -559,10 +557,10 @@ int fnic_fc_trace_init(void) int err = 0; int i; - fc_trace_max_entries = (fnic_fc_trace_max_pages * PAGE_SIZE)/ + fc_trace_max_entries = array_size(fnic_fc_trace_max_pages, PAGE_SIZE)/ FC_TRC_SIZE_BYTES; fnic_fc_ctlr_trace_buf_p = - (unsigned long)vmalloc(array_size(PAGE_SIZE, + (unsigned long)vzalloc(array_size(PAGE_SIZE, fnic_fc_trace_max_pages)); if (!fnic_fc_ctlr_trace_buf_p) { pr_err("fnic: Failed to allocate memory for " @@ -571,12 +569,9 @@ int fnic_fc_trace_init(void) goto err_fnic_fc_ctlr_trace_buf_init; } - memset((void *)fnic_fc_ctlr_trace_buf_p, 0, - fnic_fc_trace_max_pages * PAGE_SIZE); - /* Allocate memory for page offset */ fc_trace_entries.page_offset = - vmalloc(array_size(fc_trace_max_entries, + vzalloc(array_size(fc_trace_max_entries, sizeof(unsigned long))); if (!fc_trace_entries.page_offset) { pr_err("fnic:Failed to allocate memory for page_offset\n"); @@ -588,9 +583,6 @@ int fnic_fc_trace_init(void) err = -ENOMEM; goto err_fnic_fc_ctlr_trace_buf_init; } - memset((void *)fc_trace_entries.page_offset, 0, - (fc_trace_max_entries * sizeof(unsigned long))); - fc_trace_entries.rd_idx = fc_trace_entries.wr_idx = 0; fc_trace_buf_head = fnic_fc_ctlr_trace_buf_p; -- 2.27.0
RE: [PATCH -next] scsi: fnic: Remove set but not used variable 'vdev'
Looks good to me. Acked-by: Satish Kharat Satish Kharat TECHNICAL LEADER.ENGINEERING satis...@cisco.com Tel: +1 408 526 7504 Cisco Systems, Inc. 3800 Zanker Road SAN JOSE 95134 United States cisco.com Think before you print. This email may contain confidential and privileged material for the sole use of the intended recipient. Any review, use, distribution or disclosure by others is strictly prohibited. If you are not the intended recipient (or authorized to receive for the recipient), please contact the sender by reply email and delete all copies of this message. http://www.cisco.com/c/en/us/about/legal/terms-sale-software-license-agreement/company-registration-information.html -Original Message- From: YueHaibing Sent: Thursday, January 24, 2019 6:00 PM To: Satish Kharat (satishkh) ; Sesidhar Baddela (sebaddel) ; Karan Tilak Kumar (kartilak) ; j...@linux.ibm.com; martin.peter...@oracle.com Cc: linux-kernel@vger.kernel.org; linux-s...@vger.kernel.org; YueHaibing Subject: [PATCH -next] scsi: fnic: Remove set but not used variable 'vdev' Fixes gcc '-Wunused-but-set-variable' warning: drivers/scsi/fnic/vnic_wq.c: In function 'vnic_wq_alloc_bufs': drivers/scsi/fnic/vnic_wq.c:50:19: warning: variable 'vdev' set but not used [-Wunused-but-set-variable] drivers/scsi/fnic/vnic_rq.c: In function 'vnic_rq_alloc_bufs': drivers/scsi/fnic/vnic_rq.c:30:19: warning: variable 'vdev' set but not used [-Wunused-but-set-variable] It never used since introduction Signed-off-by: YueHaibing --- drivers/scsi/fnic/vnic_rq.c | 3 --- drivers/scsi/fnic/vnic_wq.c | 3 --- 2 files changed, 6 deletions(-) diff --git a/drivers/scsi/fnic/vnic_rq.c b/drivers/scsi/fnic/vnic_rq.c index 74d2f2c..6a35b1b 100644 --- a/drivers/scsi/fnic/vnic_rq.c +++ b/drivers/scsi/fnic/vnic_rq.c @@ -27,12 +27,9 @@ static int vnic_rq_alloc_bufs(struct vnic_rq *rq) { struct vnic_rq_buf *buf; - struct vnic_dev *vdev; unsigned int i, j, count = rq->ring.desc_count; unsigned int blks = VNIC_RQ_BUF_BLKS_NEEDED(count); - vdev = rq->vdev; - for (i = 0; i < blks; i++) { rq->bufs[i] = kzalloc(VNIC_RQ_BUF_BLK_SZ, GFP_ATOMIC); if (!rq->bufs[i]) { diff --git a/drivers/scsi/fnic/vnic_wq.c b/drivers/scsi/fnic/vnic_wq.c index 74cfc56..015af2c 100644 --- a/drivers/scsi/fnic/vnic_wq.c +++ b/drivers/scsi/fnic/vnic_wq.c @@ -47,12 +47,9 @@ int vnic_wq_alloc_ring(struct vnic_dev *vdev, struct vnic_wq *wq, static int vnic_wq_alloc_bufs(struct vnic_wq *wq) { struct vnic_wq_buf *buf; - struct vnic_dev *vdev; unsigned int i, j, count = wq->ring.desc_count; unsigned int blks = VNIC_WQ_BUF_BLKS_NEEDED(count); - vdev = wq->vdev; - for (i = 0; i < blks; i++) { wq->bufs[i] = kzalloc(VNIC_WQ_BUF_BLK_SZ, GFP_ATOMIC); if (!wq->bufs[i]) { -- 2.7.0
RE: [PATCH 3/7] scsi: fnic: no need to check return value of debugfs_create functions
Looks good to me. Acked-by: Satish Kharat Satish Kharat TECHNICAL LEADER.ENGINEERING satis...@cisco.com -Original Message- From: Greg Kroah-Hartman Sent: Tuesday, January 22, 2019 7:09 AM To: James Bottomley ; Martin Petersen Cc: linux-kernel@vger.kernel.org; linux-s...@vger.kernel.org; Greg Kroah-Hartman ; Satish Kharat (satishkh) ; Sesidhar Baddela (sebaddel) ; Karan Tilak Kumar (kartilak) Subject: [PATCH 3/7] scsi: fnic: no need to check return value of debugfs_create functions When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this. Cc: Satish Kharat Cc: Sesidhar Baddela Cc: Karan Tilak Kumar Cc: "James E.J. Bottomley" Cc: "Martin K. Petersen" Cc: linux-s...@vger.kernel.org Signed-off-by: Greg Kroah-Hartman --- drivers/scsi/fnic/fnic_debugfs.c | 88 ++-- drivers/scsi/fnic/fnic_main.c| 7 +-- drivers/scsi/fnic/fnic_stats.h | 2 +- drivers/scsi/fnic/fnic_trace.c | 17 ++ drivers/scsi/fnic/fnic_trace.h | 4 +- 5 files changed, 10 insertions(+), 108 deletions(-) diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c index 139fffa3658a..21991c99db7c 100644 --- a/drivers/scsi/fnic/fnic_debugfs.c +++ b/drivers/scsi/fnic/fnic_debugfs.c @@ -54,23 +54,9 @@ int fnic_debugfs_init(void) { int rc = -1; fnic_trace_debugfs_root = debugfs_create_dir("fnic", NULL); - if (!fnic_trace_debugfs_root) { - printk(KERN_DEBUG "Cannot create debugfs root\n"); - return rc; - } - - if (!fnic_trace_debugfs_root) { - printk(KERN_DEBUG - "fnic root directory doesn't exist in debugfs\n"); - return rc; - } fnic_stats_debugfs_root = debugfs_create_dir("statistics", fnic_trace_debugfs_root); - if (!fnic_stats_debugfs_root) { - printk(KERN_DEBUG "Cannot create Statistics directory\n"); - return rc; - } /* Allocate memory to structure */ fc_trc_flag = (struct fc_trace_flag_type *) @@ -356,39 +342,19 @@ static const struct file_operations fnic_trace_debugfs_fops = { * it will also create file trace_enable to control enable/disable of * trace logging into trace buffer. */ -int fnic_trace_debugfs_init(void) +void fnic_trace_debugfs_init(void) { - int rc = -1; - if (!fnic_trace_debugfs_root) { - printk(KERN_DEBUG - "FNIC Debugfs root directory doesn't exist\n"); - return rc; - } fnic_trace_enable = debugfs_create_file("tracing_enable", S_IFREG|S_IRUGO|S_IWUSR, fnic_trace_debugfs_root, &(fc_trc_flag->fnic_trace), _trace_ctrl_fops); - if (!fnic_trace_enable) { - printk(KERN_DEBUG - "Cannot create trace_enable file under debugfs\n"); - return rc; - } - fnic_trace_debugfs_file = debugfs_create_file("trace", S_IFREG|S_IRUGO|S_IWUSR, fnic_trace_debugfs_root, &(fc_trc_flag->fnic_trace), _trace_debugfs_fops); - - if (!fnic_trace_debugfs_file) { - printk(KERN_DEBUG - "Cannot create trace file under debugfs\n"); - return rc; - } - rc = 0; - return rc; } /* @@ -419,37 +385,20 @@ void fnic_trace_debugfs_terminate(void) * trace logging into trace buffer. */ -int fnic_fc_trace_debugfs_init(void) +void fnic_fc_trace_debugfs_init(void) { - int rc = -1; - - if (!fnic_trace_debugfs_root) { - pr_err("fnic:Debugfs root directory doesn't exist\n"); - return rc; - } - 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), _trace_ctrl_fops); - if (!fnic_fc_trace_enable) { - pr_err("fnic: Failed create fc_trace_enable file\n"); - return rc; - } - fnic_fc_trace_clear = debugfs_create_file("fc_trace_clear", S_IFREG|S_IRUGO|S_IWUSR, fnic_trace_debugfs_root,
RE: [PATCH] [RESEND] scsi: fnic: use 64-bit timestamps
Looks good to me. Acked-by: Satish Kharat <satis...@cisco.com> -Original Message- From: Arnd Bergmann [mailto:a...@arndb.de] Sent: Wednesday, January 17, 2018 7:17 AM To: Satish Kharat (satishkh) <satis...@cisco.com>; Sesidhar Baddela (sebaddel) <sebad...@cisco.com>; Karan Tilak Kumar (kartilak) <karti...@cisco.com>; James E.J. Bottomley <j...@linux.vnet.ibm.com>; Martin K. Petersen <martin.peter...@oracle.com> Cc: Arnd Bergmann <a...@arndb.de>; Vasyl Gomonovych <gomonov...@gmail.com>; linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: [PATCH] [RESEND] scsi: fnic: use 64-bit timestamps struct timespec is deprecated since it overflows in 2038 on 32-bit architectures, so we should use timespec64 consistently. I'm slightly adapting the format strings here, to make sure we print the nanoseconds with the correct number of leading zeroes. Signed-off-by: Arnd Bergmann <a...@arndb.de> --- Originally submitted in November 2017. Martin asked Satish to review my patch, but I never heard back after that, so I'm trying again now. --- drivers/scsi/fnic/fnic_debugfs.c | 2 +- drivers/scsi/fnic/fnic_stats.h | 4 +-- drivers/scsi/fnic/fnic_trace.c | 58 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c index 9858484dd126..6d3e1cb4fea6 100644 --- a/drivers/scsi/fnic/fnic_debugfs.c +++ b/drivers/scsi/fnic/fnic_debugfs.c @@ -614,7 +614,7 @@ static ssize_t fnic_reset_stats_write(struct file *file, sizeof(struct io_path_stats) - sizeof(u64)); memset(fw_stats_p+1, 0, sizeof(struct fw_stats) - sizeof(u64)); - getnstimeofday(>stats_timestamps.last_reset_time); + ktime_get_real_ts64(>stats_timestamps.last_reset_time); } (*ppos)++; diff --git a/drivers/scsi/fnic/fnic_stats.h b/drivers/scsi/fnic/fnic_stats.h index e007feedbf72..9daa6ada6fa0 100644 --- a/drivers/scsi/fnic/fnic_stats.h +++ b/drivers/scsi/fnic/fnic_stats.h @@ -18,8 +18,8 @@ #define _FNIC_STATS_H_ struct stats_timestamps { - struct timespec last_reset_time; - struct timespec last_read_time; + struct timespec64 last_reset_time; + struct timespec64 last_read_time; }; struct io_path_stats { diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c index 4826f596cb31..abddde11982b 100644 --- a/drivers/scsi/fnic/fnic_trace.c +++ b/drivers/scsi/fnic/fnic_trace.c @@ -111,7 +111,7 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) int len = 0; unsigned long flags; char str[KSYM_SYMBOL_LEN]; - struct timespec val; + struct timespec64 val; fnic_trace_data_t *tbp; spin_lock_irqsave(_trace_lock, flags); @@ -129,10 +129,10 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) /* Convert function pointer to function name */ if (sizeof(unsigned long) < 8) { sprint_symbol(str, tbp->fnaddr.low); - jiffies_to_timespec(tbp->timestamp.low, ); + jiffies_to_timespec64(tbp->timestamp.low, ); } else { sprint_symbol(str, tbp->fnaddr.val); - jiffies_to_timespec(tbp->timestamp.val, ); + jiffies_to_timespec64(tbp->timestamp.val, ); } /* * Dump trace buffer entry to memory file @@ -140,8 +140,8 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) */ len += snprintf(fnic_dbgfs_prt->buffer + len, (trace_max_pages * PAGE_SIZE * 3) - len, - "%16lu.%16lu %-50s %8x %8x %16llx %16llx " - "%16llx %16llx %16llx\n", val.tv_sec, + "%16llu.%09lu %-50s %8x %8x %16llx %16llx " + "%16llx %16llx %16llx\n", (u64)val.tv_sec, val.tv_nsec, str, tbp->host_no, tbp->tag, tbp->data[0], tbp->data[1], tbp->data[2], tbp->data[3], tbp->data[4]); @@ -171,10 +171,10 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) /* Convert function pointer to function name */ if (sizeof(unsigned long) < 8) { sprint_symbol(str, tbp->fnaddr.low); - jiffies_to_timespec(tbp->timestamp.low, ); +
RE: [PATCH] [RESEND] scsi: fnic: use 64-bit timestamps
Looks good to me. Acked-by: Satish Kharat -Original Message- From: Arnd Bergmann [mailto:a...@arndb.de] Sent: Wednesday, January 17, 2018 7:17 AM To: Satish Kharat (satishkh) ; Sesidhar Baddela (sebaddel) ; Karan Tilak Kumar (kartilak) ; James E.J. Bottomley ; Martin K. Petersen Cc: Arnd Bergmann ; Vasyl Gomonovych ; linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: [PATCH] [RESEND] scsi: fnic: use 64-bit timestamps struct timespec is deprecated since it overflows in 2038 on 32-bit architectures, so we should use timespec64 consistently. I'm slightly adapting the format strings here, to make sure we print the nanoseconds with the correct number of leading zeroes. Signed-off-by: Arnd Bergmann --- Originally submitted in November 2017. Martin asked Satish to review my patch, but I never heard back after that, so I'm trying again now. --- drivers/scsi/fnic/fnic_debugfs.c | 2 +- drivers/scsi/fnic/fnic_stats.h | 4 +-- drivers/scsi/fnic/fnic_trace.c | 58 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/drivers/scsi/fnic/fnic_debugfs.c b/drivers/scsi/fnic/fnic_debugfs.c index 9858484dd126..6d3e1cb4fea6 100644 --- a/drivers/scsi/fnic/fnic_debugfs.c +++ b/drivers/scsi/fnic/fnic_debugfs.c @@ -614,7 +614,7 @@ static ssize_t fnic_reset_stats_write(struct file *file, sizeof(struct io_path_stats) - sizeof(u64)); memset(fw_stats_p+1, 0, sizeof(struct fw_stats) - sizeof(u64)); - getnstimeofday(>stats_timestamps.last_reset_time); + ktime_get_real_ts64(>stats_timestamps.last_reset_time); } (*ppos)++; diff --git a/drivers/scsi/fnic/fnic_stats.h b/drivers/scsi/fnic/fnic_stats.h index e007feedbf72..9daa6ada6fa0 100644 --- a/drivers/scsi/fnic/fnic_stats.h +++ b/drivers/scsi/fnic/fnic_stats.h @@ -18,8 +18,8 @@ #define _FNIC_STATS_H_ struct stats_timestamps { - struct timespec last_reset_time; - struct timespec last_read_time; + struct timespec64 last_reset_time; + struct timespec64 last_read_time; }; struct io_path_stats { diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c index 4826f596cb31..abddde11982b 100644 --- a/drivers/scsi/fnic/fnic_trace.c +++ b/drivers/scsi/fnic/fnic_trace.c @@ -111,7 +111,7 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) int len = 0; unsigned long flags; char str[KSYM_SYMBOL_LEN]; - struct timespec val; + struct timespec64 val; fnic_trace_data_t *tbp; spin_lock_irqsave(_trace_lock, flags); @@ -129,10 +129,10 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) /* Convert function pointer to function name */ if (sizeof(unsigned long) < 8) { sprint_symbol(str, tbp->fnaddr.low); - jiffies_to_timespec(tbp->timestamp.low, ); + jiffies_to_timespec64(tbp->timestamp.low, ); } else { sprint_symbol(str, tbp->fnaddr.val); - jiffies_to_timespec(tbp->timestamp.val, ); + jiffies_to_timespec64(tbp->timestamp.val, ); } /* * Dump trace buffer entry to memory file @@ -140,8 +140,8 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) */ len += snprintf(fnic_dbgfs_prt->buffer + len, (trace_max_pages * PAGE_SIZE * 3) - len, - "%16lu.%16lu %-50s %8x %8x %16llx %16llx " - "%16llx %16llx %16llx\n", val.tv_sec, + "%16llu.%09lu %-50s %8x %8x %16llx %16llx " + "%16llx %16llx %16llx\n", (u64)val.tv_sec, val.tv_nsec, str, tbp->host_no, tbp->tag, tbp->data[0], tbp->data[1], tbp->data[2], tbp->data[3], tbp->data[4]); @@ -171,10 +171,10 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt) /* Convert function pointer to function name */ if (sizeof(unsigned long) < 8) { sprint_symbol(str, tbp->fnaddr.low); - jiffies_to_timespec(tbp->timestamp.low, ); + jiffies_to_timespec64(tbp->timestamp.low, ); } else { sprint_symbol(str, tbp->fnaddr.val); - jiffies_t