Skip to contentSkip to conversation

staging: unisys: use kzalloc instead of kmalloc/memset 0

PATCH In review Daeseok Youn 6 participants 21 messages Mar 12, 2014 – Mar 19, 2014 Staging

lore
Share & tools
lore

Daeseok Youn

author
#0 lore raw

PATCH 1/1 staging: unisys: use kzalloc instead of kmalloc/memset 0

Signed-off-by Daeseok Youn <[email protected]>
---
 drivers/staging/unisys/uislib/uislib.c   |    5 +----
 drivers/staging/unisys/uislib/uisutils.c |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)
drivers/staging/unisys/uislib/uislib.c +1 -4
index d77df9a..9748fcb 100644
@@ -339,8 +339,6 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
 		return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
 	}
 
-	memset(bus, 0, size);
-
 	/* Currently by default, the bus Number is the GuestHandle.
 	 * Configure Bus message can override this.
 	 */

@@ -530,7 +528,6 @@ create_device(CONTROLVM_MESSAGE *msg, char *buf)
 		return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
 	}
 
-	memset(dev, 0, sizeof(struct device_info));
 	dev->channelTypeGuid = msg->cmd.createDevice.dataTypeGuid;
 	dev->intr = msg->cmd.createDevice.intr;
 	dev->channelAddr = msg->cmd.createDevice.channelAddr;

@@ -1437,7 +1434,7 @@ uislib_malloc(size_t siz, gfp_t gfp, U8 contiguous, char *fn, int ln)
 		* get memory for you (like, invoke oom killer), which
 		* will probably cripple the system.
 		*/
-		p = kmalloc(siz, gfp | __GFP_NORETRY);
+		p = kzalloc(siz, gfp | __GFP_NORETRY);
 	}
 	if (p == NULL) {
 		LOGERR("uislib_malloc failed to alloc %d bytes @%s:%d",
drivers/staging/unisys/uislib/uisutils.c +1 -1
index 208b7ea..2f05be1 100644
@@ -294,7 +294,7 @@ ReqHandlerAdd(GUID switchTypeGuid,
 	rc = UISMALLOC(sizeof(*rc), GFP_ATOMIC);
 	if (!rc)
 		return NULL;
-	memset(rc, 0, sizeof(*rc));
+
 	rc->switchTypeGuid = switchTypeGuid;
 	rc->controlfunc = controlfunc;
 	rc->min_channel_bytes = min_channel_bytes;
Signature · 1 line
1.7.4.4
maintainer
#1 lore raw
Quoted 42 lines · Daeseok Youn
Signed-off-by Daeseok Youn <[email protected]>
---
 drivers/staging/unisys/uislib/uislib.c   |    5 +----
 drivers/staging/unisys/uislib/uisutils.c |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)
drivers/staging/unisys/uislib/uislib.c +1 -4
index d77df9a..9748fcb 100644
@@ -339,8 +339,6 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
 		return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
 	}
 
-	memset(bus, 0, size);
-
 	/* Currently by default, the bus Number is the GuestHandle.
 	 * Configure Bus message can override this.
 	 */

@@ -530,7 +528,6 @@ create_device(CONTROLVM_MESSAGE *msg, char *buf)
 		return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
 	}
 
-	memset(dev, 0, sizeof(struct device_info));
 	dev->channelTypeGuid = msg->cmd.createDevice.dataTypeGuid;
 	dev->intr = msg->cmd.createDevice.intr;
 	dev->channelAddr = msg->cmd.createDevice.channelAddr;

@@ -1437,7 +1434,7 @@ uislib_malloc(size_t siz, gfp_t gfp, U8 contiguous, char *fn, int ln)
 		* get memory for you (like, invoke oom killer), which
 		* will probably cripple the system.
 		*/
-		p = kmalloc(siz, gfp | __GFP_NORETRY);
+		p = kzalloc(siz, gfp | __GFP_NORETRY);
 	}
 	if (p == NULL) {
 		LOGERR("uislib_malloc failed to alloc %d bytes @%s:%d",
drivers/staging/unisys/uislib/uisutils.c +0 -0
index 208b7ea..2f05be1 100644

drivers/staging/unisys/uislib/uisutils.c ReqHandlerAdd(GUID switchTypeGuid, · line 300

Quoted 8 lines
drivers/staging/unisys/uislib/uisutils.c +1 -1
@@ -294,7 +294,7 @@ ReqHandlerAdd(GUID switchTypeGuid,
 	rc = UISMALLOC(sizeof(*rc), GFP_ATOMIC);
 	if (!rc)
 		return NULL;
-	memset(rc, 0, sizeof(*rc));
+
 	rc->switchTypeGuid = switchTypeGuid;
 	rc->controlfunc = controlfunc;
 	rc->min_channel_bytes = min_channel_bytes;

Can you just remove the UISMALLOC() macro completly, so that it's easier to verify that changes like this are actually correct?

thanks,

greg k-h

author
#2 lore raw

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.
(UISMALLOC() macro is also removed.)
And uislib_malloc() is renamed to "uislib_trace_buffer_status()" which is just tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse ...) for info_proc_read_helper().

If this change is accepted, it also need to change uislib_free().

Is it fine to change like this?

Thanks.
Daeseok Youn.

2014-03-18 6:41 GMT+09:00 Greg KH <[email protected]>:

Quoted 58 lines · Daeseok Youn
Quoted 42 lines
Signed-off-by Daeseok Youn <[email protected]>
---
 drivers/staging/unisys/uislib/uislib.c   |    5 +----
 drivers/staging/unisys/uislib/uisutils.c |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)
drivers/staging/unisys/uislib/uislib.c +1 -4
index d77df9a..9748fcb 100644
@@ -339,8 +339,6 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
              return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
      }
 
-     memset(bus, 0, size);
-
      /* Currently by default, the bus Number is the GuestHandle.
       * Configure Bus message can override this.
       */

@@ -530,7 +528,6 @@ create_device(CONTROLVM_MESSAGE *msg, char *buf)
              return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
      }
 
-     memset(dev, 0, sizeof(struct device_info));
      dev->channelTypeGuid = msg->cmd.createDevice.dataTypeGuid;
      dev->intr = msg->cmd.createDevice.intr;
      dev->channelAddr = msg->cmd.createDevice.channelAddr;

@@ -1437,7 +1434,7 @@ uislib_malloc(size_t siz, gfp_t gfp, U8 contiguous, char *fn, int ln)
              * get memory for you (like, invoke oom killer), which
              * will probably cripple the system.
              */
-             p = kmalloc(siz, gfp | __GFP_NORETRY);
+             p = kzalloc(siz, gfp | __GFP_NORETRY);
      }
      if (p == NULL) {
              LOGERR("uislib_malloc failed to alloc %d bytes @%s:%d",
drivers/staging/unisys/uislib/uisutils.c +0 -0
index 208b7ea..2f05be1 100644

drivers/staging/unisys/uislib/uisutils.c ReqHandlerAdd(GUID switchTypeGuid, · line 300

Quoted 8 lines
drivers/staging/unisys/uislib/uisutils.c +1 -1
@@ -294,7 +294,7 @@ ReqHandlerAdd(GUID switchTypeGuid,
      rc = UISMALLOC(sizeof(*rc), GFP_ATOMIC);
      if (!rc)
              return NULL;
-     memset(rc, 0, sizeof(*rc));
+
      rc->switchTypeGuid = switchTypeGuid;
      rc->controlfunc = controlfunc;
      rc->min_channel_bytes = min_channel_bytes;

Can you just remove the UISMALLOC() macro completly, so that it's easier to verify that changes like this are actually correct?

thanks,

greg k-h

maintainer
#3 lore raw
Quoted 2 lines · DaeSeok Youn

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.

Yes. Actually, just use kmalloc, I don't knwo why vmalloc is being used, but cc: the driver maintainers just to be sure.

Quoted 1 line

(UISMALLOC() macro is also removed.)

Yes.

Quoted 3 lines

And uislib_malloc() is renamed to "uislib_trace_buffer_status()" which is just tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse ...) for info_proc_read_helper().

The whole tracing stuff needs to be ripped out, so no problem deleting it here as well.

Quoted 1 line

If this change is accepted, it also need to change uislib_free().

Drop it and just use kfree().

thanks,

greg k-h

author
#4 lore raw

2014-03-18 9:37 GMT+09:00 Greg KH <[email protected]>:

Quoted 6 lines · DaeSeok Youn
Quoted 2 lines

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.

Yes. Actually, just use kmalloc, I don't knwo why vmalloc is being used, but cc: the driver maintainers just to be sure.

It try to allocate 128KiB(131072byte) with vmalloc(). I think if it trying to allocate with kmalloc()
it has a possibility to fail because of memory fragmentation even if system has enough memory to use.
Just my opinion. If I'm wrong, let me know.

Quoted 11 lines
Quoted 2 lines

(UISMALLOC() macro is also removed.)

Yes.

Quoted 3 lines

And uislib_malloc() is renamed to "uislib_trace_buffer_status()" which is just tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse ...) for info_proc_read_helper().

The whole tracing stuff needs to be ripped out, so no problem deleting it here as well.

OK. I will remove that information in info_proc_read_helper().

Quoted 4 lines
Quoted 2 lines

If this change is accepted, it also need to change uislib_free().

Drop it and just use kfree().

OK. replace kfree() with uislib_free().

Quoted 4 lines

thanks,

greg k-h

author
#5 lore raw

Hi, greg.

Review my comment below.

Thanks.
Daeseok Youn.

2014-03-18 17:11 GMT+09:00 DaeSeok Youn <[email protected]>:

Quoted 38 lines

2014-03-18 9:37 GMT+09:00 Greg KH <[email protected]>:

Quoted 6 lines · DaeSeok Youn
Quoted 2 lines

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.

Yes. Actually, just use kmalloc, I don't knwo why vmalloc is being used, but cc: the driver maintainers just to be sure.

It try to allocate 128KiB(131072byte) with vmalloc(). I think if it trying to allocate with kmalloc()
it has a possibility to fail because of memory fragmentation even if system has enough memory to use.
Just my opinion. If I'm wrong, let me know.

Quoted 11 lines
Quoted 2 lines

(UISMALLOC() macro is also removed.)

Yes.

Quoted 3 lines

And uislib_malloc() is renamed to "uislib_trace_buffer_status()" which is just tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse ...) for info_proc_read_helper().

The whole tracing stuff needs to be ripped out, so no problem deleting it here as well.

OK. I will remove that information in info_proc_read_helper().

Quoted 4 lines
Quoted 2 lines

If this change is accepted, it also need to change uislib_free().

Drop it and just use kfree().

OK. replace kfree() with uislib_free().

Quoted 4 lines

thanks,

greg k-h

maintainer
#6 lore raw
Quoted 3 lines · DaeSeok Youn

Hi, greg.

Review my comment below.

What comment?

confused,

greg k-h-

author
#7 lore raw

oh...
You didn't get my reply about vmalloc usage.

My replay attach again, below.

Quoted 8 lines

2014-03-18 9:37 GMT+09:00 Greg KH <[email protected]>:

Quoted 6 lines · DaeSeok Youn
Quoted 2 lines

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.

Yes. Actually, just use kmalloc, I don't knwo why vmalloc is being used, but cc: the driver maintainers just to be sure.

Here, need to check by you.

Quoted 18 lines

It try to allocate 128KiB(131072byte) with vmalloc(). I think if it trying to allocate with kmalloc()
it has a possibility to fail because of memory fragmentation even if system has enough memory to use.
Just my opinion. If I'm wrong, let me know.

Quoted 11 lines
Quoted 2 lines

(UISMALLOC() macro is also removed.)

Yes.

Quoted 3 lines

And uislib_malloc() is renamed to "uislib_trace_buffer_status()" which is just tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse ...) for info_proc_read_helper().

