Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[Derek Chickles]

> From: wanghai (M) <wanghai38@huawei.com>
> Subject: [EXT] Re: [PATCH net-next] liquidio: Remove unneeded cast from
> memory allocation
> 
> 在 2020/7/28 17:11, Joe Perches 写道:
> > On Tue, 2020-07-28 at 16:42 +0800, wanghai (M) wrote:
> >> 在 2020/7/25 5:29, Joe Perches 写道:
> >>> On Fri, 2020-07-24 at 21:00 +0800, Wang Hai wrote:
> >>>> Remove casting the values returned by memory allocation function.
> >>>>
> >>>> Coccinelle emits WARNING:
> >>>>
> >>>> ./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36:
> WARNING:
> >>>>    casting value returned by memory allocation function to (struct
> octeon_dispatch *) is useless.
> >>> []
> >>>> diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> >>>> b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> >>> []
> >>>> @@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct
> >>>> octeon_device *oct,
> >>>>
> >>>>    		dev_dbg(&oct->pci_dev->dev,
> >>>>    			"Adding opcode to dispatch list linked list\n");
> >>>> -		dispatch = (struct octeon_dispatch *)
> >>>> -			   vmalloc(sizeof(struct octeon_dispatch));
> >>>> +		dispatch = vmalloc(sizeof(struct octeon_dispatch));
> >>> More the question is why this is vmalloc at all as the structure
> >>> size is very small.
> >>>
> >>> Likely this should just be kmalloc.
> >>>
> >>>
> >> Thanks for your advice.  It is indeed best to use kmalloc here.

I think that is fine as well. We just used vmalloc since there is no need
for a physically contiguous piece of memory.

...

