| Message ID | 20200724130001.71528-1-wanghai38@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] liquidio: Remove unneeded cast from memory allocation | expand |
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.
在 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 > . >
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.
在 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; > . >
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;
在 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()" >
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");
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(-)