[PATCH] scsi: remove memset before memcpy

[PATCH] scsi: remove memset before memcpy

[Himanshu Jha]

calling memcpy immediately after memset with the same region of memory
makes memset redundant.

Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
---
 drivers/scsi/bfa/bfa_ioc.c                  | 1 -
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 --
 drivers/scsi/qla4xxx/ql4_os.c               | 1 -
 3 files changed, 4 deletions(-)

diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 256f4af..c37a4be 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -2763,7 +2763,6 @@ bfa_ioc_get_type(struct bfa_ioc_s *ioc)
 void
 bfa_ioc_get_adapter_serial_num(struct bfa_ioc_s *ioc, char *serial_num)
 {
-	memset((void *)serial_num, 0, BFA_ADAPTER_SERIAL_NUM_LEN);
 	memcpy((void *)serial_num,
 			(void *)ioc->attr->brcd_serialnum,
 			BFA_ADAPTER_SERIAL_NUM_LEN);
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 11bd2e6..20bbb9b 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -1505,8 +1505,6 @@ map_cmd_status(struct fusion_context *fusion,
 
 		scmd->result = (DID_OK << 16) | ext_status;
 		if (ext_status == SAM_STAT_CHECK_CONDITION) {
-			memset(scmd->sense_buffer, 0,
-			       SCSI_SENSE_BUFFERSIZE);
 			memcpy(scmd->sense_buffer, sense,
 			       SCSI_SENSE_BUFFERSIZE);
 			scmd->result |= DRIVER_SENSE << 24;
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 64c6fa5..c87658c 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -619,7 +619,6 @@ static void qla4xxx_create_chap_list(struct scsi_qla_host *ha)
 		goto exit_chap_list;
 	}
 
-	memset(ha->chap_list, 0, chap_size);
 	memcpy(ha->chap_list, chap_flash_data, chap_size);
 
 exit_chap_list:
-- 
2.7.4

Re: [PATCH] scsi: remove memset before memcpy

[Joe Perches]

On Wed, 2017-08-30 at 00:19 +0530, Himanshu Jha wrote:
> drivers/scsi/megaraid/megaraid_sas_fusion.c

I don't know if you did this with coccinelle.

If so, it would be good to show the tool and script
in the commit message.

If not, the input script is pretty simple.

$ cat memset_before_memcpy.cocci
@@
expression e1;
expression e2;
expression e3;
@@

-	memset(e1, 0, e3);
	memcpy(e1, e2, e3);
$

Adding a test to make sure e1 or e3 isn't
modified before any other code uses them
by doing

$ cat memset_before_memcpy_2.cocci
@@
expression e1;
expression e2;
expressi
on e3;
@@

-	memset(e1, 0, e3);
	... when != \( e1 \| e3 \)
	memcpy(e1, e2, e3);
$

finds more cases but there may be a
false positive if e1 is a passed
function argument and if the operation
isn't effectively atomic like below:

----------------------------------------------------------------
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -2556,7 +2556,6 @@ void br_multicast_get_stats(const struct
 	struct br_mcast_stats tdst;
 	int i;
 
-	memset(dest, 0, sizeof(*dest));
 	if (p)
 		stats = p->mcast_stats;
 	else

----------------------------------------------------------------

where the memcpy is the last line of the function.

Re: [PATCH] scsi: remove memset before memcpy

[Himanshu Jha]

On Tue, Aug 29, 2017 at 12:29:35PM -0700, Joe Perches wrote:
> On Wed, 2017-08-30 at 00:19 +0530, Himanshu Jha wrote:
> > drivers/scsi/megaraid/megaraid_sas_fusion.c
> 
> I don't know if you did this with coccinelle.

Yes, I did this with coccinelle.

> 
> If so, it would be good to show the tool and script
> in the commit message.

Alright, next time.
> 
> If not, the input script is pretty simple.
> 
> $ cat memset_before_memcpy.cocci
> @@
> expression e1;
> expression e2;
> expression e3;
> @@
> 
> -	memset(e1, 0, e3);
> 	memcpy(e1, e2, e3);
> $
> 
> Adding a test to make sure e1 or e3 isn't
> modified before any other code uses them
> by doing
> 
> $ cat memset_before_memcpy_2.cocci
> @@
> expression e1;
> expression e2;
> expressi
> on e3;
> @@
> 
> -	memset(e1, 0, e3);
> 	... when != \( e1 \| e3 \)
> 	memcpy(e1, e2, e3);
> $
> 
> finds more cases but there may be a
> false positive if e1 is a passed
> function argument and if the operation
> isn't effectively atomic like below:
>

I usually check all my diff before sending to see if any false positives
occured or not.

It's been 4 days since I learnt coccinelle and before I used to look
into every files for occurences or search the identifiers on
FreeElectrons.

>From now I will mention whatever tool I used on my patch!

Thanks

> ----------------------------------------------------------------
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -2556,7 +2556,6 @@ void br_multicast_get_stats(const struct
>  	struct br_mcast_stats tdst;
>  	int i;
>  
> -	memset(dest, 0, sizeof(*dest));
>  	if (p)
>  		stats = p->mcast_stats;
>  	else
> 
> ----------------------------------------------------------------
> 
> where the memcpy is the last line of the function.
>