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
'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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
> 'svc_create_memory_pool()' returns an error pointer on error, not NULL.
Such information is helpful.
> Fix the corresponding test and return value accordingly.
>
> Move the genpool allocation after a few devm_kzalloc in order to ease
> error handling.
How do you think about a wording variant like the following?
Change description:
* Use a check of the failure predicate which is documented for
the svc_create_memory_pool() function in the same source file.
* Move the genpool allocation behind a few devm_kzalloc() calls
in order to ease exception handling.
Regards,
Markus
> 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.
I would find it clearer to integrate also such small source code improvements
by separate patches.
Regards,
Markus
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
>>> ---
>>> 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
>>
>
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 <[email protected]>
>>>> ---
>>>> 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
>>>
>>
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>>>>> ---
>>>>> 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
>>>>
>>>
>>
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
>> ---
>> 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
>
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
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 <[email protected]>
---
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
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
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?
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
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