[PATCH] input: altera_ps2: Store struct serio in struct ps2if

[PATCH] input: altera_ps2: Store struct serio in struct ps2if

[Tobias Klauser]

It doesn't make much sense to allocate struct ps2if and struct serio
separately as they're allocated and used in conjunction.

Also ps2if->io wasn't freed in altera_ps2_remove() even tough it should
have been and thus caused a memory leak. This is also (implicitely)
fixed by this change.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 drivers/input/serio/altera_ps2.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
index cc11f4e..fca6196 100644
--- a/drivers/input/serio/altera_ps2.c
+++ b/drivers/input/serio/altera_ps2.c
@@ -24,10 +24,10 @@
 #define DRV_NAME "altera_ps2"
 
 struct ps2if {
-	struct serio *io;
 	struct resource *iomem_res;
 	void __iomem *base;
 	unsigned irq;
+	struct serio io;
 };
 
 /*
@@ -41,7 +41,7 @@ static irqreturn_t altera_ps2_rxint(int irq, void *dev_id)
 	int handled = IRQ_NONE;
 
 	while ((status = readl(ps2if->base)) & 0xffff0000) {
-		serio_interrupt(ps2if->io, status & 0xff, 0);
+		serio_interrupt(&ps2if->io, status & 0xff, 0);
 		handled = IRQ_HANDLED;
 	}
 
@@ -88,11 +88,11 @@ static int __devinit altera_ps2_probe(struct platform_device *pdev)
 	int error, irq;
 
 	ps2if = kzalloc(sizeof(struct ps2if), GFP_KERNEL);
-	serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
-	if (!ps2if || !serio) {
+	if (!ps2if) {
 		error = -ENOMEM;
 		goto err_free_mem;
 	}
+	serio = &ps2if->io;
 
 	serio->id.type		= SERIO_8042;
 	serio->write		= altera_ps2_write;
@@ -102,7 +102,6 @@ static int __devinit altera_ps2_probe(struct platform_device *pdev)
 	strlcpy(serio->phys, dev_name(&pdev->dev), sizeof(serio->phys));
 	serio->port_data	= ps2if;
 	serio->dev.parent	= &pdev->dev;
-	ps2if->io		= serio;
 
 	ps2if->iomem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (ps2if->iomem_res == NULL) {
@@ -110,7 +109,6 @@ static int __devinit altera_ps2_probe(struct platform_device *pdev)
 		goto err_free_mem;
 	}
 
-
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		error = -ENXIO;
@@ -140,7 +138,7 @@ static int __devinit altera_ps2_probe(struct platform_device *pdev)
 
 	dev_info(&pdev->dev, "base %p, irq %d\n", ps2if->base, ps2if->irq);
 
-	serio_register_port(ps2if->io);
+	serio_register_port(&ps2if->io);
 	platform_set_drvdata(pdev, ps2if);
 
 	return 0;
@@ -152,7 +150,6 @@ static int __devinit altera_ps2_probe(struct platform_device *pdev)
 			   resource_size(ps2if->iomem_res));
  err_free_mem:
 	kfree(ps2if);
-	kfree(serio);
 	return error;
 }
 
@@ -164,7 +161,7 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
 	struct ps2if *ps2if = platform_get_drvdata(pdev);
 
 	platform_set_drvdata(pdev, NULL);
-	serio_unregister_port(ps2if->io);
+	serio_unregister_port(&ps2if->io);
 	free_irq(ps2if->irq, ps2if);
 	iounmap(ps2if->base);
 	release_mem_region(ps2if->iomem_res->start,
-- 
1.7.5.4

Re: [PATCH] input: altera_ps2: Store struct serio in struct ps2if

[Dmitry Torokhov]

Hi Tobias,

On Wed, Mar 07, 2012 at 02:44:24PM +0100, Tobias Klauser wrote:
> It doesn't make much sense to allocate struct ps2if and struct serio
> separately as they're allocated and used in conjunction.
> 

Actually, it does. Serio is a refcounted structure and has different
lifetime rules than struct ps2if. It may get deleted by
serio_unregister_port() or it may live on until after ps2if is freed and
altera_ps2 module is unloaded and it should stay allocated separately.

> Also ps2if->io wasn't freed in altera_ps2_remove() even tough it should
> have been and thus caused a memory leak. This is also (implicitely)
> fixed by this change.

No, there was't a memory leak as serio core takes care of freeing the
object when refrerence count reaches 0.

Thanks.

-- 
Dmitry

Re: [PATCH] input: altera_ps2: Store struct serio in struct ps2if

[Tobias Klauser]

Hi Dmitry

On 2012-03-07 at 18:25:24 +0100, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Wed, Mar 07, 2012 at 02:44:24PM +0100, Tobias Klauser wrote:
> > It doesn't make much sense to allocate struct ps2if and struct serio
> > separately as they're allocated and used in conjunction.
> > 
> 
> Actually, it does. Serio is a refcounted structure and has different
> lifetime rules than struct ps2if. It may get deleted by
> serio_unregister_port() or it may live on until after ps2if is freed and
> altera_ps2 module is unloaded and it should stay allocated separately.

Oh, sorry. I didn't look careful enough then. Thanks a lot for clearing
that up!

I was looking at xilinx_ps2 and figured they do it like this. Would that
mean a "bug" there, or am I mislead again?

Thanks
Tobias

Re: [PATCH] input: altera_ps2: Store struct serio in struct ps2if

[Dmitry Torokhov]

On Wed, Mar 07, 2012 at 09:03:25PM +0100, Tobias Klauser wrote:
> Hi Dmitry
> 
> On 2012-03-07 at 18:25:24 +0100, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Mar 07, 2012 at 02:44:24PM +0100, Tobias Klauser wrote:
> > > It doesn't make much sense to allocate struct ps2if and struct serio
> > > separately as they're allocated and used in conjunction.
> > > 
> > 
> > Actually, it does. Serio is a refcounted structure and has different
> > lifetime rules than struct ps2if. It may get deleted by
> > serio_unregister_port() or it may live on until after ps2if is freed and
> > altera_ps2 module is unloaded and it should stay allocated separately.
> 
> Oh, sorry. I didn't look careful enough then. Thanks a lot for clearing
> that up!
> 
> I was looking at xilinx_ps2 and figured they do it like this. Would that
> mean a "bug" there, or am I mislead again?

Yes, there is a bug, thanks for letting me know.

-- 
Dmitry