The whole tracing stuff needs to be ripped out, so no problem deleting it here as well.

Quoted 12 lines

OK. I will remove that information in info_proc_read_helper().

Quoted 4 lines
Quoted 2 lines

If this change is accepted, it also need to change uislib_free().

Drop it and just use kfree().

OK. replace kfree() with uislib_free().

Quoted 4 lines

thanks,

greg k-h

2014-03-19 9:58 GMT+09:00 Greg KH <[email protected]>:

Quoted 10 lines · DaeSeok Youn
Quoted 3 lines

Hi, greg.

Review my comment below.

What comment?

confused,

greg k-h-

maintainer
#8 lore raw
Quoted 6 lines · DaeSeok Youn

oh...
You didn't get my reply about vmalloc usage.

My replay attach again, below.

Quoted 1 line

2014-03-18 9:37 GMT+09:00 Greg KH <[email protected]>:

Quoted 13 lines
Quoted 7 lines
Quoted 6 lines · DaeSeok Youn
Quoted 2 lines

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.

Yes. Actually, just use kmalloc, I don't knwo why vmalloc is being used, but cc: the driver maintainers just to be sure.

Here, need to check by you.

Quoted 5 lines

It try to allocate 128KiB(131072byte) with vmalloc(). I think if it trying to allocate with kmalloc()
it has a possibility to fail because of memory fragmentation even if system has enough memory to use.
Just my opinion. If I'm wrong, let me know.

Check to see just how big you are allocating, you should know based on the flags which code path happened uislib_malloc().

Just keep the logic the same and you should be fine.

greg k-h

author
#9 lore raw

Thanks for reply.
I will do that.

Regards,
Daeseok Youn.

2014-03-19 11:31 GMT+09:00 Greg KH <[email protected]>:

Quoted 27 lines · DaeSeok Youn
Quoted 6 lines

oh...
You didn't get my reply about vmalloc usage.

My replay attach again, below.

Quoted 1 line

2014-03-18 9:37 GMT+09:00 Greg KH <[email protected]>:

Quoted 13 lines
Quoted 7 lines
Quoted 6 lines · DaeSeok Youn
Quoted 2 lines

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.

Yes. Actually, just use kmalloc, I don't knwo why vmalloc is being used, but cc: the driver maintainers just to be sure.

Here, need to check by you.

Quoted 5 lines

It try to allocate 128KiB(131072byte) with vmalloc(). I think if it trying to allocate with kmalloc()
it has a possibility to fail because of memory fragmentation even if system has enough memory to use.
Just my opinion. If I'm wrong, let me know.

Check to see just how big you are allocating, you should know based on the flags which code path happened uislib_malloc().