> 
> Can it be modified like this?
> 
> --- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> +++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> @@ -1152,11 +1152,8 @@ octeon_register_dispatch_fn(struct
> octeon_device *oct,
> 
>                  dev_dbg(&oct->pci_dev->dev,
>                          "Adding opcode to dispatch list linked list\n");
> -               dispatch = (struct octeon_dispatch *)
> -                          vmalloc(sizeof(struct octeon_dispatch));
> +               dispatch = kmalloc(sizeof(struct octeon_dispatch),
> GFP_KERNEL);
>                  if (!dispatch) {
> -                       dev_err(&oct->pci_dev->dev,
> -                               "No memory to add dispatch function\n");
>                          return 1;
>                  }
>                  dispatch->opcode = combined_opcode;

Looks fine to me.

Thanks,
Derek

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[Joe Perches]

On Tue, 2020-07-28 at 15:39 +0000, Derek Chickles wrote:
> I think that is fine as well. We just used vmalloc since there is no need
> for a physically contiguous piece of memory.

Do any of the allocs in the driver actually need vmalloc?

[PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[YueHaibing]

Remove unneeded return value cast.
This is detected by coccinelle.

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/cavium/liquidio/octeon_droq.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_droq.c b/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
index 017169023cca..cf4fe5b17f8a 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
@@ -280,13 +280,10 @@ int octeon_init_droq(struct octeon_device *oct,
 	dev_dbg(&oct->pci_dev->dev, "droq[%d]: num_desc: %d\n", q_no,
 		droq->max_count);
 
-	droq->recv_buf_list = (struct octeon_recv_buffer *)
-	      vzalloc_node(array_size(droq->max_count, OCT_DROQ_RECVBUF_SIZE),
-			   numa_node);
+	droq->recv_buf_list = vzalloc_node(array_size(droq->max_count, OCT_DROQ_RECVBUF_SIZE),
+					   numa_node);
 	if (!droq->recv_buf_list)
-		droq->recv_buf_list = (struct octeon_recv_buffer *)
-		      vzalloc(array_size(droq->max_count,
-					 OCT_DROQ_RECVBUF_SIZE));
+		droq->recv_buf_list = vzalloc(array_size(droq->max_count, OCT_DROQ_RECVBUF_SIZE));
 	if (!droq->recv_buf_list) {
 		dev_err(&oct->pci_dev->dev, "Output queue recv buf list alloc failed\n");
 		goto init_droq_fail;
-- 
2.17.1

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[David Miller]

From: YueHaibing <yuehaibing@huawei.com>
Date: Tue, 1 Sep 2020 22:11:15 +0800

> Remove unneeded return value cast.
> This is detected by coccinelle.
> 
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Applied.

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[Derek Chickles]

> From: Joe Perches <joe@perches.com>
> Subject: [EXT] Re: [PATCH net-next] liquidio: Remove unneeded cast from
> memory allocation
> 
> On Tue, 2020-07-28 at 15:39 +0000, Derek Chickles wrote:
> > I think that is fine as well. We just used vmalloc since there is no
> > need for a physically contiguous piece of memory.
> 
> Do any of the allocs in the driver actually need vmalloc?
> 
This doesn't, but I believe some do, especially when they allocate lists.

[PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[Wang Hai]

Remove casting the values returned by memory allocation function.

Coccinelle emits WARNING:

./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36: WARNING:
 casting value returned by memory allocation function to (struct octeon_dispatch *) is useless.

Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
 drivers/net/ethernet/cavium/liquidio/octeon_device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
index 934115d18..1473a669f 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
 
 		dev_dbg(&oct->pci_dev->dev,
 			"Adding opcode to dispatch list linked list\n");
-		dispatch = (struct octeon_dispatch *)
-			   vmalloc(sizeof(struct octeon_dispatch));
+		dispatch = vmalloc(sizeof(struct octeon_dispatch));
 		if (!dispatch) {
 			dev_err(&oct->pci_dev->dev,
 				"No memory to add dispatch function\n");
-- 
2.17.1

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[Joe Perches]

On Fri, 2020-07-24 at 21:00 +0800, Wang Hai wrote:
> Remove casting the values returned by memory allocation function.
> 
> Coccinelle emits WARNING:
> 
> ./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36: WARNING:
>  casting value returned by memory allocation function to (struct octeon_dispatch *) is useless.
[]
> diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
[]
> @@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
>  
>  		dev_dbg(&oct->pci_dev->dev,
>  			"Adding opcode to dispatch list linked list\n");
> -		dispatch = (struct octeon_dispatch *)
> -			   vmalloc(sizeof(struct octeon_dispatch));
> +		dispatch = vmalloc(sizeof(struct octeon_dispatch));

More the question is why this is vmalloc at all
as the structure size is very small.

Likely this should just be kmalloc.

drivers/net/ethernet/cavium/liquidio/octeon_device.h:struct octeon_dispatch {
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   /** List head for this entry */
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   struct list_head list;
drivers/net/ethernet/cavium/liquidio/octeon_device.h-
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   /** The opcode for which the dispatch function & arg should be used */
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   u16 opcode;
drivers/net/ethernet/cavium/liquidio/octeon_device.h-
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   /** The function to be called for a packet received by the driver */
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   octeon_dispatch_fn_t dispatch_fn;
drivers/net/ethernet/cavium/liquidio/octeon_device.h-
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   /* The application specified argument to be passed to the above
drivers/net/ethernet/cavium/liquidio/octeon_device.h-    * function along with the received packet
drivers/net/ethernet/cavium/liquidio/octeon_device.h-    */
drivers/net/ethernet/cavium/liquidio/octeon_device.h-   void *arg;
drivers/net/ethernet/cavium/liquidio/octeon_device.h-}

>  		if (!dispatch) {
>  			dev_err(&oct->pci_dev->dev,
>  				"No memory to add dispatch function\n");

And this dev_err is unnecessary.

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[wanghai (M)]

在 2020/7/25 5:29, Joe Perches 写道:
> On Fri, 2020-07-24 at 21:00 +0800, Wang Hai wrote:
>> Remove casting the values returned by memory allocation function.
>>
>> Coccinelle emits WARNING:
>>
>> ./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36: WARNING:
>>   casting value returned by memory allocation function to (struct octeon_dispatch *) is useless.
> []
>> diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> []
>> @@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
>>   
>>   		dev_dbg(&oct->pci_dev->dev,
>>   			"Adding opcode to dispatch list linked list\n");
>> -		dispatch = (struct octeon_dispatch *)
>> -			   vmalloc(sizeof(struct octeon_dispatch));
>> +		dispatch = vmalloc(sizeof(struct octeon_dispatch));
> More the question is why this is vmalloc at all
> as the structure size is very small.
>
> Likely this should just be kmalloc.
>
>
Thanks for your advice.  It is indeed best to use kmalloc here.
>>   		if (!dispatch) {
>>   			dev_err(&oct->pci_dev->dev,
>>   				"No memory to add dispatch function\n");
> And this dev_err is unnecessary.
>
>
I don't understand why dev_err is not needed here. We can easily know 
that an error has occurred here through dev_err
> .
>

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[Joe Perches]

On Tue, 2020-07-28 at 16:42 +0800, wanghai (M) wrote:
> 在 2020/7/25 5:29, Joe Perches 写道:
> > On Fri, 2020-07-24 at 21:00 +0800, Wang Hai wrote:
> > > Remove casting the values returned by memory allocation function.
> > > 
> > > Coccinelle emits WARNING:
> > > 
> > > ./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36: WARNING:
> > >   casting value returned by memory allocation function to (struct octeon_dispatch *) is useless.
> > []
> > > diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> > []
> > > @@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
> > >   
> > >   		dev_dbg(&oct->pci_dev->dev,
> > >   			"Adding opcode to dispatch list linked list\n");
> > > -		dispatch = (struct octeon_dispatch *)
> > > -			   vmalloc(sizeof(struct octeon_dispatch));
> > > +		dispatch = vmalloc(sizeof(struct octeon_dispatch));
> > More the question is why this is vmalloc at all
> > as the structure size is very small.
> > 
> > Likely this should just be kmalloc.
> > 
> > 
> Thanks for your advice.  It is indeed best to use kmalloc here.
> > >   		if (!dispatch) {
> > >   			dev_err(&oct->pci_dev->dev,
> > >   				"No memory to add dispatch function\n");
> > And this dev_err is unnecessary.
> > 
> > 
> I don't understand why dev_err is not needed here. We can easily know 
> that an error has occurred here through dev_err

Memory allocation failures without __GFP_NOWARN. already
do a dump_stack to show the location of the code that
could not successfully allocate memory.

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[wanghai (M)]

在 2020/7/28 17:11, Joe Perches 写道:
> On Tue, 2020-07-28 at 16:42 +0800, wanghai (M) wrote:
>> 在 2020/7/25 5:29, Joe Perches 写道:
>>> On Fri, 2020-07-24 at 21:00 +0800, Wang Hai wrote:
>>>> Remove casting the values returned by memory allocation function.
>>>>
>>>> Coccinelle emits WARNING:
>>>>
>>>> ./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36: WARNING:
>>>>    casting value returned by memory allocation function to (struct octeon_dispatch *) is useless.
>>> []
>>>> diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
>>> []
>>>> @@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
>>>>    
>>>>    		dev_dbg(&oct->pci_dev->dev,
>>>>    			"Adding opcode to dispatch list linked list\n");
>>>> -		dispatch = (struct octeon_dispatch *)
>>>> -			   vmalloc(sizeof(struct octeon_dispatch));
>>>> +		dispatch = vmalloc(sizeof(struct octeon_dispatch));
>>> More the question is why this is vmalloc at all
>>> as the structure size is very small.
>>>
>>> Likely this should just be kmalloc.
>>>
>>>
>> Thanks for your advice.  It is indeed best to use kmalloc here.
>>>>    		if (!dispatch) {
>>>>    			dev_err(&oct->pci_dev->dev,
>>>>    				"No memory to add dispatch function\n");
>>> And this dev_err is unnecessary.
>>>
>>>
>> I don't understand why dev_err is not needed here. We can easily know
>> that an error has occurred here through dev_err
> Memory allocation failures without __GFP_NOWARN. already
> do a dump_stack to show the location of the code that
> could not successfully allocate memory.
>
>
Thanks for your explanation. I got it.

Can it be modified like this?

--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -1152,11 +1152,8 @@ octeon_register_dispatch_fn(struct octeon_device 
*oct,

                 dev_dbg(&oct->pci_dev->dev,
                         "Adding opcode to dispatch list linked list\n");
-               dispatch = (struct octeon_dispatch *)
-                          vmalloc(sizeof(struct octeon_dispatch));
+               dispatch = kmalloc(sizeof(struct octeon_dispatch), 
GFP_KERNEL);
                 if (!dispatch) {
-                       dev_err(&oct->pci_dev->dev,
-                               "No memory to add dispatch function\n");
                         return 1;
                 }
                 dispatch->opcode = combined_opcode;

> .
>

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[Joe Perches]

On Tue, 2020-07-28 at 21:38 +0800, wanghai (M) wrote:
> Thanks for your explanation. I got it.
> 
> Can it be modified like this?
[]
> +++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> @@ -1152,11 +1152,8 @@ octeon_register_dispatch_fn(struct octeon_device 
> *oct,
> 
>                  dev_dbg(&oct->pci_dev->dev,
>                          "Adding opcode to dispatch list linked list\n");
> -               dispatch = (struct octeon_dispatch *)
> -                          vmalloc(sizeof(struct octeon_dispatch));
> +               dispatch = kmalloc(sizeof(struct octeon_dispatch), 
> GFP_KERNEL);
>                  if (!dispatch) {
> -                       dev_err(&oct->pci_dev->dev,
> -                               "No memory to add dispatch function\n");
>                          return 1;
>                  }
>                  dispatch->opcode = combined_opcode;

Yes, but the free also needs to be changed.

I think it's:
---
 drivers/net/ethernet/cavium/liquidio/octeon_device.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
index 934115d18488..4ee4cb946e1d 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -1056,7 +1056,7 @@ void octeon_delete_dispatch_list(struct octeon_device *oct)
 
 	list_for_each_safe(temp, tmp2, &freelist) {
 		list_del(temp);
-		vfree(temp);
+		kfree(temp);
 	}
 }
 
@@ -1152,13 +1152,10 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
 
 		dev_dbg(&oct->pci_dev->dev,
 			"Adding opcode to dispatch list linked list\n");
-		dispatch = (struct octeon_dispatch *)
-			   vmalloc(sizeof(struct octeon_dispatch));
-		if (!dispatch) {
-			dev_err(&oct->pci_dev->dev,
-				"No memory to add dispatch function\n");
+		dispatch = kmalloc(sizeof(struct octeon_dispatch), GFP_KERNEL);
+		if (!dispatch)
 			return 1;
-		}
+
 		dispatch->opcode = combined_opcode;
 		dispatch->dispatch_fn = fn;
 		dispatch->arg = fn_arg;

Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation

[wanghai (M)]

在 2020/7/28 23:54, Joe Perches 写道:
> On Tue, 2020-07-28 at 21:38 +0800, wanghai (M) wrote:
>> Thanks for your explanation. I got it.
>>
>> Can it be modified like this?
> []
>> +++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
>> @@ -1152,11 +1152,8 @@ octeon_register_dispatch_fn(struct octeon_device
>> *oct,
>>
>>                   dev_dbg(&oct->pci_dev->dev,
>>                           "Adding opcode to dispatch list linked list\n");
>> -               dispatch = (struct octeon_dispatch *)
>> -                          vmalloc(sizeof(struct octeon_dispatch));
>> +               dispatch = kmalloc(sizeof(struct octeon_dispatch),
>> GFP_KERNEL);
>>                   if (!dispatch) {
>> -                       dev_err(&oct->pci_dev->dev,
>> -                               "No memory to add dispatch function\n");
>>                           return 1;
>>                   }
>>                   dispatch->opcode = combined_opcode;
> Yes, but the free also needs to be changed.
>
> I think it's:
> ---
>   drivers/net/ethernet/cavium/liquidio/octeon_device.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> index 934115d18488..4ee4cb946e1d 100644
> --- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> +++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
> @@ -1056,7 +1056,7 @@ void octeon_delete_dispatch_list(struct octeon_device *oct)
>   
>   	list_for_each_safe(temp, tmp2, &freelist) {
>   		list_del(temp);
> -		vfree(temp);
> +		kfree(temp);
>   	}
>   }
>   
> @@ -1152,13 +1152,10 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
>   
>   		dev_dbg(&oct->pci_dev->dev,
>   			"Adding opcode to dispatch list linked list\n");
> -		dispatch = (struct octeon_dispatch *)
> -			   vmalloc(sizeof(struct octeon_dispatch));
> -		if (!dispatch) {
> -			dev_err(&oct->pci_dev->dev,
> -				"No memory to add dispatch function\n");
> +		dispatch = kmalloc(sizeof(struct octeon_dispatch), GFP_KERNEL);
> +		if (!dispatch)
>   			return 1;
> -		}
> +
>   		dispatch->opcode = combined_opcode;
>   		dispatch->dispatch_fn = fn;
>   		dispatch->arg = fn_arg;
>
>
>
> .
Thanks for your suggestion. I just sent another patch for this.

"[PATCH net-next] liquidio: Replace vmalloc with kmalloc in 
octeon_register_dispatch_fn()"

>