* [PATCH 0/4 v2] firmware: stratix10-svc: Fix some error handling code @ 2020-04-29 6:52 Christophe JAILLET 2020-04-29 6:52 ` [PATCH 1/4 v2] firmware: stratix10-svc: Fix genpool creation error handling Christophe JAILLET ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Christophe JAILLET @ 2020-04-29 6:52 UTC (permalink / raw) To: richard.gong, gregkh, atull Cc: linux-kernel, kernel-janitors, Christophe JAILLET This serie was previously sent as a single patch. After a comment from Dan Carpenter about an error handling path that could be improved, I've looked deeper at the code and found other issues. The previous patch corresponds to patch 3/4 in this serie. This v2 takes Dan's comment into account and fix another resource leak. Patch 1/4: svc_create_memory_pool returns an error code on error, not NULL Patch 2/4: undo a memremap on error path and remove funtion Patch 3/4: improve error handling in the probe function Patch 4/4: unrelated clean-up Christophe JAILLET (4): firmware: stratix10-svc: Fix genpool creation error handling firmware: stratix10-svc: Unmap some previously memremap'ed memory firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' firmware: stratix10-svc: Slighly simplify call drivers/firmware/stratix10-svc.c | 42 +++++++++++++++++++------------- 1 file changed, 25 insertions(+), 17 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/4 v2] firmware: stratix10-svc: Fix genpool creation error handling 2020-04-29 6:52 [PATCH 0/4 v2] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET @ 2020-04-29 6:52 ` Christophe JAILLET 2020-04-29 6:52 ` [PATCH 2/4 v2] firmware: stratix10-svc: Unmap some previously memremap'ed memory Christophe JAILLET ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Christophe JAILLET @ 2020-04-29 6:52 UTC (permalink / raw) To: richard.gong, gregkh, atull Cc: linux-kernel, kernel-janitors, Christophe JAILLET 'svc_create_memory_pool()' returns an error pointer on error, not NULL. Fix the corresponding test and return value accordingly. Move the genpool allocation after a few devm_kzalloc in order to ease error handling. Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/firmware/stratix10-svc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c index d5f0769f3761..3a176e62754a 100644 --- a/drivers/firmware/stratix10-svc.c +++ b/drivers/firmware/stratix10-svc.c @@ -997,10 +997,6 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) if (ret) return ret; - genpool = svc_create_memory_pool(pdev, sh_memory); - if (!genpool) - return -ENOMEM; - /* allocate service controller and supporting channel */ controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); if (!controller) @@ -1011,6 +1007,10 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) if (!chans) return -ENOMEM; + genpool = svc_create_memory_pool(pdev, sh_memory); + if (IS_ERR(genpool)) + return PTR_ERR(genpool); + controller->dev = dev; controller->num_chans = SVC_NUM_CHANNEL; controller->num_active_client = 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/4 v2] firmware: stratix10-svc: Unmap some previously memremap'ed memory 2020-04-29 6:52 [PATCH 0/4 v2] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET 2020-04-29 6:52 ` [PATCH 1/4 v2] firmware: stratix10-svc: Fix genpool creation error handling Christophe JAILLET @ 2020-04-29 6:52 ` Christophe JAILLET 2020-05-05 14:43 ` Greg KH 2020-04-29 6:52 ` [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' Christophe JAILLET 2020-04-29 6:52 ` [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code Christophe JAILLET 3 siblings, 1 reply; 21+ messages in thread From: Christophe JAILLET @ 2020-04-29 6:52 UTC (permalink / raw) To: richard.gong, gregkh, atull Cc: linux-kernel, kernel-janitors, Christophe JAILLET In 'svc_create_memory_pool()' we memremap some memory. This has to be undone in case of error and if the driver is removed. The easiest way to do it is to use 'devm_memremap()'. Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/firmware/stratix10-svc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c index 3a176e62754a..de5870f76c5e 100644 --- a/drivers/firmware/stratix10-svc.c +++ b/drivers/firmware/stratix10-svc.c @@ -631,7 +631,7 @@ svc_create_memory_pool(struct platform_device *pdev, end = rounddown(sh_memory->addr + sh_memory->size, PAGE_SIZE); paddr = begin; size = end - begin; - va = memremap(paddr, size, MEMREMAP_WC); + va = devm_memremap(dev, paddr, size, MEMREMAP_WC); if (!va) { dev_err(dev, "fail to remap shared memory\n"); return ERR_PTR(-EINVAL); -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4 v2] firmware: stratix10-svc: Unmap some previously memremap'ed memory 2020-04-29 6:52 ` [PATCH 2/4 v2] firmware: stratix10-svc: Unmap some previously memremap'ed memory Christophe JAILLET @ 2020-05-05 14:43 ` Greg KH 2020-05-05 15:09 ` Christophe JAILLET 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2020-05-05 14:43 UTC (permalink / raw) To: Christophe JAILLET; +Cc: richard.gong, atull, linux-kernel, kernel-janitors On Wed, Apr 29, 2020 at 08:52:43AM +0200, Christophe JAILLET wrote: > In 'svc_create_memory_pool()' we memremap some memory. This has to be > undone in case of error and if the driver is removed. > > The easiest way to do it is to use 'devm_memremap()'. > > Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/firmware/stratix10-svc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c > index 3a176e62754a..de5870f76c5e 100644 > --- a/drivers/firmware/stratix10-svc.c > +++ b/drivers/firmware/stratix10-svc.c > @@ -631,7 +631,7 @@ svc_create_memory_pool(struct platform_device *pdev, > end = rounddown(sh_memory->addr + sh_memory->size, PAGE_SIZE); > paddr = begin; > size = end - begin; > - va = memremap(paddr, size, MEMREMAP_WC); > + va = devm_memremap(dev, paddr, size, MEMREMAP_WC); > if (!va) { > dev_err(dev, "fail to remap shared memory\n"); > return ERR_PTR(-EINVAL); And there was no previous unmap happening when the pool was torn down that now needs to be removed? thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4 v2] firmware: stratix10-svc: Unmap some previously memremap'ed memory 2020-05-05 14:43 ` Greg KH @ 2020-05-05 15:09 ` Christophe JAILLET 0 siblings, 0 replies; 21+ messages in thread From: Christophe JAILLET @ 2020-05-05 15:09 UTC (permalink / raw) To: Greg KH; +Cc: richard.gong, atull, linux-kernel, kernel-janitors Le 05/05/2020 à 16:43, Greg KH a écrit : > On Wed, Apr 29, 2020 at 08:52:43AM +0200, Christophe JAILLET wrote: >> In 'svc_create_memory_pool()' we memremap some memory. This has to be >> undone in case of error and if the driver is removed. >> >> The easiest way to do it is to use 'devm_memremap()'. >> >> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> drivers/firmware/stratix10-svc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c >> index 3a176e62754a..de5870f76c5e 100644 >> --- a/drivers/firmware/stratix10-svc.c >> +++ b/drivers/firmware/stratix10-svc.c >> @@ -631,7 +631,7 @@ svc_create_memory_pool(struct platform_device *pdev, >> end = rounddown(sh_memory->addr + sh_memory->size, PAGE_SIZE); >> paddr = begin; >> size = end - begin; >> - va = memremap(paddr, size, MEMREMAP_WC); >> + va = devm_memremap(dev, paddr, size, MEMREMAP_WC); >> if (!va) { >> dev_err(dev, "fail to remap shared memory\n"); >> return ERR_PTR(-EINVAL); > And there was no previous unmap happening when the pool was torn down > that now needs to be removed? I don't think so, there is no memunmap call in 'stratix10-svc.c' CJ > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' 2020-04-29 6:52 [PATCH 0/4 v2] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET 2020-04-29 6:52 ` [PATCH 1/4 v2] firmware: stratix10-svc: Fix genpool creation error handling Christophe JAILLET 2020-04-29 6:52 ` [PATCH 2/4 v2] firmware: stratix10-svc: Unmap some previously memremap'ed memory Christophe JAILLET @ 2020-04-29 6:52 ` Christophe JAILLET 2020-05-05 14:02 ` Richard Gong 2020-04-29 6:52 ` [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code Christophe JAILLET 3 siblings, 1 reply; 21+ messages in thread From: Christophe JAILLET @ 2020-04-29 6:52 UTC (permalink / raw) To: richard.gong, gregkh, atull Cc: linux-kernel, kernel-janitors, Christophe JAILLET If an error occurs after calling 'svc_create_memory_pool()', the allocated genpool should be destroyed with 'gen_pool_destroy()', as already done in the remove function. If an error occurs after calling 'kfifo_alloc()', the allocated memory should be freed with 'kfifo_free()', as already done in the remove function. While at it, also move a 'platform_device_put()' call to the error handling path. Fixes: b5dc75c915cd ("firmware: stratix10-svc: extend svc to support new RSU features") Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/firmware/stratix10-svc.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c index de5870f76c5e..739004398877 100644 --- a/drivers/firmware/stratix10-svc.c +++ b/drivers/firmware/stratix10-svc.c @@ -1024,7 +1024,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL); if (ret) { dev_err(dev, "failed to allocate FIFO\n"); - return ret; + goto err_destroy_pool; } spin_lock_init(&controller->svc_fifo_lock); @@ -1043,24 +1043,34 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) /* add svc client device(s) */ svc = devm_kzalloc(dev, sizeof(*svc), GFP_KERNEL); - if (!svc) - return -ENOMEM; + if (!svc) { + ret = -ENOMEM; + goto err_free_kfifo; + } svc->stratix10_svc_rsu = platform_device_alloc(STRATIX10_RSU, 0); if (!svc->stratix10_svc_rsu) { dev_err(dev, "failed to allocate %s device\n", STRATIX10_RSU); - return -ENOMEM; + ret = -ENOMEM; + goto err_free_kfifo; } ret = platform_device_add(svc->stratix10_svc_rsu); - if (ret) { - platform_device_put(svc->stratix10_svc_rsu); - return ret; - } + if (ret) + goto put_platform; + dev_set_drvdata(dev, svc); pr_info("Intel Service Layer Driver Initialized\n"); + return 0; + +put_platform: + platform_device_put(svc->stratix10_svc_rsu); +err_free_kfifo: + kfifo_free(&controller->svc_fifo); +err_destroy_pool: + gen_pool_destroy(genpool); return ret; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' 2020-04-29 6:52 ` [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' Christophe JAILLET @ 2020-05-05 14:02 ` Richard Gong 2020-05-05 15:15 ` Christophe JAILLET 0 siblings, 1 reply; 21+ messages in thread From: Richard Gong @ 2020-05-05 14:02 UTC (permalink / raw) To: Christophe JAILLET, gregkh, atull; +Cc: linux-kernel, kernel-janitors Hi, Similarly we need add error handling for controller and chans, something like below: @@ -997,13 +997,17 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) /* allocate service controller and supporting channel */ controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); - if (!controller) - return -ENOMEM; + if (!controller) { + ret = -ENOMEM; + goto err_destroy_pool; + } chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, sizeof(*chans), GFP_KERNEL | __GFP_ZERO); - if (!chans) - return -ENOMEM; + if (!chans) { + ret = -ENOMEM; + goto err_destroy_pool; + } controller->dev = dev; controller->num_chans = SVC_NUM_CHANNEL; On 4/29/20 1:52 AM, Christophe JAILLET wrote: > If an error occurs after calling 'svc_create_memory_pool()', the allocated > genpool should be destroyed with 'gen_pool_destroy()', as already done in > the remove function. > > If an error occurs after calling 'kfifo_alloc()', the allocated memory > should be freed with 'kfifo_free()', as already done in the remove > function. > > While at it, also move a 'platform_device_put()' call to the error handling > path. > > Fixes: b5dc75c915cd ("firmware: stratix10-svc: extend svc to support new RSU features") > Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/firmware/stratix10-svc.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > I am fine with below changes. > diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c > index de5870f76c5e..739004398877 100644 > --- a/drivers/firmware/stratix10-svc.c > +++ b/drivers/firmware/stratix10-svc.c > @@ -1024,7 +1024,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) > ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL); > if (ret) { > dev_err(dev, "failed to allocate FIFO\n"); > - return ret; > + goto err_destroy_pool; > } > spin_lock_init(&controller->svc_fifo_lock); > > @@ -1043,24 +1043,34 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) > > /* add svc client device(s) */ > svc = devm_kzalloc(dev, sizeof(*svc), GFP_KERNEL); > - if (!svc) > - return -ENOMEM; > + if (!svc) { > + ret = -ENOMEM; > + goto err_free_kfifo; > + } > > svc->stratix10_svc_rsu = platform_device_alloc(STRATIX10_RSU, 0); > if (!svc->stratix10_svc_rsu) { > dev_err(dev, "failed to allocate %s device\n", STRATIX10_RSU); > - return -ENOMEM; > + ret = -ENOMEM; > + goto err_free_kfifo; > } > > ret = platform_device_add(svc->stratix10_svc_rsu); > - if (ret) { > - platform_device_put(svc->stratix10_svc_rsu); > - return ret; > - } > + if (ret) > + goto put_platform; > + > dev_set_drvdata(dev, svc); > > pr_info("Intel Service Layer Driver Initialized\n"); > > + return 0; > + > +put_platform: > + platform_device_put(svc->stratix10_svc_rsu); > +err_free_kfifo: > + kfifo_free(&controller->svc_fifo); > +err_destroy_pool: > + gen_pool_destroy(genpool); > return ret; > } > > Regards, Richard ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' 2020-05-05 14:02 ` Richard Gong @ 2020-05-05 15:15 ` Christophe JAILLET 2020-06-26 19:37 ` [PATCH V3] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET 0 siblings, 1 reply; 21+ messages in thread From: Christophe JAILLET @ 2020-05-05 15:15 UTC (permalink / raw) To: Richard Gong, gregkh, atull; +Cc: linux-kernel, kernel-janitors Le 05/05/2020 à 16:02, Richard Gong a écrit : > Hi, > > Similarly we need add error handling for controller and chans, > something like below: > > @@ -997,13 +997,17 @@ static int stratix10_svc_drv_probe(struct > platform_device *pdev) > > /* allocate service controller and supporting channel */ > controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); > - if (!controller) > - return -ENOMEM; > + if (!controller) { > + ret = -ENOMEM; > + goto err_destroy_pool; > + } > > chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, > sizeof(*chans), GFP_KERNEL | > __GFP_ZERO); > - if (!chans) > - return -ENOMEM; > + if (!chans) { > + ret = -ENOMEM; > + goto err_destroy_pool; > + } > > controller->dev = dev; > controller->num_chans = SVC_NUM_CHANNEL; > Hi, This is addressed in patch 1/4. It moves 'svc_create_memory_pool' after these 2 allocations in order to avoid the goto. I'll send a V3 in only 1 patch, as you proposed, it will ease review. CJ > On 4/29/20 1:52 AM, Christophe JAILLET wrote: >> If an error occurs after calling 'svc_create_memory_pool()', the >> allocated >> genpool should be destroyed with 'gen_pool_destroy()', as already >> done in >> the remove function. >> >> If an error occurs after calling 'kfifo_alloc()', the allocated memory >> should be freed with 'kfifo_free()', as already done in the remove >> function. >> >> While at it, also move a 'platform_device_put()' call to the error >> handling >> path. >> >> Fixes: b5dc75c915cd ("firmware: stratix10-svc: extend svc to support >> new RSU features") >> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer >> driver") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> drivers/firmware/stratix10-svc.c | 26 ++++++++++++++++++-------- >> 1 file changed, 18 insertions(+), 8 deletions(-) >> > I am fine with below changes. > >> diff --git a/drivers/firmware/stratix10-svc.c >> b/drivers/firmware/stratix10-svc.c >> index de5870f76c5e..739004398877 100644 >> --- a/drivers/firmware/stratix10-svc.c >> +++ b/drivers/firmware/stratix10-svc.c >> @@ -1024,7 +1024,7 @@ static int stratix10_svc_drv_probe(struct >> platform_device *pdev) >> ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL); >> if (ret) { >> dev_err(dev, "failed to allocate FIFO\n"); >> - return ret; >> + goto err_destroy_pool; >> } >> spin_lock_init(&controller->svc_fifo_lock); >> @@ -1043,24 +1043,34 @@ static int stratix10_svc_drv_probe(struct >> platform_device *pdev) >> /* add svc client device(s) */ >> svc = devm_kzalloc(dev, sizeof(*svc), GFP_KERNEL); >> - if (!svc) >> - return -ENOMEM; >> + if (!svc) { >> + ret = -ENOMEM; >> + goto err_free_kfifo; >> + } >> svc->stratix10_svc_rsu = platform_device_alloc(STRATIX10_RSU, >> 0); >> if (!svc->stratix10_svc_rsu) { >> dev_err(dev, "failed to allocate %s device\n", STRATIX10_RSU); >> - return -ENOMEM; >> + ret = -ENOMEM; >> + goto err_free_kfifo; >> } >> ret = platform_device_add(svc->stratix10_svc_rsu); >> - if (ret) { >> - platform_device_put(svc->stratix10_svc_rsu); >> - return ret; >> - } >> + if (ret) >> + goto put_platform; >> + >> dev_set_drvdata(dev, svc); >> pr_info("Intel Service Layer Driver Initialized\n"); >> + return 0; >> + >> +put_platform: >> + platform_device_put(svc->stratix10_svc_rsu); >> +err_free_kfifo: >> + kfifo_free(&controller->svc_fifo); >> +err_destroy_pool: >> + gen_pool_destroy(genpool); >> return ret; >> } >> > Regards, > Richard > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3] firmware: stratix10-svc: Fix some error handling code 2020-05-05 15:15 ` Christophe JAILLET @ 2020-06-26 19:37 ` Christophe JAILLET 2020-06-27 5:15 ` Greg KH 2020-06-27 5:15 ` Greg KH 0 siblings, 2 replies; 21+ messages in thread From: Christophe JAILLET @ 2020-06-26 19:37 UTC (permalink / raw) To: richard.gong, atull, gregkh Cc: linux-kernel, kernel-janitors, Christophe JAILLET Fix several error handling issues: - In 'svc_create_memory_pool()' we memremap some memory. This has to be undone in case of error and if the driver is removed. The easiest way to do it is to use 'devm_memremap()'. - 'svc_create_memory_pool()' returns an error pointer on error, not NULL. In 'stratix10_svc_drv_probe()', fix the corresponding test and return value accordingly. - Move the genpool allocation after a few devm_kzalloc in order to ease error handling. (some call to 'gen_pool_destroy()' were missing) - If an error occurs after calling 'svc_create_memory_pool()', the allocated genpool should be destroyed with 'gen_pool_destroy()', as already done in the remove function. Add an error handling path for that - If an error occurs after calling 'kfifo_alloc()', the allocated memory should be freed with 'kfifo_free()', as already done in the remove function. Add an error handling path for that While at it, do some clean-up: - Use 'devm_kcalloc()' instead of 'devm_kmalloc_array(... __GFP_ZERO)' - move a 'platform_device_put()' call to the new error handling path. - In 'stratix10_svc_drv_remove()', 'ctrl->genpool' can not be NULL, so axe a useless test in the remove function. Fixes: b5dc75c915cd ("firmware: stratix10-svc: extend svc to support new RSU features") Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- v2: takes Dan's comment into account and fix another resource leak. v3: merge the previous 4 patches in a single one to ease review --- drivers/firmware/stratix10-svc.c | 42 +++++++++++++++++++------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c index e0db8dbfc9d1..90f7d68a2473 100644 --- a/drivers/firmware/stratix10-svc.c +++ b/drivers/firmware/stratix10-svc.c @@ -605,7 +605,7 @@ svc_create_memory_pool(struct platform_device *pdev, end = rounddown(sh_memory->addr + sh_memory->size, PAGE_SIZE); paddr = begin; size = end - begin; - va = memremap(paddr, size, MEMREMAP_WC); + va = devm_memremap(dev, paddr, size, MEMREMAP_WC); if (!va) { dev_err(dev, "fail to remap shared memory\n"); return ERR_PTR(-EINVAL); @@ -971,20 +971,19 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) if (ret) return ret; - genpool = svc_create_memory_pool(pdev, sh_memory); - if (!genpool) - return -ENOMEM; - /* allocate service controller and supporting channel */ controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); if (!controller) return -ENOMEM; - chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, - sizeof(*chans), GFP_KERNEL | __GFP_ZERO); + chans = devm_kcalloc(dev, SVC_NUM_CHANNEL, sizeof(*chans), GFP_KERNEL); if (!chans) return -ENOMEM; + genpool = svc_create_memory_pool(pdev, sh_memory); + if (IS_ERR(genpool)) + return PTR_ERR(genpool); + controller->dev = dev; controller->num_chans = SVC_NUM_CHANNEL; controller->num_active_client = 0; @@ -998,7 +997,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL); if (ret) { dev_err(dev, "failed to allocate FIFO\n"); - return ret; + goto err_destroy_pool; } spin_lock_init(&controller->svc_fifo_lock); @@ -1017,24 +1016,34 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) /* add svc client device(s) */ svc = devm_kzalloc(dev, sizeof(*svc), GFP_KERNEL); - if (!svc) - return -ENOMEM; + if (!svc) { + ret = -ENOMEM; + goto err_free_kfifo; + } svc->stratix10_svc_rsu = platform_device_alloc(STRATIX10_RSU, 0); if (!svc->stratix10_svc_rsu) { dev_err(dev, "failed to allocate %s device\n", STRATIX10_RSU); - return -ENOMEM; + ret = -ENOMEM; + goto err_free_kfifo; } ret = platform_device_add(svc->stratix10_svc_rsu); - if (ret) { - platform_device_put(svc->stratix10_svc_rsu); - return ret; - } + if (ret) + goto put_platform; + dev_set_drvdata(dev, svc); pr_info("Intel Service Layer Driver Initialized\n"); + return 0; + +put_platform: + platform_device_put(svc->stratix10_svc_rsu); +err_free_kfifo: + kfifo_free(&controller->svc_fifo); +err_destroy_pool: + gen_pool_destroy(genpool); return ret; } @@ -1050,8 +1059,7 @@ static int stratix10_svc_drv_remove(struct platform_device *pdev) kthread_stop(ctrl->task); ctrl->task = NULL; } - if (ctrl->genpool) - gen_pool_destroy(ctrl->genpool); + gen_pool_destroy(ctrl->genpool); list_del(&ctrl->node); return 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH V3] firmware: stratix10-svc: Fix some error handling code 2020-06-26 19:37 ` [PATCH V3] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET @ 2020-06-27 5:15 ` Greg KH 2020-06-27 5:15 ` Greg KH 1 sibling, 0 replies; 21+ messages in thread From: Greg KH @ 2020-06-27 5:15 UTC (permalink / raw) To: Christophe JAILLET; +Cc: richard.gong, atull, linux-kernel, kernel-janitors On Fri, Jun 26, 2020 at 09:37:20PM +0200, Christophe JAILLET wrote: > Fix several error handling issues: <snip> When you have to list a bunch of different things you do in a driver, that's a huge hint this needs to be broken up into multiple patches. Please do that here. thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3] firmware: stratix10-svc: Fix some error handling code 2020-06-26 19:37 ` [PATCH V3] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET 2020-06-27 5:15 ` Greg KH @ 2020-06-27 5:15 ` Greg KH 2020-06-27 7:31 ` Marion & Christophe JAILLET 1 sibling, 1 reply; 21+ messages in thread From: Greg KH @ 2020-06-27 5:15 UTC (permalink / raw) To: Christophe JAILLET; +Cc: richard.gong, atull, linux-kernel, kernel-janitors On Fri, Jun 26, 2020 at 09:37:20PM +0200, Christophe JAILLET wrote: > --- > v2: takes Dan's comment into account and fix another resource leak. > v3: merge the previous 4 patches in a single one to ease review No, 4 small patches are _MUCH_ easier to review than one larger one that mixes everything together. Who told you to put them together? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3] firmware: stratix10-svc: Fix some error handling code 2020-06-27 5:15 ` Greg KH @ 2020-06-27 7:31 ` Marion & Christophe JAILLET 2020-06-27 8:03 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Marion & Christophe JAILLET @ 2020-06-27 7:31 UTC (permalink / raw) To: Greg KH; +Cc: richard.gong, atull, linux-kernel, kernel-janitors Le 27/06/2020 à 07:15, Greg KH a écrit : > On Fri, Jun 26, 2020 at 09:37:20PM +0200, Christophe JAILLET wrote: >> --- >> v2: takes Dan's comment into account and fix another resource leak. >> v3: merge the previous 4 patches in a single one to ease review > > No, 4 small patches are _MUCH_ easier to review than one larger one that > mixes everything together. Who told you to put them together? The cover letter of v2 serie can be found at [1]. The request for merging them in 1 patch is in [2]. V3, should be the same as v2, but all in one. [1]: https://lkml.org/lkml/2020/4/29/77 [2]: https://lkml.org/lkml/2020/5/5/541 CJ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3] firmware: stratix10-svc: Fix some error handling code 2020-06-27 7:31 ` Marion & Christophe JAILLET @ 2020-06-27 8:03 ` Greg KH 0 siblings, 0 replies; 21+ messages in thread From: Greg KH @ 2020-06-27 8:03 UTC (permalink / raw) To: Marion & Christophe JAILLET Cc: richard.gong, atull, linux-kernel, kernel-janitors On Sat, Jun 27, 2020 at 09:31:27AM +0200, Marion & Christophe JAILLET wrote: > > Le 27/06/2020 à 07:15, Greg KH a écrit : > > On Fri, Jun 26, 2020 at 09:37:20PM +0200, Christophe JAILLET wrote: > > > --- > > > v2: takes Dan's comment into account and fix another resource leak. > > > v3: merge the previous 4 patches in a single one to ease review > > > > No, 4 small patches are _MUCH_ easier to review than one larger one that > > mixes everything together. Who told you to put them together? > > The cover letter of v2 serie can be found at [1]. > The request for merging them in 1 patch is in [2]. > > V3, should be the same as v2, but all in one. > > [1]: https://lkml.org/lkml/2020/4/29/77 > [2]: https://lkml.org/lkml/2020/5/5/541 Please use lore.kernel.org in the future, we don't control lkml.org and can't rely on it. Anyway, that request was incorrect, sorry. Please keep them split up in a way that makes it easy to review. Which would you want to read if you had to review hundreds of patches? thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code 2020-04-29 6:52 [PATCH 0/4 v2] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET ` (2 preceding siblings ...) 2020-04-29 6:52 ` [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' Christophe JAILLET @ 2020-04-29 6:52 ` Christophe JAILLET 2020-05-01 15:40 ` Richard Gong 3 siblings, 1 reply; 21+ messages in thread From: Christophe JAILLET @ 2020-04-29 6:52 UTC (permalink / raw) To: richard.gong, gregkh, atull Cc: linux-kernel, kernel-janitors, Christophe JAILLET Replace 'devm_kmalloc_array(... | __GFP_ZERO)' with the equivalent and shorter 'devm_kcalloc(...)'. 'ctrl->genpool' can not be NULL, so axe a useless test in the remove function. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/firmware/stratix10-svc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c index 739004398877..c228337cb0a1 100644 --- a/drivers/firmware/stratix10-svc.c +++ b/drivers/firmware/stratix10-svc.c @@ -1002,8 +1002,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) if (!controller) return -ENOMEM; - chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, - sizeof(*chans), GFP_KERNEL | __GFP_ZERO); + chans = devm_kcalloc(dev, SVC_NUM_CHANNEL, sizeof(*chans), GFP_KERNEL); if (!chans) return -ENOMEM; @@ -1086,8 +1085,7 @@ static int stratix10_svc_drv_remove(struct platform_device *pdev) kthread_stop(ctrl->task); ctrl->task = NULL; } - if (ctrl->genpool) - gen_pool_destroy(ctrl->genpool); + gen_pool_destroy(ctrl->genpool); list_del(&ctrl->node); return 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code 2020-04-29 6:52 ` [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code Christophe JAILLET @ 2020-05-01 15:40 ` Richard Gong 2020-05-01 15:48 ` Christophe JAILLET 2020-05-06 10:04 ` Dan Carpenter 0 siblings, 2 replies; 21+ messages in thread From: Richard Gong @ 2020-05-01 15:40 UTC (permalink / raw) To: Christophe JAILLET, gregkh, atull; +Cc: linux-kernel, kernel-janitors Hi, On 4/29/20 1:52 AM, Christophe JAILLET wrote: > Replace 'devm_kmalloc_array(... | __GFP_ZERO)' with the equivalent and > shorter 'devm_kcalloc(...)'. > It doesn't make much sense. Actually devm_kcalloc returns devm_kmalloc_array(.., flag | __GFP_ZERO). > 'ctrl->genpool' can not be NULL, so axe a useless test in the remove > function. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/firmware/stratix10-svc.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c > index 739004398877..c228337cb0a1 100644 > --- a/drivers/firmware/stratix10-svc.c > +++ b/drivers/firmware/stratix10-svc.c > @@ -1002,8 +1002,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) > if (!controller) > return -ENOMEM; > > - chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, > - sizeof(*chans), GFP_KERNEL | __GFP_ZERO); > + chans = devm_kcalloc(dev, SVC_NUM_CHANNEL, sizeof(*chans), GFP_KERNEL); > if (!chans) > return -ENOMEM; > > @@ -1086,8 +1085,7 @@ static int stratix10_svc_drv_remove(struct platform_device *pdev) > kthread_stop(ctrl->task); > ctrl->task = NULL; > } > - if (ctrl->genpool) > - gen_pool_destroy(ctrl->genpool); > + gen_pool_destroy(ctrl->genpool); > list_del(&ctrl->node); > > return 0; > Regards, Richard ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code 2020-05-01 15:40 ` Richard Gong @ 2020-05-01 15:48 ` Christophe JAILLET 2020-05-01 16:55 ` Richard Gong 2020-05-06 10:04 ` Dan Carpenter 1 sibling, 1 reply; 21+ messages in thread From: Christophe JAILLET @ 2020-05-01 15:48 UTC (permalink / raw) To: Richard Gong, gregkh, atull; +Cc: linux-kernel, kernel-janitors Le 01/05/2020 à 17:40, Richard Gong a écrit : > Hi, > > On 4/29/20 1:52 AM, Christophe JAILLET wrote: >> Replace 'devm_kmalloc_array(... | __GFP_ZERO)' with the equivalent and >> shorter 'devm_kcalloc(...)'. >> > It doesn't make much sense. > Actually devm_kcalloc returns devm_kmalloc_array(.., flag | __GFP_ZERO). > The only goal is to have a sightly less verbose code. This saves one line of code and there is no need to wonder why we explicitly pass __GFP_ZERO to kmalloc_array. Mostly a matter of taste. 'devm_kcalloc' is inlined, so the binary should be exactly the same. CJ >> 'ctrl->genpool' can not be NULL, so axe a useless test in the remove >> function. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> drivers/firmware/stratix10-svc.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/firmware/stratix10-svc.c >> b/drivers/firmware/stratix10-svc.c >> index 739004398877..c228337cb0a1 100644 >> --- a/drivers/firmware/stratix10-svc.c >> +++ b/drivers/firmware/stratix10-svc.c >> @@ -1002,8 +1002,7 @@ static int stratix10_svc_drv_probe(struct >> platform_device *pdev) >> if (!controller) >> return -ENOMEM; >> - chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, >> - sizeof(*chans), GFP_KERNEL | __GFP_ZERO); >> + chans = devm_kcalloc(dev, SVC_NUM_CHANNEL, sizeof(*chans), >> GFP_KERNEL); >> if (!chans) >> return -ENOMEM; >> @@ -1086,8 +1085,7 @@ static int stratix10_svc_drv_remove(struct >> platform_device *pdev) >> kthread_stop(ctrl->task); >> ctrl->task = NULL; >> } >> - if (ctrl->genpool) >> - gen_pool_destroy(ctrl->genpool); >> + gen_pool_destroy(ctrl->genpool); >> list_del(&ctrl->node); >> return 0; >> > > Regards, > Richard > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code 2020-05-01 15:48 ` Christophe JAILLET @ 2020-05-01 16:55 ` Richard Gong 2020-05-02 8:49 ` Christophe JAILLET 0 siblings, 1 reply; 21+ messages in thread From: Richard Gong @ 2020-05-01 16:55 UTC (permalink / raw) To: Christophe JAILLET, gregkh, atull; +Cc: linux-kernel, kernel-janitors Hi, On 5/1/20 10:48 AM, Christophe JAILLET wrote: > Le 01/05/2020 à 17:40, Richard Gong a écrit : >> Hi, >> >> On 4/29/20 1:52 AM, Christophe JAILLET wrote: >>> Replace 'devm_kmalloc_array(... | __GFP_ZERO)' with the equivalent and >>> shorter 'devm_kcalloc(...)'. >>> >> It doesn't make much sense. >> Actually devm_kcalloc returns devm_kmalloc_array(.., flag | __GFP_ZERO). >> > The only goal is to have a sightly less verbose code. > This saves one line of code and there is no need to wonder why we > explicitly pass __GFP_ZERO to kmalloc_array. > > Mostly a matter of taste. I prefer this part remain unchanged. Regards, Richard > > 'devm_kcalloc' is inlined, so the binary should be exactly the same. > > CJ > >>> 'ctrl->genpool' can not be NULL, so axe a useless test in the remove >>> function. >>> >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>> --- >>> drivers/firmware/stratix10-svc.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/firmware/stratix10-svc.c >>> b/drivers/firmware/stratix10-svc.c >>> index 739004398877..c228337cb0a1 100644 >>> --- a/drivers/firmware/stratix10-svc.c >>> +++ b/drivers/firmware/stratix10-svc.c >>> @@ -1002,8 +1002,7 @@ static int stratix10_svc_drv_probe(struct >>> platform_device *pdev) >>> if (!controller) >>> return -ENOMEM; >>> - chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, >>> - sizeof(*chans), GFP_KERNEL | __GFP_ZERO); >>> + chans = devm_kcalloc(dev, SVC_NUM_CHANNEL, sizeof(*chans), >>> GFP_KERNEL); >>> if (!chans) >>> return -ENOMEM; >>> @@ -1086,8 +1085,7 @@ static int stratix10_svc_drv_remove(struct >>> platform_device *pdev) >>> kthread_stop(ctrl->task); >>> ctrl->task = NULL; >>> } >>> - if (ctrl->genpool) >>> - gen_pool_destroy(ctrl->genpool); >>> + gen_pool_destroy(ctrl->genpool); >>> list_del(&ctrl->node); >>> return 0; >>> >> >> Regards, >> Richard >> > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code 2020-05-01 16:55 ` Richard Gong @ 2020-05-02 8:49 ` Christophe JAILLET 2020-05-05 14:18 ` Richard Gong 0 siblings, 1 reply; 21+ messages in thread From: Christophe JAILLET @ 2020-05-02 8:49 UTC (permalink / raw) To: Richard Gong, gregkh, atull; +Cc: linux-kernel, kernel-janitors Le 01/05/2020 à 18:55, Richard Gong a écrit : > Hi, > > On 5/1/20 10:48 AM, Christophe JAILLET wrote: >> Le 01/05/2020 à 17:40, Richard Gong a écrit : >>> Hi, >>> >>> On 4/29/20 1:52 AM, Christophe JAILLET wrote: >>>> Replace 'devm_kmalloc_array(... | __GFP_ZERO)' with the equivalent and >>>> shorter 'devm_kcalloc(...)'. >>>> >>> It doesn't make much sense. >>> Actually devm_kcalloc returns devm_kmalloc_array(.., flag | >>> __GFP_ZERO). >>> >> The only goal is to have a sightly less verbose code. >> This saves one line of code and there is no need to wonder why we >> explicitly pass __GFP_ZERO to kmalloc_array. >> >> Mostly a matter of taste. > I prefer this part remain unchanged. > The easiest would be just to ignore this patch entirely but if you need a v3 for the series, could you tell me if you have any comments on the 3 other patches? CJ > Regards, > Richard > >> >> 'devm_kcalloc' is inlined, so the binary should be exactly the same. > >> CJ >> >>>> 'ctrl->genpool' can not be NULL, so axe a useless test in the remove >>>> function. >>>> >>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>> --- >>>> drivers/firmware/stratix10-svc.c | 6 ++---- >>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/firmware/stratix10-svc.c >>>> b/drivers/firmware/stratix10-svc.c >>>> index 739004398877..c228337cb0a1 100644 >>>> --- a/drivers/firmware/stratix10-svc.c >>>> +++ b/drivers/firmware/stratix10-svc.c >>>> @@ -1002,8 +1002,7 @@ static int stratix10_svc_drv_probe(struct >>>> platform_device *pdev) >>>> if (!controller) >>>> return -ENOMEM; >>>> - chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, >>>> - sizeof(*chans), GFP_KERNEL | __GFP_ZERO); >>>> + chans = devm_kcalloc(dev, SVC_NUM_CHANNEL, sizeof(*chans), >>>> GFP_KERNEL); >>>> if (!chans) >>>> return -ENOMEM; >>>> @@ -1086,8 +1085,7 @@ static int stratix10_svc_drv_remove(struct >>>> platform_device *pdev) >>>> kthread_stop(ctrl->task); >>>> ctrl->task = NULL; >>>> } >>>> - if (ctrl->genpool) >>>> - gen_pool_destroy(ctrl->genpool); >>>> + gen_pool_destroy(ctrl->genpool); >>>> list_del(&ctrl->node); >>>> return 0; >>>> >>> >>> Regards, >>> Richard >>> >> > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code 2020-05-02 8:49 ` Christophe JAILLET @ 2020-05-05 14:18 ` Richard Gong 0 siblings, 0 replies; 21+ messages in thread From: Richard Gong @ 2020-05-05 14:18 UTC (permalink / raw) To: Christophe JAILLET, gregkh, atull; +Cc: linux-kernel, kernel-janitors Hi, On 5/2/20 3:49 AM, Christophe JAILLET wrote: > Le 01/05/2020 à 18:55, Richard Gong a écrit : >> Hi, >> >> On 5/1/20 10:48 AM, Christophe JAILLET wrote: >>> Le 01/05/2020 à 17:40, Richard Gong a écrit : >>>> Hi, >>>> >>>> On 4/29/20 1:52 AM, Christophe JAILLET wrote: >>>>> Replace 'devm_kmalloc_array(... | __GFP_ZERO)' with the equivalent and >>>>> shorter 'devm_kcalloc(...)'. >>>>> >>>> It doesn't make much sense. >>>> Actually devm_kcalloc returns devm_kmalloc_array(.., flag | >>>> __GFP_ZERO). >>>> >>> The only goal is to have a sightly less verbose code. >>> This saves one line of code and there is no need to wonder why we >>> explicitly pass __GFP_ZERO to kmalloc_array. >>> >>> Mostly a matter of taste. >> I prefer this part remain unchanged. >> > > The easiest would be just to ignore this patch entirely Yes, please. but if you need > a v3 for the series, could you tell me if you have any comments on the 3 > other patches? > I added some comments in your patch #3, also recommend putting all changes in one patch. Regards, Richard > CJ > > >> Regards, >> Richard >> >>> >>> 'devm_kcalloc' is inlined, so the binary should be exactly the same. > >>> CJ >>> >>>>> 'ctrl->genpool' can not be NULL, so axe a useless test in the remove >>>>> function. >>>>> >>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>>> --- >>>>> drivers/firmware/stratix10-svc.c | 6 ++---- >>>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/firmware/stratix10-svc.c >>>>> b/drivers/firmware/stratix10-svc.c >>>>> index 739004398877..c228337cb0a1 100644 >>>>> --- a/drivers/firmware/stratix10-svc.c >>>>> +++ b/drivers/firmware/stratix10-svc.c >>>>> @@ -1002,8 +1002,7 @@ static int stratix10_svc_drv_probe(struct >>>>> platform_device *pdev) >>>>> if (!controller) >>>>> return -ENOMEM; >>>>> - chans = devm_kmalloc_array(dev, SVC_NUM_CHANNEL, >>>>> - sizeof(*chans), GFP_KERNEL | __GFP_ZERO); >>>>> + chans = devm_kcalloc(dev, SVC_NUM_CHANNEL, sizeof(*chans), >>>>> GFP_KERNEL); >>>>> if (!chans) >>>>> return -ENOMEM; >>>>> @@ -1086,8 +1085,7 @@ static int stratix10_svc_drv_remove(struct >>>>> platform_device *pdev) >>>>> kthread_stop(ctrl->task); >>>>> ctrl->task = NULL; >>>>> } >>>>> - if (ctrl->genpool) >>>>> - gen_pool_destroy(ctrl->genpool); >>>>> + gen_pool_destroy(ctrl->genpool); >>>>> list_del(&ctrl->node); >>>>> return 0; >>>>> >>>> >>>> Regards, >>>> Richard >>>> >>> >> > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code 2020-05-01 15:40 ` Richard Gong 2020-05-01 15:48 ` Christophe JAILLET @ 2020-05-06 10:04 ` Dan Carpenter 1 sibling, 0 replies; 21+ messages in thread From: Dan Carpenter @ 2020-05-06 10:04 UTC (permalink / raw) To: Richard Gong Cc: Christophe JAILLET, gregkh, atull, linux-kernel, kernel-janitors On Fri, May 01, 2020 at 10:40:20AM -0500, Richard Gong wrote: > Hi, > > On 4/29/20 1:52 AM, Christophe JAILLET wrote: > > Replace 'devm_kmalloc_array(... | __GFP_ZERO)' with the equivalent and > > shorter 'devm_kcalloc(...)'. > > > It doesn't make much sense. > Actually devm_kcalloc returns devm_kmalloc_array(.., flag | __GFP_ZERO). > devm_kcalloc() is better style and easier to read. I was just reading a bunch of AMD code that does this and I almost complained to them that devm_kmalloc_array() doesn't zero the memory so they were freeing uninitialized pointers. regards, dan carpenter ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in stratix10_svc_drv_probe() @ 2020-04-29 11:51 Markus Elfring 0 siblings, 0 replies; 21+ messages in thread From: Markus Elfring @ 2020-04-29 11:51 UTC (permalink / raw) To: Christophe Jaillet, Richard Gong Cc: kernel-janitors, linux-kernel, Alan Tull, Greg Kroah-Hartman, Moritz Fischer > While at it, also move a 'platform_device_put()' call to the error handling path. How do you think about to use the message “Complete exception handling in stratix10_svc_drv_probe()” in the final commit subject? … > +++ b/drivers/firmware/stratix10-svc.c … > @@ -1043,24 +1043,34 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev) … > + return 0; > + > +put_platform: > + platform_device_put(svc->stratix10_svc_rsu); > +err_free_kfifo: … > return ret; > } Can the label “err_put_device” be more appropriate for the improved function implementation? (Or: Would you like to omit the prefix “err_” for these jump targets?) Regards, Markus ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-06-27 8:03 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-29 6:52 [PATCH 0/4 v2] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET 2020-04-29 6:52 ` [PATCH 1/4 v2] firmware: stratix10-svc: Fix genpool creation error handling Christophe JAILLET 2020-04-29 6:52 ` [PATCH 2/4 v2] firmware: stratix10-svc: Unmap some previously memremap'ed memory Christophe JAILLET 2020-05-05 14:43 ` Greg KH 2020-05-05 15:09 ` Christophe JAILLET 2020-04-29 6:52 ` [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in 'stratix10_svc_drv_probe()' Christophe JAILLET 2020-05-05 14:02 ` Richard Gong 2020-05-05 15:15 ` Christophe JAILLET 2020-06-26 19:37 ` [PATCH V3] firmware: stratix10-svc: Fix some error handling code Christophe JAILLET 2020-06-27 5:15 ` Greg KH 2020-06-27 5:15 ` Greg KH 2020-06-27 7:31 ` Marion & Christophe JAILLET 2020-06-27 8:03 ` Greg KH 2020-04-29 6:52 ` [PATCH 4/4 v2] firmware: stratix10-svc: Slightly simplify code Christophe JAILLET 2020-05-01 15:40 ` Richard Gong 2020-05-01 15:48 ` Christophe JAILLET 2020-05-01 16:55 ` Richard Gong 2020-05-02 8:49 ` Christophe JAILLET 2020-05-05 14:18 ` Richard Gong 2020-05-06 10:04 ` Dan Carpenter 2020-04-29 11:51 [PATCH 3/4 v2] firmware: stratix10-svc: Fix some error handling paths in stratix10_svc_drv_probe() Markus Elfring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).