Re: [PATCH] pinctrl: single: Fix memleak in pcs_dt_node_to_map

From: Tony Lindgren <tony@atomide.com>
To: "guomengqi (A)" <guomengqi3@huawei.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	haojian.zhuang@linaro.org, linux-arm-kernel@lists.infradead.org,
	linux-omap@vger.kernel.org
Subject: Re: [PATCH] pinctrl: single: Fix memleak in pcs_dt_node_to_map
Date: Thu, 6 Jul 2023 07:07:55 +0300	[thread overview]
Message-ID: <20230706040755.GB5089@atomide.com> (raw)
In-Reply-To: <9a490082-bc30-8a7e-2d19-fcd212771a4b@huawei.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel