[PATCH v1 1/4] auxdisplay: hd44780: Fix memory leak on ->remove()

[PATCH v1 1/4] auxdisplay: hd44780: Fix memory leak on ->remove()

[Andy Shevchenko]

We have to free on ->remove() the allocated resources on ->probe().

Fixes: d47d88361fee ("auxdisplay: Add HD44780 Character LCD support")
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/hd44780.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
index 9ad93ea42fdc..3cde351fb5c9 100644
--- a/drivers/auxdisplay/hd44780.c
+++ b/drivers/auxdisplay/hd44780.c
@@ -280,6 +280,8 @@ static int hd44780_remove(struct platform_device *pdev)
 	struct charlcd *lcd = platform_get_drvdata(pdev);
 
 	charlcd_unregister(lcd);
+
+	kfree(lcd);
 	return 0;
 }
 
-- 
2.20.1

[PATCH v1 2/4] auxdisplay: charlcd: Introduce charlcd_free() helper

[Andy Shevchenko]

The charlcd_free() is a counterpart to charlcd_alloc()
and should be called symmetrically on tear down.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/charlcd.c | 6 ++++++
 include/misc/charlcd.h       | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 60e0b772673f..882be08ddad5 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -818,6 +818,12 @@ struct charlcd *charlcd_alloc(unsigned int drvdata_size)
 }
 EXPORT_SYMBOL_GPL(charlcd_alloc);
 
+void charlcd_free(struct charlcd *lcd)
+{
+	kfree(lcd);
+}
+EXPORT_SYMBOL_GPL(charlcd_free);
+
 static int panel_notify_sys(struct notifier_block *this, unsigned long code,
 			    void *unused)
 {
diff --git a/include/misc/charlcd.h b/include/misc/charlcd.h
index 23f61850f363..1832402324ce 100644
--- a/include/misc/charlcd.h
+++ b/include/misc/charlcd.h
@@ -35,6 +35,7 @@ struct charlcd_ops {
 };
 
 struct charlcd *charlcd_alloc(unsigned int drvdata_size);
+void charlcd_free(struct charlcd *lcd);
 
 int charlcd_register(struct charlcd *lcd);
 int charlcd_unregister(struct charlcd *lcd);
-- 
2.20.1

[PATCH v1 3/4] auxdisplay: panel: Convert to use charlcd_free()

[Andy Shevchenko]

Convert to use charlcd_free() instead of kfree() for sake of type check.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/panel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index 21b9b2f2470a..e06de63497cf 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -1620,7 +1620,7 @@ static void panel_attach(struct parport *port)
 	if (lcd.enabled)
 		charlcd_unregister(lcd.charlcd);
 err_unreg_device:
-	kfree(lcd.charlcd);
+	charlcd_free(lcd.charlcd);
 	lcd.charlcd = NULL;
 	parport_unregister_device(pprt);
 	pprt = NULL;
@@ -1647,7 +1647,7 @@ static void panel_detach(struct parport *port)
 	if (lcd.enabled) {
 		charlcd_unregister(lcd.charlcd);
 		lcd.initialized = false;
-		kfree(lcd.charlcd);
+		charlcd_free(lcd.charlcd);
 		lcd.charlcd = NULL;
 	}
 
-- 
2.20.1

[PATCH v1 4/4] auxdisplay: hd44780: Convert to use charlcd_free()

[Andy Shevchenko]

Convert to use charlcd_free() instead of kfree() for sake of type check.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/hd44780.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
index 3cde351fb5c9..ab15b64707ad 100644
--- a/drivers/auxdisplay/hd44780.c
+++ b/drivers/auxdisplay/hd44780.c
@@ -271,7 +271,7 @@ static int hd44780_probe(struct platform_device *pdev)
 	return 0;
 
 fail:
-	kfree(lcd);
+	charlcd_free(lcd);
 	return ret;
 }
 
@@ -281,7 +281,7 @@ static int hd44780_remove(struct platform_device *pdev)
 
 	charlcd_unregister(lcd);
 
-	kfree(lcd);
+	charlcd_free(lcd);
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH v1 1/4] auxdisplay: hd44780: Fix memory leak on ->remove()

[Geert Uytterhoeven]

Hi Andy,

On Tue, Mar 12, 2019 at 2:18 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> We have to free on ->remove() the allocated resources on ->probe().
>
> Fixes: d47d88361fee ("auxdisplay: Add HD44780 Character LCD support")
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks, nice catch!

> --- a/drivers/auxdisplay/hd44780.c
> +++ b/drivers/auxdisplay/hd44780.c
> @@ -280,6 +280,8 @@ static int hd44780_remove(struct platform_device *pdev)
>         struct charlcd *lcd = platform_get_drvdata(pdev);
>
>         charlcd_unregister(lcd);
> +
> +       kfree(lcd);

(wearing a newer (and hopefully wiser ;-) hat than when I wrote the code)

While this is correct for the current implementation of struct charlcd_priv,
this may be a bit fragile.

What about adding a charlcd_free() wrapper, which can do kfree(to_priv(lcd)),
and be used in the probe failure path, too?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 1/4] auxdisplay: hd44780: Fix memory leak on ->remove()

[Andy Shevchenko]

On Tue, Mar 12, 2019 at 02:47:01PM +0100, Geert Uytterhoeven wrote:
> On Tue, Mar 12, 2019 at 2:18 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > We have to free on ->remove() the allocated resources on ->probe().
> >
> > Fixes: d47d88361fee ("auxdisplay: Add HD44780 Character LCD support")
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thanks, nice catch!
> 
> (wearing a newer (and hopefully wiser ;-) hat than when I wrote the code)
> 
> While this is correct for the current implementation of struct charlcd_priv,
> this may be a bit fragile.

This patch will be the same in the next version due to possibility to easy
backport.

> What about adding a charlcd_free() wrapper, which can do kfree(to_priv(lcd)),
> and be used in the probe failure path, too?

This has been partially done in the rest of the series, but I got your advise
and will change to use to_priv() in v2. Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] auxdisplay: hd44780: Fix memory leak on ->remove()

[Geert Uytterhoeven]

Hi Andy,

On Tue, Mar 12, 2019 at 3:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Mar 12, 2019 at 02:47:01PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Mar 12, 2019 at 2:18 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > We have to free on ->remove() the allocated resources on ->probe().
> > >
> > > Fixes: d47d88361fee ("auxdisplay: Add HD44780 Character LCD support")
> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Thanks, nice catch!
> >
> > (wearing a newer (and hopefully wiser ;-) hat than when I wrote the code)
> >
> > While this is correct for the current implementation of struct charlcd_priv,
> > this may be a bit fragile.
>
> This patch will be the same in the next version due to possibility to easy
> backport.

OK.

> > What about adding a charlcd_free() wrapper, which can do kfree(to_priv(lcd)),
> > and be used in the probe failure path, too?
>
> This has been partially done in the rest of the series, but I got your advise
> and will change to use to_priv() in v2. Thanks!

Ah, hadn't noticed the rest of the series. Will review...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds