[PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

[PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

[Tejun Heo]

piix_init_one() allocates host private data which should be freed by
piix_host_stop().  ich_pata_ops wasn't converted to piix_host_stop()
while merging, leaking 4 bytes on driver detach.  Fix it.

This was spotted using Kmemleak by Catalin Marinas.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Catalin Marinas <catalin.marinas@gmail.com>
---
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index c7de0bb..dfe17e1 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -330,7 +330,7 @@ static const struct ata_port_operations ich_pata_ops = {
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_host_stop,
+	.host_stop		= piix_host_stop,
 };
 
 static const struct ata_port_operations piix_sata_ops = {

Re: [PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

[Alan]

On Mon, 11 Dec 2006 22:26:25 +0900
Tejun Heo <htejun@gmail.com> wrote:

> piix_init_one() allocates host private data which should be freed by
> piix_host_stop().  ich_pata_ops wasn't converted to piix_host_stop()
> while merging, leaking 4 bytes on driver detach.  Fix it.
> 
> This was spotted using Kmemleak by Catalin Marinas.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@gmail.com>

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

[Catalin Marinas]

Tejun,

On 11/12/06, Tejun Heo <htejun@gmail.com> wrote:
> piix_init_one() allocates host private data which should be freed by
> piix_host_stop().  ich_pata_ops wasn't converted to piix_host_stop()
> while merging, leaking 4 bytes on driver detach.  Fix it.

I tried your patch last night but the leak is still reported. I need
to investigate further and put some printk's in the piix_host_stop
function to check whether the freeing really takes place.

What I can't follow is where the ata_port_info.private_data
(port_info[] or ppinfo[]) in piix_init_one gets transfered to
ata_host.private_data (the "host" argument) that piix_host_stop tries
to free.

-- 
Catalin

Re: [PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

[Catalin Marinas]

On 13/12/06, Catalin Marinas <catalin.marinas@gmail.com> wrote:
> On 11/12/06, Tejun Heo <htejun@gmail.com> wrote:
> > piix_init_one() allocates host private data which should be freed by
> > piix_host_stop().  ich_pata_ops wasn't converted to piix_host_stop()
> > while merging, leaking 4 bytes on driver detach.  Fix it.
>
> I tried your patch last night but the leak is still reported. I need
> to investigate further and put some printk's in the piix_host_stop
> function to check whether the freeing really takes place.

The piix_host_stop() isn't actually called on my machine, so this is
not the cause of the leak.

What causes the leak seem to be the error returned by
ata_pci_init_one() called from piix_init_one(). These are the related
kernel messages:

ata_piix 0000:00:1f.1: version 2.00ac7
ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 16 (level, low) -> IRQ 18
PCI: Unable to reserve I/O region #1:8@1f0 for device 0000:00:1f.1
ata_piix: probe of 0000:00:1f.1 failed with error -16

I think the call to ata_pci_init_one() should be followed by some
clean-up in case it fails. There is also another return without
clean-up in piix_init_one() after the call to piix_disable_ahci(). I
don't have time to try this now.

-- 
Catalin

[PATCH] Fix a memory leak in piix_init_one

[Catalin Marinas]

The piix_init_one() function in drivers/ata/ata_piix.c can return an
error without cleaning up the piix_host_priv structure
allocation. This patch adds a kfree() call on the error path.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---

 drivers/ata/ata_piix.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index dfe17e1..6717891 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -1051,6 +1051,7 @@ static int piix_init_one (struct pci_dev
 	struct ata_port_info *ppinfo[2] = { &port_info[0], &port_info[1] };
 	struct piix_host_priv *hpriv;
 	unsigned long port_flags;
+	int ret;
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev,
@@ -1075,9 +1076,9 @@ static int piix_init_one (struct pci_dev
 		u8 tmp;
 		pci_read_config_byte(pdev, PIIX_SCC, &tmp);
 		if (tmp == PIIX_AHCI_DEVICE) {
-			int rc = piix_disable_ahci(pdev);
-			if (rc)
-				return rc;
+			ret = piix_disable_ahci(pdev);
+			if (ret)
+				goto cleanup;
 		}
 	}
 
@@ -1107,7 +1108,15 @@ static int piix_init_one (struct pci_dev
 		port_info[1].mwdma_mask = 0;
 		port_info[1].udma_mask = 0;
 	}
-	return ata_pci_init_one(pdev, ppinfo, 2);
+	ret = ata_pci_init_one(pdev, ppinfo, 2);
+	if (ret)
+		goto cleanup;
+	goto out;
+
+ cleanup:
+	kfree(hpriv);
+ out:
+	return ret;
 }
 
 static void piix_host_stop(struct ata_host *host)