Messages in this thread | | | Subject | Re: [PATCH v2 1/5] thermal/drivers/core: Use a char pointer for the cooling device name | From | Lukasz Luba <> | Date | Mon, 15 Mar 2021 09:58:24 +0000 |
| |
On 3/15/21 9:40 AM, Lukasz Luba wrote: > > > On 3/12/21 9:01 PM, Daniel Lezcano wrote: >> On 12/03/2021 19:49, Lukasz Luba wrote: >>> >>> >>> On 3/12/21 5:03 PM, Daniel Lezcano wrote: >>>> We want to have any kind of name for the cooling devices as we do no >>>> longer want to rely on auto-numbering. Let's replace the cooling >>>> device's fixed array by a char pointer to be allocated dynamically >>>> when registering the cooling device, so we don't limit the length of >>>> the name. >>>> >>>> Rework the error path at the same time as we have to rollback the >>>> allocations in case of error. >>>> >>>> Tested with a dummy device having the name: >>>> "Llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch" >>>> >>>> A village on the island of Anglesey (Wales), known to have the longest >>>> name in Europe. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> .../ethernet/mellanox/mlxsw/core_thermal.c | 2 +- >>>> drivers/thermal/thermal_core.c | 38 >>>> +++++++++++-------- >>>> include/linux/thermal.h | 2 +- >>>> 3 files changed, 24 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c >>>> b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c >>>> index bf85ce9835d7..7447c2a73cbd 100644 >>>> --- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c >>>> +++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c >>>> @@ -141,7 +141,7 @@ static int mlxsw_get_cooling_device_idx(struct >>>> mlxsw_thermal *thermal, >>>> /* Allow mlxsw thermal zone binding to an external cooling >>>> device */ >>>> for (i = 0; i < ARRAY_SIZE(mlxsw_thermal_external_allowed_cdev); >>>> i++) { >>>> if (strnstr(cdev->type, >>>> mlxsw_thermal_external_allowed_cdev[i], >>>> - sizeof(cdev->type))) >>>> + strlen(cdev->type))) >>>> return 0; >>>> } >>>> diff --git a/drivers/thermal/thermal_core.c >>>> b/drivers/thermal/thermal_core.c >>>> index 996c038f83a4..9ef8090eb645 100644 >>>> --- a/drivers/thermal/thermal_core.c >>>> +++ b/drivers/thermal/thermal_core.c >>>> @@ -960,10 +960,7 @@ __thermal_cooling_device_register(struct >>>> device_node *np, >>>> { >>>> struct thermal_cooling_device *cdev; >>>> struct thermal_zone_device *pos = NULL; >>>> - int result; >>>> - >>>> - if (type && strlen(type) >= THERMAL_NAME_LENGTH) >>>> - return ERR_PTR(-EINVAL); >>>> + int ret; >>>> if (!ops || !ops->get_max_state || !ops->get_cur_state || >>>> !ops->set_cur_state) >>>> @@ -973,14 +970,17 @@ __thermal_cooling_device_register(struct >>>> device_node *np, >>>> if (!cdev) >>>> return ERR_PTR(-ENOMEM); >>>> - result = ida_simple_get(&thermal_cdev_ida, 0, 0, GFP_KERNEL); >>>> - if (result < 0) { >>>> - kfree(cdev); >>>> - return ERR_PTR(result); >>>> + ret = ida_simple_get(&thermal_cdev_ida, 0, 0, GFP_KERNEL); >>>> + if (ret < 0) >>>> + goto out_kfree_cdev; >>>> + cdev->id = ret; >>>> + >>>> + cdev->type = kstrdup(type ? type : "", GFP_KERNEL); >>>> + if (!cdev->type) { >>>> + ret = -ENOMEM; >>> >>> Since we haven't called the device_register() yet, I would call here: >>> kfree(cdev); >>> and then jump >> >> I'm not sure to understand, we have to remove the ida, no ? > > Yes, we have to remove 'ida' and you jump to that label: > goto out_ida_remove; > but under that label, there is no 'put_device()'. > We could have here, before the 'goto', a simple kfree, which > should be safe, since we haven't called the device_register() yet. > Something like: > > --------8<------------------------------ > cdev->type = kstrdup(type ? type : "", GFP_KERNEL); > if (!cdev->type) { > ret = -ENOMEM; > kfree(cdev); > goto out_ida_remove; > } > > -------->8------------------------------ >
I've check that label and probably not easy to modify it and put conditional there. So probably you would have to call everything here (not jumping to label):
ida_simple_remove(&thermal_cdev_ida, cdev->id); kfree(cdev); return ERR_PTR(-_ENOMEM);
> >> >>> Other than that, LGTM >>> >>> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com> >>> >>> Regards, >>> Lukasz >> >>
| |