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

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

[Daeseok Youn]

Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
---
 drivers/staging/unisys/uislib/uislib.c   |    5 +----
 drivers/staging/unisys/uislib/uisutils.c |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/unisys/uislib/uislib.c b/drivers/staging/unisys/uislib/uislib.c
index d77df9a..9748fcb 100644
--- a/drivers/staging/unisys/uislib/uislib.c
+++ b/drivers/staging/unisys/uislib/uislib.c
@@ -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",
diff --git a/drivers/staging/unisys/uislib/uisutils.c b/drivers/staging/unisys/uislib/uisutils.c
index 208b7ea..2f05be1 100644
--- a/drivers/staging/unisys/uislib/uisutils.c
+++ b/drivers/staging/unisys/uislib/uisutils.c
@@ -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;
-- 
1.7.4.4

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[Greg KH]

On Wed, Mar 12, 2014 at 07:37:50PM +0900, Daeseok Youn wrote:
> 
> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
> ---
>  drivers/staging/unisys/uislib/uislib.c   |    5 +----
>  drivers/staging/unisys/uislib/uisutils.c |    2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/unisys/uislib/uislib.c b/drivers/staging/unisys/uislib/uislib.c
> index d77df9a..9748fcb 100644
> --- a/drivers/staging/unisys/uislib/uislib.c
> +++ b/drivers/staging/unisys/uislib/uislib.c
> @@ -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",
> diff --git a/drivers/staging/unisys/uislib/uisutils.c b/drivers/staging/unisys/uislib/uisutils.c
> index 208b7ea..2f05be1 100644
> --- a/drivers/staging/unisys/uislib/uisutils.c
> +++ b/drivers/staging/unisys/uislib/uisutils.c
> @@ -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

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[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?

Thanks.
Daeseok Youn.

2014-03-18 6:41 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
> On Wed, Mar 12, 2014 at 07:37:50PM +0900, Daeseok Youn wrote:
>>
>> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>> ---
>>  drivers/staging/unisys/uislib/uislib.c   |    5 +----
>>  drivers/staging/unisys/uislib/uisutils.c |    2 +-
>>  2 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/unisys/uislib/uislib.c b/drivers/staging/unisys/uislib/uislib.c
>> index d77df9a..9748fcb 100644
>> --- a/drivers/staging/unisys/uislib/uislib.c
>> +++ b/drivers/staging/unisys/uislib/uislib.c
>> @@ -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",
>> diff --git a/drivers/staging/unisys/uislib/uisutils.c b/drivers/staging/unisys/uislib/uisutils.c
>> index 208b7ea..2f05be1 100644
>> --- a/drivers/staging/unisys/uislib/uisutils.c
>> +++ b/drivers/staging/unisys/uislib/uisutils.c
>> @@ -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

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[Greg KH]

On Tue, Mar 18, 2014 at 09:26:07AM +0900, DaeSeok Youn wrote:
> 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.

> (UISMALLOC() macro is also removed.)

Yes.

> 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.

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

Drop it and just use kfree().

thanks,

greg k-h

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[DaeSeok Youn]

2014-03-18 9:37 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
> On Tue, Mar 18, 2014 at 09:26:07AM +0900, DaeSeok Youn wrote:
>> 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.

>
>> (UISMALLOC() macro is also removed.)
>
> Yes.
>
>> 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().

>
>> If this change is accepted, it also need to change uislib_free().
>
> Drop it and just use kfree().
OK. replace kfree() with uislib_free().

>
> thanks,
>
> greg k-h

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[Ken Cox]

On 03/17/2014 07:26 PM, DaeSeok Youn wrote:
> 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.
>
> Thanks.
> Daeseok Youn.
>
> 2014-03-18 6:41 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
>> On Wed, Mar 12, 2014 at 07:37:50PM +0900, Daeseok Youn wrote:
>>> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>>> ---
>>>   drivers/staging/unisys/uislib/uislib.c   |    5 +----
>>>   drivers/staging/unisys/uislib/uisutils.c |    2 +-
>>>   2 files changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/staging/unisys/uislib/uislib.c b/drivers/staging/unisys/uislib/uislib.c
>>> index d77df9a..9748fcb 100644
>>> --- a/drivers/staging/unisys/uislib/uislib.c
>>> +++ b/drivers/staging/unisys/uislib/uislib.c
>>> @@ -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",
>>> diff --git a/drivers/staging/unisys/uislib/uisutils.c b/drivers/staging/unisys/uislib/uisutils.c
>>> index 208b7ea..2f05be1 100644
>>> --- a/drivers/staging/unisys/uislib/uisutils.c
>>> +++ b/drivers/staging/unisys/uislib/uisutils.c
>>> @@ -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

[patch][wip] staging: unisys: kmalloc/memset move to kzalloc

[Silvio F]

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.

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

[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"

Signed-off-by: Silvio F <silvio.fricke@gmail.com>
---
 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(-)

diff --git a/drivers/staging/unisys/include/uisutils.h b/drivers/staging/unisys/include/uisutils.h
index 81b5d9b..0dd9c80 100644
--- a/drivers/staging/unisys/include/uisutils.h
+++ b/drivers/staging/unisys/include/uisutils.h
@@ -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
diff --git a/drivers/staging/unisys/virthba/virthba.c b/drivers/staging/unisys/virthba/virthba.c
index 8aefaa8..d99a38d 100644
--- a/drivers/staging/unisys/virthba/virthba.c
+++ b/drivers/staging/unisys/virthba/virthba.c
@@ -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;
diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c
index 1b64632..8e34650 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -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 */
diff --git a/drivers/staging/unisys/visorutil/memregion_direct.c b/drivers/staging/unisys/visorutil/memregion_direct.c
index 82c651d..b9b61e8 100644
--- a/drivers/staging/unisys/visorutil/memregion_direct.c
+++ b/drivers/staging/unisys/visorutil/memregion_direct.c
@@ -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;
diff --git a/drivers/staging/unisys/visorutil/periodic_work.c b/drivers/staging/unisys/visorutil/periodic_work.c
index cc1c678..fb1e894 100644
--- a/drivers/staging/unisys/visorutil/periodic_work.c
+++ b/drivers/staging/unisys/visorutil/periodic_work.c
@@ -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;
-- 
1.9.0

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

[Joe Perches]

On Tue, 2014-03-18 at 21:07 +0100, Silvio F wrote:
> This patch solves the Coccinelle warning: "kzalloc should be used
> instead of kmalloc/memset"
[]
> diff --git a/drivers/staging/unisys/include/uisutils.h b/drivers/staging/unisys/include/uisutils.h
[]
> @@ -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.

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

[Greg KH]

On Tue, Mar 18, 2014 at 09:07:51PM +0100, Silvio F wrote:
> 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 <silvio.fricke@gmail.com>
> ---
>  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

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

[Greg KH]

On Tue, Mar 18, 2014 at 01:17:25PM -0700, Joe Perches wrote:
> On Tue, 2014-03-18 at 21:07 +0100, Silvio F wrote:
> > This patch solves the Coccinelle warning: "kzalloc should be used
> > instead of kmalloc/memset"
> []
> > diff --git a/drivers/staging/unisys/include/uisutils.h b/drivers/staging/unisys/include/uisutils.h
> []
> > @@ -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.

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

thanks,

greg k-h

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

[Silvio F.]

Hi,

> On Tue, Mar 18, 2014 at 09:07:51PM +0100, Silvio F wrote:
> > 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 <silvio.fricke@gmail.com>
> > ---
> >  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 :(

ouu!

> Care to resend it properly?

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

Cheers,
Silvio

-- 
-- S. Fricke ---------------------------------------- silvio@port1024.net --
   Diplom-Informatiker (FH)
   Linux-Entwicklung             JABBER: silvio@conversation.port1024.net   
----------------------------------------------------------------------------

[patch][wip] staging: unisys: kmalloc/memset move to kzalloc

[Silvio F]

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

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

[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"

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

Signed-off-by: Silvio F <silvio.fricke@gmail.com>
---
 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(-)

diff --git a/drivers/staging/unisys/include/uisutils.h b/drivers/staging/unisys/include/uisutils.h
index 81b5d9b..effd893 100644
--- a/drivers/staging/unisys/include/uisutils.h
+++ b/drivers/staging/unisys/include/uisutils.h
@@ -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.
diff --git a/drivers/staging/unisys/virthba/virthba.c b/drivers/staging/unisys/virthba/virthba.c
index 8aefaa8..817b11d 100644
--- a/drivers/staging/unisys/virthba/virthba.c
+++ b/drivers/staging/unisys/virthba/virthba.c
@@ -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 */
diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c
index 1b64632..8e34650 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -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 */
diff --git a/drivers/staging/unisys/visorutil/memregion_direct.c b/drivers/staging/unisys/visorutil/memregion_direct.c
index 82c651d..b9b61e8 100644
--- a/drivers/staging/unisys/visorutil/memregion_direct.c
+++ b/drivers/staging/unisys/visorutil/memregion_direct.c
@@ -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;
diff --git a/drivers/staging/unisys/visorutil/periodic_work.c b/drivers/staging/unisys/visorutil/periodic_work.c
index cc1c678..fb1e894 100644
--- a/drivers/staging/unisys/visorutil/periodic_work.c
+++ b/drivers/staging/unisys/visorutil/periodic_work.c
@@ -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;
-- 
1.9.0

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[DaeSeok Youn]

Hi, greg.

Review my comment below.

Thanks.
Daeseok Youn.

2014-03-18 17:11 GMT+09:00 DaeSeok Youn <daeseok.youn@gmail.com>:
> 2014-03-18 9:37 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
>> On Tue, Mar 18, 2014 at 09:26:07AM +0900, DaeSeok Youn wrote:
>>> 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.
>
>>
>>> (UISMALLOC() macro is also removed.)
>>
>> Yes.
>>
>>> 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().
>
>>
>>> If this change is accepted, it also need to change uislib_free().
>>
>> Drop it and just use kfree().
> OK. replace kfree() with uislib_free().
>
>>
>> thanks,
>>
>> greg k-h

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[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.

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 <jkc@redhat.com>:
>
> On 03/17/2014 07:26 PM, DaeSeok Youn wrote:
>>
>> 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.
>
>>
>> Thanks.
>> Daeseok Youn.
>>
>> 2014-03-18 6:41 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
>>>
>>> On Wed, Mar 12, 2014 at 07:37:50PM +0900, Daeseok Youn wrote:
>>>>
>>>> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>>>> ---
>>>>   drivers/staging/unisys/uislib/uislib.c   |    5 +----
>>>>   drivers/staging/unisys/uislib/uisutils.c |    2 +-
>>>>   2 files changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/unisys/uislib/uislib.c
>>>> b/drivers/staging/unisys/uislib/uislib.c
>>>> index d77df9a..9748fcb 100644
>>>> --- a/drivers/staging/unisys/uislib/uislib.c
>>>> +++ b/drivers/staging/unisys/uislib/uislib.c
>>>> @@ -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",
>>>> diff --git a/drivers/staging/unisys/uislib/uisutils.c
>>>> b/drivers/staging/unisys/uislib/uisutils.c
>>>> index 208b7ea..2f05be1 100644
>>>> --- a/drivers/staging/unisys/uislib/uisutils.c
>>>> +++ b/drivers/staging/unisys/uislib/uisutils.c
>>>> @@ -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
>
>

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[Greg KH]

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

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

On Wed, Mar 19, 2014 at 09:09:59AM +0900, DaeSeok Youn wrote:
> 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

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[Greg KH]

On Wed, Mar 19, 2014 at 09:03:49AM +0900, DaeSeok Youn wrote:
> Hi, greg.
> 
> Review my comment below.

What comment?

confused,

greg k-h-

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[DaeSeok Youn]

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

My replay attach again, below.

> 2014-03-18 9:37 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
>> On Tue, Mar 18, 2014 at 09:26:07AM +0900, DaeSeok Youn wrote:
>>> 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.
> 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.
>
>>
>>> (UISMALLOC() macro is also removed.)
>>
>> Yes.
>>
>>> 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().
>
>>
>>> If this change is accepted, it also need to change uislib_free().
>>
>> Drop it and just use kfree().
> OK. replace kfree() with uislib_free().
>
>>
>> thanks,
>>
>> greg k-h

2014-03-19 9:58 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
> On Wed, Mar 19, 2014 at 09:03:49AM +0900, DaeSeok Youn wrote:
>> Hi, greg.
>>
>> Review my comment below.
>
> What comment?
>
> confused,
>
> greg k-h-

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[Greg KH]

On Wed, Mar 19, 2014 at 10:04:40AM +0900, DaeSeok Youn wrote:
> oh...
> You didn't get my reply about vmalloc usage.
> 
> My replay attach again, below.
> 
> > 2014-03-18 9:37 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
> >> On Tue, Mar 18, 2014 at 09:26:07AM +0900, DaeSeok Youn wrote:
> >>> 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.
> > 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

Re: [PATCH] staging: unisys: use kzalloc instead of kmalloc/memset 0

[DaeSeok Youn]

Thanks for reply.
I will do that.

Regards,
Daeseok Youn.

2014-03-19 11:31 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
> On Wed, Mar 19, 2014 at 10:04:40AM +0900, DaeSeok Youn wrote:
>> oh...
>> You didn't get my reply about vmalloc usage.
>>
>> My replay attach again, below.
>>
>> > 2014-03-18 9:37 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>:
>> >> On Tue, Mar 18, 2014 at 09:26:07AM +0900, DaeSeok Youn wrote:
>> >>> 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.
>> > 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