Just keep the logic the same and you should be fine.

greg k-h

#10 lore raw
In reply to DaeSeok Youn
Quoted 10 lines · DaeSeok Youn

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.
(UISMALLOC() macro is also removed.)
And uislib_malloc() is renamed to "uislib_trace_buffer_status()" which is just tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse ...) for info_proc_read_helper().

If this change is accepted, it also need to change uislib_free().

Is it fine to change like this?

This change is fine with me. It makes the logic easier to follow.

Quoted 61 lines

Thanks.
Daeseok Youn.

2014-03-18 6:41 GMT+09:00 Greg KH <[email protected]>:

Quoted 56 lines · Daeseok Youn
Quoted 41 lines
Signed-off-by Daeseok Youn <[email protected]>
---
  drivers/staging/unisys/uislib/uislib.c   |    5 +----
  drivers/staging/unisys/uislib/uisutils.c |    2 +-
  2 files changed, 2 insertions(+), 5 deletions(-)
drivers/staging/unisys/uislib/uislib.c +1 -4
index d77df9a..9748fcb 100644
@@ -339,8 +339,6 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
               return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
       }
 
-     memset(bus, 0, size);
-
       /* Currently by default, the bus Number is the GuestHandle.
        * Configure Bus message can override this.
        */

@@ -530,7 +528,6 @@ create_device(CONTROLVM_MESSAGE *msg, char *buf)
               return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
       }
 
-     memset(dev, 0, sizeof(struct device_info));
       dev->channelTypeGuid = msg->cmd.createDevice.dataTypeGuid;
       dev->intr = msg->cmd.createDevice.intr;
       dev->channelAddr = msg->cmd.createDevice.channelAddr;

@@ -1437,7 +1434,7 @@ uislib_malloc(size_t siz, gfp_t gfp, U8 contiguous, char *fn, int ln)
               * get memory for you (like, invoke oom killer), which
               * will probably cripple the system.
               */
-             p = kmalloc(siz, gfp | __GFP_NORETRY);
+             p = kzalloc(siz, gfp | __GFP_NORETRY);
       }
       if (p == NULL) {
               LOGERR("uislib_malloc failed to alloc %d bytes @%s:%d",
drivers/staging/unisys/uislib/uisutils.c +0 -0
index 208b7ea..2f05be1 100644

drivers/staging/unisys/uislib/uisutils.c ReqHandlerAdd(GUID switchTypeGuid, · line 300

Quoted 8 lines
drivers/staging/unisys/uislib/uisutils.c +1 -1
@@ -294,7 +294,7 @@ ReqHandlerAdd(GUID switchTypeGuid,
       rc = UISMALLOC(sizeof(*rc), GFP_ATOMIC);
       if (!rc)
               return NULL;
-     memset(rc, 0, sizeof(*rc));
+
       rc->switchTypeGuid = switchTypeGuid;
       rc->controlfunc = controlfunc;
       rc->min_channel_bytes = min_channel_bytes;

Can you just remove the UISMALLOC() macro completly, so that it's easier to verify that changes like this are actually correct?

thanks,

greg k-h

author
#11 lore raw

Hi, Ken
Thanks for review.

But I have a question, I wan to know why tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse...) for info_proc_read_helper() are needed.
If it doesn't need, whole tracing stuff for memory can be removed.

After getting a reply from you and greg, I will make a patch. Regards,
Daeseok Youn.

2014-03-18 22:00 GMT+09:00 Ken Cox <[email protected]>:

Quoted 85 lines · DaeSeok Youn
Quoted 11 lines

I think vmalloc/kmalloc in uislib_malloc() can be removed and just use vmalloc/kmalloc directly.
(UISMALLOC() macro is also removed.)
And uislib_malloc() is renamed to "uislib_trace_buffer_status()" which is just tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse ...) for info_proc_read_helper().

If this change is accepted, it also need to change uislib_free().

Is it fine to change like this?

This change is fine with me. It makes the logic easier to follow.

Quoted 67 lines

Thanks.
Daeseok Youn.

2014-03-18 6:41 GMT+09:00 Greg KH <[email protected]>:

Quoted 62 lines · Daeseok Youn
Quoted 45 lines
Signed-off-by Daeseok Youn <[email protected]>
---
  drivers/staging/unisys/uislib/uislib.c   |    5 +----
  drivers/staging/unisys/uislib/uisutils.c |    2 +-
  2 files changed, 2 insertions(+), 5 deletions(-)
drivers/staging/unisys/uislib/uislib.c +1 -4
index d77df9a..9748fcb 100644
@@ -339,8 +339,6 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
               return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
       }
 
-     memset(bus, 0, size);
-
       /* Currently by default, the bus Number is the GuestHandle.
        * Configure Bus message can override this.
        */

@@ -530,7 +528,6 @@ create_device(CONTROLVM_MESSAGE *msg, char *buf)
               return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
       }
 
