RE: [PATCH 0/5] bfa: fix W=1 build warnings
Hi, Thanks for the patches. Changes look fine to me, other than a minor change in patch-1 and the build issue due to the removal of bfa_isr_disable()declaration. Thanks, Sudarsana -Original Message- From: Arnd Bergmann [mailto:a...@arndb.de] Sent: 02 August 2016 20:53 To: Anil Gurumurthy <anil.gurumur...@qlogic.com>; Sudarsana Kalluru <sudarsana.kall...@qlogic.com> Cc: James E . J . Bottomley <j...@linux.vnet.ibm.com>; Martin K . Petersen <martin.peter...@oracle.com>; linux-scsi <linux-scsi@vger.kernel.org>; linux-kernel <linux-ker...@vger.kernel.org>; Baoyou Xie <baoyou@linaro.org>; Arnd Bergmann <a...@arndb.de> Subject: [PATCH 0/5] bfa: fix W=1 build warnings I tried to build an allmodconfig kernel with W=1, and the bfa driver stuck out for having the most warnings in one file, so I decided to send patches for all of these and make the driver build cleanly with the extra warnings enabled. As most warnings were for functions that lacked a 'static', I marked them that way, and also used a script to look for further functions that have a declaration but can also be static. Overall, this saves around 10% of the size of the driver module: textdata bss dec hex filename 2320273012 976 236015 399ef drivers/scsi/bfa/bfa-before.o 2218512996 908 225755 371db drivers/scsi/bfa/bfa-after.o Arnd Arnd Bergmann (5): bfa: mark symbols static where possible bfa: remove unused variables bfa: rename some global variables bfa: remove unused functions from fbbuild.c bfa: remove more unused functions drivers/scsi/bfa/bfa.h | 17 -- drivers/scsi/bfa/bfa_core.c | 54 +--- drivers/scsi/bfa/bfa_fcbuild.c | 609 +-- drivers/scsi/bfa/bfa_fcbuild.h | 90 -- drivers/scsi/bfa/bfa_fcpim.c | 81 +++--- drivers/scsi/bfa/bfa_fcpim.h | 26 -- drivers/scsi/bfa/bfa_fcs.c | 31 +- drivers/scsi/bfa/bfa_fcs.h | 54 drivers/scsi/bfa/bfa_fcs_lport.c | 211 +++--- drivers/scsi/bfa/bfa_fcs_rport.c | 48 +-- drivers/scsi/bfa/bfa_ioc.c | 83 ++ drivers/scsi/bfa/bfa_ioc.h | 25 -- drivers/scsi/bfa/bfa_ioc_cb.c| 3 +- drivers/scsi/bfa/bfa_ioc_ct.c| 12 +- drivers/scsi/bfa/bfa_plog.h | 8 - drivers/scsi/bfa/bfa_port.c | 6 +- drivers/scsi/bfa/bfa_port.h | 1 - drivers/scsi/bfa/bfa_svc.c | 95 ++ drivers/scsi/bfa/bfa_svc.h | 17 -- drivers/scsi/bfa/bfad.c | 128 drivers/scsi/bfa/bfad_attr.c | 9 +- drivers/scsi/bfa/bfad_bsg.c | 224 +++--- drivers/scsi/bfa/bfad_drv.h | 49 +--- drivers/scsi/bfa/bfad_im.c | 29 +- drivers/scsi/bfa/bfad_im.h | 10 - 25 files changed, 395 insertions(+), 1525 deletions(-) -- 2.9.0 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/5] bfa: mark symbols static where possible
Hi,
Thanks for the change. Following declarations also look to be referenced in
a single file and can be removed. Please check.
drivers/scsi/bfa/bfad_im.h:
extern struct scsi_host_template bfad_im_scsi_host_template;
extern struct scsi_transport_template *bfad_im_scsi_transport_template;
extern struct scsi_transport_template
*bfad_im_scsi_vport_transport_template;
Thanks,
Sudarsana
-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: 02 August 2016 20:53
To: Anil Gurumurthy <anil.gurumur...@qlogic.com>; Sudarsana Kalluru
<sudarsana.kall...@qlogic.com>
Cc: James E . J . Bottomley <j...@linux.vnet.ibm.com>; Martin K . Petersen
<martin.peter...@oracle.com>; linux-scsi <linux-scsi@vger.kernel.org>;
linux-kernel <linux-ker...@vger.kernel.org>; Baoyou Xie
<baoyou@linaro.org>; Arnd Bergmann <a...@arndb.de>
Subject: [PATCH 1/5] bfa: mark symbols static where possible
We get 128 warnings about global functions without a declaration
in the bfa scsi driver when building with W=1, more than any other
driver, e.g.
bfa/bfad.c:1502:1: error: no previous prototype for 'restart_bfa'
bfa/bfad_attr.c:447:1: error: no previous prototype for
'bfad_im_issue_fc_host_lip'
bfa/bfad_attr.c:575:1: error: no previous prototype for
'bfad_im_vport_set_symbolic_name'
bfa/bfad_bsg.c:27:1: error: no previous prototype for 'bfad_iocmd_ioc_enable'
bfa/bfad_bsg.c:50:1: error: no previous prototype for 'bfad_iocmd_ioc_disable'
bfa/bfad_bsg.c:148:1: error: no previous prototype for
'bfad_iocmd_ioc_get_stats'
In this case, all of those functions are only used in the file in
which they are declared and don't need a declaration, but can be
made static. A little more research reveals many more functions
in this driver that have a declaration but are also used only
in the same file, so this patch marks them all 'static' and moves
the declarations from a header file into the .c file where necessary.
Signed-off-by: Arnd Bergmann <a...@arndb.de>
---
drivers/scsi/bfa/bfa.h | 15 ---
drivers/scsi/bfa/bfa_core.c | 19 ++--
drivers/scsi/bfa/bfa_fcbuild.c | 2 +-
drivers/scsi/bfa/bfa_fcbuild.h | 3 -
drivers/scsi/bfa/bfa_fcpim.c | 66
drivers/scsi/bfa/bfa_fcpim.h | 25 -
drivers/scsi/bfa/bfa_fcs.c | 31 --
drivers/scsi/bfa/bfa_fcs.h | 41 ---
drivers/scsi/bfa/bfa_fcs_lport.c | 69
drivers/scsi/bfa/bfa_fcs_rport.c | 9 +-
drivers/scsi/bfa/bfa_ioc.c | 44
drivers/scsi/bfa/bfa_ioc.h | 23
drivers/scsi/bfa/bfa_ioc_cb.c| 3 +-
drivers/scsi/bfa/bfa_ioc_ct.c| 12 ++-
drivers/scsi/bfa/bfa_plog.h | 8 --
drivers/scsi/bfa/bfa_port.c | 6 +-
drivers/scsi/bfa/bfa_port.h | 1 -
drivers/scsi/bfa/bfa_svc.c | 38 ---
drivers/scsi/bfa/bfa_svc.h | 12 ---
drivers/scsi/bfa/bfad.c | 88 ---
drivers/scsi/bfa/bfad_attr.c | 4 +-
drivers/scsi/bfa/bfad_bsg.c | 224 +++
drivers/scsi/bfa/bfad_drv.h | 44 +---
drivers/scsi/bfa/bfad_im.c | 23 ++--
drivers/scsi/bfa/bfad_im.h | 10 --
25 files changed, 372 insertions(+), 448 deletions(-)
diff --git a/drivers/scsi/bfa/bfa.h b/drivers/scsi/bfa/bfa.h
index 0e119d838e1b..c3b499d126d5 100644
--- a/drivers/scsi/bfa/bfa.h
+++ b/drivers/scsi/bfa/bfa.h
@@ -31,11 +31,6 @@ typedef void (*bfa_isr_func_t) (struct bfa_s *bfa, struct
bfi_msg_s *m);
typedef void (*bfa_cb_cbfn_status_t) (void *cbarg, bfa_status_t status);
/*
- * Interrupt message handlers
- */
-void bfa_isr_unhandled(struct bfa_s *bfa, struct bfi_msg_s *m);
-
-/*
* Request and response queue related defines
*/
#define BFA_REQQ_NELEMS_MIN(4)
@@ -299,19 +294,11 @@ struct bfa_iocfc_s {
/*
* FC specific IOC functions.
*/
-void bfa_iocfc_meminfo(struct bfa_iocfc_cfg_s *cfg,
- struct bfa_meminfo_s *meminfo,
- struct bfa_s *bfa);
-void bfa_iocfc_attach(struct bfa_s *bfa, void *bfad,
- struct bfa_iocfc_cfg_s *cfg,
- struct bfa_pcidev_s *pcidev);
void bfa_iocfc_init(struct bfa_s *bfa);
void bfa_iocfc_start(struct bfa_s *bfa);
void bfa_iocfc_stop(struct bfa_s *bfa);
-void bfa_iocfc_isr(void *bfa, struct bfi_mbmsg_s *msg);
void bfa_iocfc_set_snsbase(struct bfa_s *bfa, int seg_no, u64 snsbase_pa);
bfa_boolean_t bfa_iocfc_is_operational(struct bfa_s *bfa);
-void bfa_iocfc_reset_queues(struct bfa_s *bfa);
void bfa_msix_all(struct bfa_s *bfa, int vec);
void bfa_msix_reqq(struct bfa_s *bfa, int vec);
@@ -413,8 +400,6 @@ void bfa_cb_init(void *bfad, bfa_status_t status);
void bfa_cb_updateq(void *bfad, bfa_status_t status);
bfa_boolean_t bfa_intx(struct bfa_s *bfa);
-void bfa_isr_enable(struct bfa_s *bfa);
-void bfa_isr_disable(struct bfa_s *bfa);
void bfa_comp_deq(struct bfa_s *bfa, struct list_h
RE: [patch] bfa: clean up some bounds checking
Both the approaches (i.e., using a loop or the strchr()) look fine to me.
Thanks,
Sudarsana
-Original Message-
From: walter harms [mailto:wha...@bfs.de]
Sent: 16 June 2016 17:50
To: Dan Carpenter <dan.carpen...@oracle.com>
Cc: Anil Gurumurthy <anil.gurumur...@qlogic.com>; Sudarsana Kalluru
<sudarsana.kall...@qlogic.com>; James E.J. Bottomley <j...@linux.vnet.ibm.com>;
Martin K. Petersen <martin.peter...@oracle.com>; linux-scsi
<linux-scsi@vger.kernel.org>; kernel-janit...@vger.kernel.org
Subject: Re: [patch] bfa: clean up some bounds checking
Am 16.06.2016 12:44, schrieb Dan Carpenter:
> This code is supposed to search ->adapter_hwpath[] and replace the
> second colon with a NUL character. Unfortunately, the boundary checks
> that ensure we don't go beyond the end of the buffer have a couple
> problems.
>
> Imagine that the string has no colons. In that case, in the first
> loop, we read one space beyond the end of the buffer and then exit the loop.
> In the next loop, we increment once, read two characters beyond the
> end of the buffer and then exit. Then after the loop we put a NUL
> character two characters past the end of the buffer.
>
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> ---
> This is from static analysis and not tested. Caveat emptor.
>
> diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
> index d1ad020..dfb26f0 100644
> --- a/drivers/scsi/bfa/bfad_bsg.c
> +++ b/drivers/scsi/bfa/bfad_bsg.c
> @@ -106,10 +106,17 @@ bfad_iocmd_ioc_get_info(struct bfad_s *bfad,
> void *cmd)
>
> /* set adapter hw path */
> strcpy(iocmd->adapter_hwpath, bfad->pci_name);
> - for (i = 0; iocmd->adapter_hwpath[i] != ':' && i < BFA_STRING_32; i++)
> - ;
> - for (; iocmd->adapter_hwpath[++i] != ':' && i < BFA_STRING_32; )
> - ;
> + i = -1;
> + while (++i < BFA_STRING_32) {
> + if (iocmd->adapter_hwpath[i] == ':')
> + break;
> + }
> + while (++i < BFA_STRING_32) {
> + if (iocmd->adapter_hwpath[i] == ':')
> + break;
> + }
> + if (i >= BFA_STRING_32)
> + i = BFA_STRING_32 - 1;
> iocmd->adapter_hwpath[i] = '\0';
> iocmd->status = BFA_STATUS_OK;
> return 0;
I do not see the use case but i assume
the idea is to have a string like aa:bb:something and kill everyhing after the
second ':' ?
/*
a few word may help here also inside the code */
second: maybe we can us strchr here ?
s1=strchr(iocmd->adapter_hwpath,':');
if (s1 != NULL ) s1=strchr(s1,":");
re,
wh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 5/11] drivers/scsi/bfa/bfad_bsg.c: Remove useless return variables
Thanks for the patch.
Acked-by: Sudarsana Kalluru sudarsana.kall...@qlogic.com
-Original Message-
From: linux-scsi-ow...@vger.kernel.org
[mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Peter Senna Tschudin
Sent: 31 May 2014 18:44
To: Anil Gurumurthy
Cc: kernel-janit...@vger.kernel.org; Sudarsana Kalluru; James E.J. Bottomley;
linux-scsi; linux-kernel
Subject: [PATCH 5/11] drivers/scsi/bfa/bfad_bsg.c: Remove useless return
variables
This patch remove variables that are initialized with a constant, are never
updated, and are only used as parameter of return.
Return the constant instead of using a variable.
Verified by compilation only.
The coccinelle script that find and fixes this issue is:
// smpl
@@
type T;
constant C;
identifier ret;
@@
- T ret = C;
... when != ret
when strict
return
- ret
+ C
;
// /smpl
Signed-off-by: Peter Senna Tschudin peter.se...@gmail.com
---
drivers/scsi/bfa/bfad_bsg.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c index
8994fb8..a6ee1cf 100644
--- a/drivers/scsi/bfa/bfad_bsg.c
+++ b/drivers/scsi/bfa/bfad_bsg.c
@@ -26,7 +26,6 @@ int
bfad_iocmd_ioc_enable(struct bfad_s *bfad, void *cmd) {
struct bfa_bsg_gen_s *iocmd = (struct bfa_bsg_gen_s *)cmd;
- int rc = 0;
unsigned long flags;
spin_lock_irqsave(bfad-bfad_lock, flags); @@ -34,7 +33,7 @@
bfad_iocmd_ioc_enable(struct bfad_s *bfad, void *cmd)
if (!bfa_ioc_is_disabled(bfad-bfa.ioc)) {
spin_unlock_irqrestore(bfad-bfad_lock, flags);
iocmd-status = BFA_STATUS_OK;
- return rc;
+ return 0;
}
init_completion(bfad-enable_comp);
@@ -43,21 +42,20 @@ bfad_iocmd_ioc_enable(struct bfad_s *bfad, void *cmd)
spin_unlock_irqrestore(bfad-bfad_lock, flags);
wait_for_completion(bfad-enable_comp);
- return rc;
+ return 0;
}
int
bfad_iocmd_ioc_disable(struct bfad_s *bfad, void *cmd) {
struct bfa_bsg_gen_s *iocmd = (struct bfa_bsg_gen_s *)cmd;
- int rc = 0;
unsigned long flags;
spin_lock_irqsave(bfad-bfad_lock, flags);
if (bfa_ioc_is_disabled(bfad-bfa.ioc)) {
spin_unlock_irqrestore(bfad-bfad_lock, flags);
iocmd-status = BFA_STATUS_OK;
- return rc;
+ return 0;
}
if (bfad-disable_active) {
@@ -74,7 +72,7 @@ bfad_iocmd_ioc_disable(struct bfad_s *bfad, void *cmd)
bfad-disable_active = BFA_FALSE;
iocmd-status = BFA_STATUS_OK;
- return rc;
+ return 0;
}
static int
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in the
body of a message to majord...@vger.kernel.org More majordomo info at
http://vger.kernel.org/majordomo-info.html
This message and any attached documents contain information from QLogic
Corporation or its wholly-owned subsidiaries that may be confidential. If you
are not the intended recipient, you may not read, copy, distribute, or use this
information. If you have received this transmission in error, please notify the
sender immediately by reply e-mail and then delete this message.
attachment: winmail.dat
