| Message ID | 20230703081716.15810-1-guomengqi3@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: single: Fix memleak in pcs_dt_node_to_map | expand |
On Mon, Jul 3, 2023 at 10:24 AM Guo Mengqi <guomengqi3@huawei.com> wrote: > In a reliability test which repeatedly load and remove a module, > I found some kmalloc-256 memory leaks in pinctrl-single. > > pcs_dt_node_to_map() will recognize a dt_node and > make a mapping for it. Along the way some pinctrl functions and groups > are registered in pinctrl-single controller. These functions/groups are > registered once and not removed during the system lifetime. > > When the client module loads again, pcs_dt_node_to_map() fail to consider > this situation, create the same set of resources, and does not release or > use them. > > To fix this, add a check at the start of pcs_parse_one_pinctrl_entry/ > pcs_parse_bits_in_pinctrl_entry. If the target is found, > then all the resource allocation and parsing work can be skipped, > just set the mapping with existing function/group information. > > Fixes: 8b8b091bf07f ("pinctrl: Add one-register-per-pin type device tree > based pinctrl driver") > > Signed-off-by: Guo Mengqi <guomengqi3@huawei.com> Good catch! I expect Tony to review the patch in-depth. > -static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, > +int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, > const char *function) > { > const struct pinmux_ops *ops = pctldev->desc->pmxops; It appears you need to add EXPORT_SYMBOL_GPL() for this function so the module can build. (This is why the build robot complains.) Yours, Linus Walleij
在 2023/7/4 17:18, Linus Walleij 写道: > On Mon, Jul 3, 2023 at 10:24 AM Guo Mengqi <guomengqi3@huawei.com> wrote: > >> In a reliability test which repeatedly load and remove a module, >> I found some kmalloc-256 memory leaks in pinctrl-single. >> >> pcs_dt_node_to_map() will recognize a dt_node and >> make a mapping for it. Along the way some pinctrl functions and groups >> are registered in pinctrl-single controller. These functions/groups are >> registered once and not removed during the system lifetime. >> >> When the client module loads again, pcs_dt_node_to_map() fail to consider >> this situation, create the same set of resources, and does not release or >> use them. >> >> To fix this, add a check at the start of pcs_parse_one_pinctrl_entry/ >> pcs_parse_bits_in_pinctrl_entry. If the target is found, >> then all the resource allocation and parsing work can be skipped, >> just set the mapping with existing function/group information. >> >> Fixes: 8b8b091bf07f ("pinctrl: Add one-register-per-pin type device tree >> based pinctrl driver") >> >> Signed-off-by: Guo Mengqi <guomengqi3@huawei.com> > Good catch! > > I expect Tony to review the patch in-depth. Thank you :) >> -static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, >> +int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, >> const char *function) >> { >> const struct pinmux_ops *ops = pctldev->desc->pmxops; > It appears you need to add EXPORT_SYMBOL_GPL() for this function > so the module can build. (This is why the build robot complains.) Yes, it happens when config=M. I will send a v2 patch later to fix this. > Yours, > Linus Walleij > .
Hi, * guomengqi (A) <guomengqi3@huawei.com> [230706 03:21]: > 在 2023/7/4 17:18, Linus Walleij 写道: > > On Mon, Jul 3, 2023 at 10:24 AM Guo Mengqi <guomengqi3@huawei.com> wrote: > > > > > In a reliability test which repeatedly load and remove a module, > > > I found some kmalloc-256 memory leaks in pinctrl-single. > > > > > > pcs_dt_node_to_map() will recognize a dt_node and > > > make a mapping for it. Along the way some pinctrl functions and groups > > > are registered in pinctrl-single controller. These functions/groups are > > > registered once and not removed during the system lifetime. > > > > > > When the client module loads again, pcs_dt_node_to_map() fail to consider > > > this situation, create the same set of resources, and does not release or > > > use them. > > > > > > To fix this, add a check at the start of pcs_parse_one_pinctrl_entry/ > > > pcs_parse_bits_in_pinctrl_entry. If the target is found, > > > then all the resource allocation and parsing work can be skipped, > > > just set the mapping with existing function/group information. > > > > > > Fixes: 8b8b091bf07f ("pinctrl: Add one-register-per-pin type device tree > > > based pinctrl driver") > > > > > > Signed-off-by: Guo Mengqi <guomengqi3@huawei.com> > > Good catch! > > > > I expect Tony to review the patch in-depth. > > Thank you :) Thanks for looking into it. I wonder if we can rely on naming for pinmux_func_name_to_selector() though. Can things change in a way where we need to release everything and reparse? Mostly wondering what happens with DT overlays? > > > -static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, > > > +int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, > > > const char *function) > > > { > > > const struct pinmux_ops *ops = pctldev->desc->pmxops; > > It appears you need to add EXPORT_SYMBOL_GPL() for this function > > so the module can build. (This is why the build robot complains.) > Yes, it happens when config=M. I will send a v2 patch later to fix this. That change might be worth doing in any case if there is need for it. Regards, Tony
在 2023/7/6 12:07, Tony Lindgren 写道: > Hi, > > * guomengqi (A) <guomengqi3@huawei.com> [230706 03:21]: >> 在 2023/7/4 17:18, Linus Walleij 写道: >>> On Mon, Jul 3, 2023 at 10:24 AM Guo Mengqi <guomengqi3@huawei.com> wrote: >>> >>>> In a reliability test which repeatedly load and remove a module, >>>> I found some kmalloc-256 memory leaks in pinctrl-single. >>>> >>>> pcs_dt_node_to_map() will recognize a dt_node and >>>> make a mapping for it. Along the way some pinctrl functions and groups >>>> are registered in pinctrl-single controller. These functions/groups are >>>> registered once and not removed during the system lifetime. >>>> >>>> When the client module loads again, pcs_dt_node_to_map() fail to consider >>>> this situation, create the same set of resources, and does not release or >>>> use them. >>>> >>>> To fix this, add a check at the start of pcs_parse_one_pinctrl_entry/ >>>> pcs_parse_bits_in_pinctrl_entry. If the target is found, >>>> then all the resource allocation and parsing work can be skipped, >>>> just set the mapping with existing function/group information. >>>> >>>> Fixes: 8b8b091bf07f ("pinctrl: Add one-register-per-pin type device tree >>>> based pinctrl driver") >>>> >>>> Signed-off-by: Guo Mengqi <guomengqi3@huawei.com> >>> Good catch! >>> >>> I expect Tony to review the patch in-depth. >> Thank you :) > Thanks for looking into it. I wonder if we can rely on naming for > pinmux_func_name_to_selector() though. Can things change in a way where > we need to release everything and reparse? Mostly wondering what happens > with DT overlays? Hi Let me confirm, you mean when the pin controller dtsi changed at runtime, some functions and groups can change silently while the dt-node name remains same, so the old data needs to be released and reparsed, right? I don't know much about DT overlays. I can look deeper into revelant codes, maybe do some experiments too. My guess now is DT overlay will first remove the old parsed nodes, then create new ones. If so, the modification to pcs_dt_node_to_map() in this patch is not affected. Regards, Mengqi >>>> -static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, >>>> +int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, >>>> const char *function) >>>> { >>>> const struct pinmux_ops *ops = pctldev->desc->pmxops; >>> It appears you need to add EXPORT_SYMBOL_GPL() for this function >>> so the module can build. (This is why the build robot complains.) >> Yes, it happens when config=M. I will send a v2 patch later to fix this. > That change might be worth doing in any case if there is need for it. > > Regards, > > Tony > > .
Hi, * guomengqi (A) <guomengqi3@huawei.com> [230712 10:00]: > 在 2023/7/6 12:07, Tony Lindgren 写道: > > Thanks for looking into it. I wonder if we can rely on naming for > > pinmux_func_name_to_selector() though. Can things change in a way where > > we need to release everything and reparse? Mostly wondering what happens > > with DT overlays? > > Let me confirm, you mean when the pin controller dtsi changed at runtime, > some functions and groups can change silently while the dt-node name remains > same, so the old data needs to be released and reparsed, right? > > I don't know much about DT overlays. I can look deeper into revelant codes, > maybe do some experiments too. > > My guess now is DT overlay will first remove the old parsed nodes, then > create new ones. If so, the modification to pcs_dt_node_to_map() in this > patch is not affected. OK yeah good to check it to confirm. Regards, Tony
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 0dabbcf68b9f..3412e7f248d9 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -949,11 +949,14 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np, if (!nconfs) return -ENOTSUPP; - func->conf = devm_kcalloc(pcs->dev, + if (!func->conf) { + func->conf = devm_kcalloc(pcs->dev, nconfs, sizeof(struct pcs_conf_vals), GFP_KERNEL); - if (!func->conf) - return -ENOMEM; + if (!func->conf) + return -ENOMEM; + } + func->nconfs = nconfs; conf = &(func->conf[0]); m++; @@ -1005,6 +1008,17 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, struct pcs_func_vals *vals; int rows, *pins, found = 0, res = -ENOMEM, i, fsel, gsel; struct pcs_function *function = NULL; + struct function_desc *desc; + + mutex_lock(&pcs->mutex); + fsel = pinmux_func_name_to_selector(pcs->pctl, np->name); + if (fsel >= 0) { + desc = pinmux_generic_get_function(pcs->pctl, fsel); + WARN_ON(!desc); + function = desc->data; + goto set_map; + } + mutex_unlock(&pcs->mutex); rows = pinctrl_count_index_with_args(np, name); if (rows <= 0) { @@ -1075,6 +1089,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, goto free_function; } +set_map: (*map)->type = PIN_MAP_TYPE_MUX_GROUP; (*map)->data.mux.group = np->name; (*map)->data.mux.function = np->name; @@ -1132,6 +1147,12 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs, return -ENOTSUPP; } + mutex_lock(&pcs->mutex); + fsel = pinmux_func_name_to_selector(pcs->pctl, np->name); + if (fsel >= 0) + goto set_map; + mutex_unlock(&pcs->mutex); + npins_in_row = pcs->width / pcs->bits_per_pin; vals = devm_kzalloc(pcs->dev, @@ -1223,6 +1244,7 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs, if (res < 0) goto free_function; +set_map: (*map)->type = PIN_MAP_TYPE_MUX_GROUP; (*map)->data.mux.group = np->name; (*map)->data.mux.function = np->name; diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 82c750a31952..1d7b3df972b4 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -320,7 +320,7 @@ int pinmux_gpio_direction(struct pinctrl_dev *pctldev, return ret; } -static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, +int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, const char *function) { const struct pinmux_ops *ops = pctldev->desc->pmxops; diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h index ea6f99c24aa5..3da8b38910b5 100644 --- a/drivers/pinctrl/pinmux.h +++ b/drivers/pinctrl/pinmux.h @@ -37,6 +37,8 @@ int pinmux_gpio_direction(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned pin, bool input); +int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev, + const char *function); int pinmux_map_to_setting(const struct pinctrl_map *map, struct pinctrl_setting *setting); void pinmux_free_setting(const struct pinctrl_setting *setting);
In a reliability test which repeatedly load and remove a module, I found some kmalloc-256 memory leaks in pinctrl-single. pcs_dt_node_to_map() will recognize a dt_node and make a mapping for it. Along the way some pinctrl functions and groups are registered in pinctrl-single controller. These functions/groups are registered once and not removed during the system lifetime. When the client module loads again, pcs_dt_node_to_map() fail to consider this situation, create the same set of resources, and does not release or use them. To fix this, add a check at the start of pcs_parse_one_pinctrl_entry/ pcs_parse_bits_in_pinctrl_entry. If the target is found, then all the resource allocation and parsing work can be skipped, just set the mapping with existing function/group information. Fixes: 8b8b091bf07f ("pinctrl: Add one-register-per-pin type device tree based pinctrl driver") Signed-off-by: Guo Mengqi <guomengqi3@huawei.com> --- drivers/pinctrl/pinctrl-single.c | 28 +++++++++++++++++++++++++--- drivers/pinctrl/pinmux.c | 2 +- drivers/pinctrl/pinmux.h | 2 ++ 3 files changed, 28 insertions(+), 4 deletions(-)