-     memset(dev, 0, sizeof(struct device_info));
       dev->channelTypeGuid = msg->cmd.createDevice.dataTypeGuid;
       dev->intr = msg->cmd.createDevice.intr;
       dev->channelAddr = msg->cmd.createDevice.channelAddr;

@@ -1437,7 +1434,7 @@ uislib_malloc(size_t siz, gfp_t gfp, U8
 contiguous, char *fn, int ln)
               * get memory for you (like, invoke oom killer), which
               * will probably cripple the system.
               */
-             p = kmalloc(siz, gfp | __GFP_NORETRY);
+             p = kzalloc(siz, gfp | __GFP_NORETRY);
       }
       if (p == NULL) {

LOGERR("uislib_malloc failed to alloc %d bytes @%s:%d",

drivers/staging/unisys/uislib/uisutils.c +0 -0
index 208b7ea..2f05be1 100644

drivers/staging/unisys/uislib/uisutils.c ReqHandlerAdd(GUID switchTypeGuid, · line 300

Quoted 8 lines
drivers/staging/unisys/uislib/uisutils.c +1 -1
@@ -294,7 +294,7 @@ ReqHandlerAdd(GUID switchTypeGuid,
       rc = UISMALLOC(sizeof(*rc), GFP_ATOMIC);
       if (!rc)
               return NULL;
-     memset(rc, 0, sizeof(*rc));
+
       rc->switchTypeGuid = switchTypeGuid;
       rc->controlfunc = controlfunc;
       rc->min_channel_bytes = min_channel_bytes;

Can you just remove the UISMALLOC() macro completly, so that it's easier to verify that changes like this are actually correct?

thanks,

greg k-h

maintainer
#12 lore raw

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

Quoted 7 lines · DaeSeok Youn

Hi, Ken
Thanks for review.

But I have a question, I wan to know why tracing buffer status(Malloc_FailuresAlloc, Malloc_BytesInUse...) for info_proc_read_helper() are needed.
If it doesn't need, whole tracing stuff for memory can be removed.

Yes, that is something that also should be removed.

greg k-h

#13 lore raw

PATCH 1/1 staging: unisys: kmalloc/memset move to kzalloc

In reply to Daeseok Youn

Hi,

I have followed this thread and have seen that not all parts was kcalloc'ized. Please review this patch and maybe include it.

This patch is WIP because I have no hardware and can only do a compile test.

#14 lore raw

PATCH 1/1 staging: unisys: kmalloc/memset to kzalloc conversation

This patch solves the Coccinelle warning: "kzalloc should be used instead of kmalloc/memset"

This patch is a fixup for

	linux-next: 97a84f1203786985856a0d4b49b1d7cc387238ce
	"Staging: unisys: Replace kmalloc/memset with kzalloc"
Signed-off-by Silvio F <[email protected]>
---
 drivers/staging/unisys/include/uisutils.h           | 5 +----
 drivers/staging/unisys/virthba/virthba.c            | 3 +--
 drivers/staging/unisys/virtpci/virtpci.c            | 3 +--
 drivers/staging/unisys/visorutil/memregion_direct.c | 5 ++---
 drivers/staging/unisys/visorutil/periodic_work.c    | 5 ++---
 5 files changed, 7 insertions(+), 14 deletions(-)
drivers/staging/unisys/include/uisutils.h +1 -4
index 81b5d9b..0dd9c80 100644
@@ -190,10 +190,7 @@ struct chaninfo {
 }
 
 #define ALLOC_CMDRSP(cmdrsp) { \
-	cmdrsp = kmalloc(SIZEOF_CMDRSP, GFP_ATOMIC); \
-	if (cmdrsp != NULL) { \
-		memset(cmdrsp, 0, SIZEOF_CMDRSP); \
-	} \
+	cmdrsp = kzalloc(SIZEOF_CMDRSP, GFP_ATOMIC); \
 }
 
 /* This is a hack until we fix IOVM to initialize the channel header
drivers/staging/unisys/virthba/virthba.c +1 -2
index 8aefaa8..d99a38d 100644
@@ -402,9 +402,8 @@ process_disk_notify(struct Scsi_Host *shost, struct uiscmdrsp *cmdrsp)
 	struct diskaddremove *dar;
 	unsigned long flags;
 
-	dar = kmalloc(sizeof(struct diskaddremove), GFP_ATOMIC);
+	dar = kzalloc(sizeof(struct diskaddremove), GFP_ATOMIC);
 	if (dar) {
-		memset(dar, 0, sizeof(struct diskaddremove));
 		dar->add = cmdrsp->disknotify.add;
 		dar->shost = shost;
 		dar->channel = cmdrsp->disknotify.channel;
drivers/staging/unisys/virtpci/virtpci.c +1 -2
index 1b64632..8e34650 100644
@@ -254,13 +254,12 @@ static int add_vbus(struct add_vbus_guestpart *addparams)
 {
 	int ret;
 	struct device *vbus;
-	vbus = kmalloc(sizeof(struct device), GFP_ATOMIC);
+	vbus = kzalloc(sizeof(struct device), GFP_ATOMIC);
 
 	POSTCODE_LINUX_2(VPCI_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
 	if (!vbus)
 		return 0;
 
-	memset(vbus, 0, sizeof(struct device));
 	dev_set_name(vbus, "vbus%d", addparams->busNo);
 	vbus->release = virtpci_bus_release;
 	vbus->parent = &virtpci_rootbus_device;	/* root bus is parent */
drivers/staging/unisys/visorutil/memregion_direct.c +2 -3
index 82c651d..b9b61e8 100644
@@ -41,13 +41,12 @@ MEMREGION *
 visor_memregion_create(HOSTADDRESS physaddr, ulong nbytes)
 {
 	MEMREGION *rc = NULL;
-	MEMREGION *memregion = kmalloc(sizeof(MEMREGION),
-				       GFP_KERNEL|__GFP_NORETRY);
+	MEMREGION *memregion = kzalloc(sizeof(MEMREGION),
+				       GFP_KERNEL | __GFP_NORETRY);
 	if (memregion == NULL) {
 		ERRDRV("visor_memregion_create allocation failed");
 		return NULL;
 	}
-	memset(memregion, 0, sizeof(MEMREGION));
 	memregion->physaddr = physaddr;
 	memregion->nbytes = nbytes;
 	memregion->overlapped = FALSE;
drivers/staging/unisys/visorutil/periodic_work.c +2 -3
index cc1c678..fb1e894 100644
@@ -56,13 +56,12 @@ PERIODIC_WORK *visor_periodic_work_create(ulong jiffy_interval,
 					  void *workfuncarg,
 					  const char *devnam)
 {
-	PERIODIC_WORK *periodic_work = kmalloc(sizeof(PERIODIC_WORK),
-					       GFP_KERNEL|__GFP_NORETRY);
+	PERIODIC_WORK *periodic_work = kzalloc(sizeof(PERIODIC_WORK),
+					       GFP_KERNEL | __GFP_NORETRY);
 	if (periodic_work == NULL) {
 		ERRDRV("periodic_work allocation failed ");
 		return NULL;
 	}
-	memset(periodic_work, '\0', sizeof(PERIODIC_WORK));
 	rwlock_init(&periodic_work->lock);
 	periodic_work->jiffy_interval = jiffy_interval;
 	periodic_work->workqueue = workqueue;
Signature · 1 line
1.9.0
#15 lore raw

Re: [PATCH] staging: unisys: kmalloc/memset to kzalloc conversation

Quoted 2 lines · Silvio F

This patch solves the Coccinelle warning: "kzalloc should be used instead of kmalloc/memset"

[]

drivers/staging/unisys/include/uisutils.h

Quoted 1 line
drivers/staging/unisys/include/uisutils.h +0 -0

[]

drivers/staging/unisys/include/uisutils.h struct chaninfo { · line 194

Quoted 10 lines
drivers/staging/unisys/include/uisutils.h +1 -4
@@ -190,10 +190,7 @@ struct chaninfo {
 }
 
 #define ALLOC_CMDRSP(cmdrsp) { \
-	cmdrsp = kmalloc(SIZEOF_CMDRSP, GFP_ATOMIC); \
-	if (cmdrsp != NULL) { \
-		memset(cmdrsp, 0, SIZEOF_CMDRSP); \
-	} \
+	cmdrsp = kzalloc(SIZEOF_CMDRSP, GFP_ATOMIC); \
 }

It'd be nicer to get rid of this macro altogether
and substitute kzalloc in the 3 places it's used.

maintainer
#16 lore raw

Re: [PATCH] staging: unisys: kmalloc/memset to kzalloc conversation

Quoted 9 lines · Joe Perches
Quoted 2 lines · Silvio F

This patch solves the Coccinelle warning: "kzalloc should be used instead of kmalloc/memset"

[]

drivers/staging/unisys/include/uisutils.h

Quoted 1 line
drivers/staging/unisys/include/uisutils.h +0 -0

[]

Quoted 3 lines
drivers/staging/unisys/include/uisutils.h +0 -0
@@ -190,10 +190,7 @@ struct chaninfo {
 }
Quoted 10 lines

drivers/staging/unisys/include/uisutils.h struct chaninfo { · line 194

Quoted 7 lines
drivers/staging/unisys/include/uisutils.h +1 -4
 #define ALLOC_CMDRSP(cmdrsp) { \
-	cmdrsp = kmalloc(SIZEOF_CMDRSP, GFP_ATOMIC); \
-	if (cmdrsp != NULL) { \
-		memset(cmdrsp, 0, SIZEOF_CMDRSP); \
-	} \
+	cmdrsp = kzalloc(SIZEOF_CMDRSP, GFP_ATOMIC); \
 }

It'd be nicer to get rid of this macro altogether
and substitute kzalloc in the 3 places it's used.

Yes, that's the better thing to do overall.

thanks,

greg k-h

maintainer
#17 lore raw

Re: [PATCH] staging: unisys: kmalloc/memset to kzalloc conversation

In reply to Silvio F
Quoted 8 lines · Silvio F

This patch solves the Coccinelle warning: "kzalloc should be used instead of kmalloc/memset"

This patch is a fixup for

	linux-next: 97a84f1203786985856a0d4b49b1d7cc387238ce
	"Staging: unisys: Replace kmalloc/memset with kzalloc"
Quoted 8 lines
Signed-off-by Silvio F <[email protected]>
---
 drivers/staging/unisys/include/uisutils.h           | 5 +----
 drivers/staging/unisys/virthba/virthba.c            | 3 +--
 drivers/staging/unisys/virtpci/virtpci.c            | 3 +--
 drivers/staging/unisys/visorutil/memregion_direct.c | 5 ++---
 drivers/staging/unisys/visorutil/periodic_work.c    | 5 ++---
 5 files changed, 7 insertions(+), 14 deletions(-)

If you don't cc: the maintainers of the code, they will never see this and it can't be applied :(

Care to resend it properly?

thanks,

greg k-h

#18 lore raw

Re: [PATCH] staging: unisys: kmalloc/memset to kzalloc conversation

Hi,

Quoted 18 lines · Silvio F
Quoted 8 lines

This patch solves the Coccinelle warning: "kzalloc should be used instead of kmalloc/memset"

This patch is a fixup for

	linux-next: 97a84f1203786985856a0d4b49b1d7cc387238ce
	"Staging: unisys: Replace kmalloc/memset with kzalloc"
Quoted 8 lines
Signed-off-by Silvio F <[email protected]>
---
 drivers/staging/unisys/include/uisutils.h           | 5 +----
 drivers/staging/unisys/virthba/virthba.c            | 3 +--
 drivers/staging/unisys/virtpci/virtpci.c            | 3 +--
 drivers/staging/unisys/visorutil/memregion_direct.c | 5 ++---
 drivers/staging/unisys/visorutil/periodic_work.c    | 5 ++---
 5 files changed, 7 insertions(+), 14 deletions(-)
Quoted 2 lines

If you don't cc: the maintainers of the code, they will never see this and it can't be applied :(

ouu!

Quoted 1 line

Care to resend it properly?

Yes I will.
I rework and add Joe Perches recommendation into the new set.

Cheers,
Silvio

Signature · 4 lines
-- S. Fricke ---------------------------------------- [email protected] --
   Diplom-Informatiker (FH)
   Linux-Entwicklung             JABBER: [email protected]   
----------------------------------------------------------------------------
#19 lore raw

PATCH 1/1 staging: unisys: kmalloc/memset move to kzalloc

In reply to Daeseok Youn

Hi,

I have followed this thread and have seen that not all parts was kcalloc'ized. Please review this patch and maybe include it.

This patch is WIP because I have no hardware and can only do a compile test.

version 2:
* remove ALLOC_CMDRSP define

Bye,
Silvio

#20 lore raw

PATCH 1/1 staging: unisys: kmalloc/memset to kzalloc conversation

This patch solves the Coccinelle warning: "kzalloc should be used instead of kmalloc/memset"

This patch is a fixup for

	linux-next: 97a84f1203786985856a0d4b49b1d7cc387238ce
	"Staging: unisys: Replace kmalloc/memset with kzalloc"

The ALLOC_CMDRSP #define is after transformation to kzalloc only a rename for kzalloc and was completly removed.

Signed-off-by Silvio F <[email protected]>
---
 drivers/staging/unisys/include/uisutils.h           | 7 -------
 drivers/staging/unisys/virthba/virthba.c            | 9 ++++-----
 drivers/staging/unisys/virtpci/virtpci.c            | 3 +--
 drivers/staging/unisys/visorutil/memregion_direct.c | 5 ++---
 drivers/staging/unisys/visorutil/periodic_work.c    | 5 ++---
 5 files changed, 9 insertions(+), 20 deletions(-)
drivers/staging/unisys/include/uisutils.h +0 -7
index 81b5d9b..effd893 100644
@@ -189,13 +189,6 @@ struct chaninfo {
 	schedule_timeout((x)*HZ); \
 }
 
-#define ALLOC_CMDRSP(cmdrsp) { \
-	cmdrsp = kmalloc(SIZEOF_CMDRSP, GFP_ATOMIC); \
-	if (cmdrsp != NULL) { \
-		memset(cmdrsp, 0, SIZEOF_CMDRSP); \
-	} \
-}
-
 /* This is a hack until we fix IOVM to initialize the channel header
  * correctly at DEVICE_CREATE time, INSTEAD OF waiting until
  * DEVICE_CONFIGURE time.
drivers/staging/unisys/virthba/virthba.c +4 -5
index 8aefaa8..817b11d 100644
@@ -402,9 +402,8 @@ process_disk_notify(struct Scsi_Host *shost, struct uiscmdrsp *cmdrsp)
 	struct diskaddremove *dar;
 	unsigned long flags;
 
-	dar = kmalloc(sizeof(struct diskaddremove), GFP_ATOMIC);
+	dar = kzalloc(sizeof(struct diskaddremove), GFP_ATOMIC);
 	if (dar) {
-		memset(dar, 0, sizeof(struct diskaddremove));
 		dar->add = cmdrsp->disknotify.add;
 		dar->shost = shost;
 		dar->channel = cmdrsp->disknotify.channel;

@@ -697,7 +696,7 @@ forward_vdiskmgmt_command(VDISK_MGMT_TYPES vdiskcmdtype,
 		return FAILED;
 	}
 
-	ALLOC_CMDRSP(cmdrsp);
+	cmdrsp = kzalloc(SIZEOF_CMDRSP, GFP_ATOMIC);
 	if (cmdrsp == NULL) {
 		LOGERR("kmalloc of cmdrsp failed.\n");
 		return FAILED;	/* reject */

@@ -759,7 +758,7 @@ forward_taskmgmt_command(TASK_MGMT_TYPES tasktype, struct scsi_device *scsidev)
 		return FAILED;
 	}
 
-	ALLOC_CMDRSP(cmdrsp);
+	cmdrsp = kzalloc(SIZEOF_CMDRSP, GFP_ATOMIC);
 	if (cmdrsp == NULL) {
 		LOGERR("kmalloc of cmdrsp failed.\n");
 		return FAILED;	/* reject */

@@ -930,7 +929,7 @@ virthba_queue_command_lck(struct scsi_cmnd *scsicmd,
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 	}
 
-	ALLOC_CMDRSP(cmdrsp);
+	cmdrsp = kzalloc(SIZEOF_CMDRSP, GFP_ATOMIC);
 	if (cmdrsp == NULL) {
 		LOGERR("kmalloc of cmdrsp failed.\n");
 		return 1;	/* reject the command */
drivers/staging/unisys/virtpci/virtpci.c +1 -2
index 1b64632..8e34650 100644
@@ -254,13 +254,12 @@ static int add_vbus(struct add_vbus_guestpart *addparams)
 {
 	int ret;
 	struct device *vbus;
-	vbus = kmalloc(sizeof(struct device), GFP_ATOMIC);
+	vbus = kzalloc(sizeof(struct device), GFP_ATOMIC);
 
 	POSTCODE_LINUX_2(VPCI_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
 	if (!vbus)
 		return 0;
 
-	memset(vbus, 0, sizeof(struct device));
 	dev_set_name(vbus, "vbus%d", addparams->busNo);
 	vbus->release = virtpci_bus_release;
 	vbus->parent = &virtpci_rootbus_device;	/* root bus is parent */
drivers/staging/unisys/visorutil/memregion_direct.c +2 -3
index 82c651d..b9b61e8 100644
@@ -41,13 +41,12 @@ MEMREGION *
 visor_memregion_create(HOSTADDRESS physaddr, ulong nbytes)
 {
 	MEMREGION *rc = NULL;
-	MEMREGION *memregion = kmalloc(sizeof(MEMREGION),
-				       GFP_KERNEL|__GFP_NORETRY);
+	MEMREGION *memregion = kzalloc(sizeof(MEMREGION),
+				       GFP_KERNEL | __GFP_NORETRY);
 	if (memregion == NULL) {
 		ERRDRV("visor_memregion_create allocation failed");
 		return NULL;
 	}
-	memset(memregion, 0, sizeof(MEMREGION));
 	memregion->physaddr = physaddr;
 	memregion->nbytes = nbytes;
 	memregion->overlapped = FALSE;
drivers/staging/unisys/visorutil/periodic_work.c +2 -3
index cc1c678..fb1e894 100644
@@ -56,13 +56,12 @@ PERIODIC_WORK *visor_periodic_work_create(ulong jiffy_interval,
 					  void *workfuncarg,
 					  const char *devnam)
 {
-	PERIODIC_WORK *periodic_work = kmalloc(sizeof(PERIODIC_WORK),
-					       GFP_KERNEL|__GFP_NORETRY);
+	PERIODIC_WORK *periodic_work = kzalloc(sizeof(PERIODIC_WORK),
+					       GFP_KERNEL | __GFP_NORETRY);
 	if (periodic_work == NULL) {
 		ERRDRV("periodic_work allocation failed ");
 		return NULL;
 	}
-	memset(periodic_work, '\0', sizeof(PERIODIC_WORK));
 	rwlock_init(&periodic_work->lock);
 	periodic_work->jiffy_interval = jiffy_interval;
 	periodic_work->workqueue = workqueue;
Signature · 1 line
1.9.0