Skip to contentSkip to conversation

: [RFC] Fix error handlnig in hwif-init and sgiioc4 driver

PATCH In review Prarit Bhargava 2 participants 6 messages Jan 26, 2005 – Feb 8, 2005

Subsystems (3)
ACPI PCI libata
lore
Share & tools
lore

Prarit Bhargava

author
#0 lore raw

PATCH 1/1 : [RFC] Fix error handlnig in hwif-init and sgiioc4 driver

I didn't see any ACKs on this so I'm resubmitting....

I discovered an issue where a hwif_init failure lead to /proc/ide files being created for devices that failed probes. This resulted in oops/WARN_ON/BUG_ON executions through the kernel depending on what actions were on going.

The first part of the fix (submitted to linux-ia64 and acpi-devel for review -- I'm including it for completeness) was to fix incorrect error handling in the ACPI layer:

===== drivers/acpi/pci_irq.c 1.35 vs edited =====

drivers/acpi/pci_irq.c 2005-01-14 08:14:34 -05:00 renamed from drivers/acpi/pci_irq.c 2005-01-04 21:48:17 -05:00 +2 -2
@@ -487,10 +487,10 @@
       * If no PRT entry was found, we'll try to derive an IRQ from the
       * device's parent bridge.
       */
-       if (!gsi)
+       if (gsi == -1)
              gsi = acpi_pci_irq_derive(dev, pin,
                                        &edge_level, &active_high_low);
-       if (!gsi)
+       if (gsi == -1)
              return_VOID;
 
      /*

The second part of the fix is to the core IDE layer and to the SGI IOC4 IDE driver which need to handle errors properly from failed hwif initializations. This patch is attached for review.

===== drivers/ide/ide-probe.c 1.90 vs edited =====

drivers/ide/ide-probe.c +4 -1
@@ -841,7 +841,10 @@
 	if (fixup)
 		fixup(hwif);
 
-	hwif_init(hwif);
+	if (!hwif_init(hwif)) {
+		printk("%s: Failed to initialize IDE interface\n", hwif->name);
+		return -1;
+	}
 
 	if (hwif->present) {
 		u16 unit = 0;

===== drivers/ide/pci/sgiioc4.c 1.22 vs edited =====

drivers/ide/pci/sgiioc4.c +4 -1
@@ -669,7 +669,10 @@
 		printk(KERN_INFO "%s: %s Bus-Master DMA disabled\n",
 		       hwif->name, d->name);
 
-	probe_hwif_init(hwif);
+	if (probe_hwif_init(hwif)) {
+		printk(KERN_INFO "%s: initialization failed\n", hwif->name);
+		return -EIO;
+	}
 
 	/* Create /proc/ide entries */
 	create_proc_ide_interfaces();
#1 lore raw
Quoted 1 line · Prarit Bhargava

I didn't see any ACKs on this so I'm resubmitting....

sorry for the delay

Quoted 2 lines

I discovered an issue where a hwif_init failure lead to /proc/ide files being created for devices that failed probes. This resulted in

create_proc_ide_interfaces() doesn't create /proc/ide files for hwif->present == 0 interfaces but hwif_init() sets hwif->present to 0 too late. Also this hwif->present games are silly and should be removed after your patch is applied and locking is verified.

Quoted 3 lines

The second part of the fix is to the core IDE layer and to the SGI IOC4 IDE driver which need to handle errors properly from failed hwif initializations. This patch is attached for review.

please remember about Signed-off-by:

drivers/ide/ide-probe.c 2005-01-14 08:18:43 -05:00 line 847

Quoted 12 lines

===== drivers/ide/ide-probe.c 1.90 vs edited =====

drivers/ide/ide-probe.c 2005-01-14 08:18:43 -05:00 renamed from drivers/ide/ide-probe.c 2004-12-10 14:12:14 -05:00 +4 -1
@@ -841,7 +841,10 @@
        if (fixup)
                fixup(hwif);
 
-       hwif_init(hwif);
+       if (!hwif_init(hwif)) {
+               printk("%s: Failed to initialize IDE interface\n", hwif->name);
+               return -1;
+       }

Could you fix hwif_init() to return meaningful error codes and return error from hwif_init() instead of '-1'?

drivers/ide/pci/sgiioc4.c 2005-01-14 08:18:56 -05:00 line 673

Quoted 12 lines
drivers/ide/ide-probe.c 2005-01-14 08:18:43 -05:00 renamed from drivers/ide/ide-probe.c 2004-12-10 14:12:14 -05:00 +0 -0
        if (hwif->present) {
                u16 unit = 0;

===== drivers/ide/pci/sgiioc4.c 1.22 vs edited =====

drivers/ide/pci/sgiioc4.c 2005-01-14 08:18:56 -05:00 renamed from drivers/ide/pci/sgiioc4.c 2005-01-06 20:35:35 -05:00 +2 -1
@@ -669,7 +669,10 @@
                printk(KERN_INFO "%s: %s Bus-Master DMA disabled\n",
                       hwif->name, d->name);
 
-       probe_hwif_init(hwif);
+       if (probe_hwif_init(hwif)) {
+               printk(KERN_INFO "%s: initialization failed\n", hwif->name);

redundant, error message is printed in probe_hwif_init()

drivers/ide/pci/sgiioc4.c 2005-01-14 08:18:56 -05:00 line 674

Quoted 1 line
drivers/ide/pci/sgiioc4.c 2005-01-14 08:18:56 -05:00 renamed from drivers/ide/pci/sgiioc4.c 2005-01-06 20:35:35 -05:00 +1 -0
+               return -EIO;

please fix probe_hwif_init() to return error codes

drivers/ide/pci/sgiioc4.c 2005-01-14 08:18:56 -05:00 line 678

Quoted 4 lines
drivers/ide/pci/sgiioc4.c 2005-01-14 08:18:56 -05:00 renamed from drivers/ide/pci/sgiioc4.c 2005-01-06 20:35:35 -05:00 +1 -0
+       }
 
        /* Create /proc/ide entries */
        create_proc_ide_interfaces();

I removed redundant printk and applied the patch

author
#2 lore raw

Thanks for the comments Bartlomiej -- your input is always appreciated.

Here's a new patch taking into account your comments. I have updated hwif_init to return 0 on success and -errno on failure.

Thanks again,

P.

Signed-off-by Prarit Bhargava <[email protected]>

===== ide-probe.c 1.90 vs edited =====

ide-probe.c renamed from drivers/ide/ide-probe.c +12 -7
@@ -841,7 +841,10 @@
 	if (fixup)
 		fixup(hwif);
 
-	hwif_init(hwif);
+	if (hwif_init(hwif) < 0) {
+		printk("%s: Failed to initialize IDE interface\n", hwif->name);
+		return -1;
+	}
 
 	if (hwif->present) {
 		u16 unit = 0;

@@ -1245,20 +1248,22 @@
 	int old_irq, unit;
 
 	if (!hwif->present)
-		return 0;
+		return -ENODEV;
 
 	if (!hwif->irq) {
 		if (!(hwif->irq = ide_default_irq(hwif->io_ports[IDE_DATA_OFFSET])))
 		{
 			printk("%s: DISABLED, NO IRQ\n", hwif->name);
-			return (hwif->present = 0);
+			hwif->present = 0;
+			return -EIO;
 		}
 	}
 #ifdef CONFIG_BLK_DEV_HD
 	if (hwif->irq == HD_IRQ && hwif->io_ports[IDE_DATA_OFFSET] != HD_DATA) {
 		printk("%s: CANNOT SHARE IRQ WITH OLD "
 			"HARDDISK DRIVER (hd.c)\n", hwif->name);
-		return (hwif->present = 0);
+		hwif->present = 0;
+		return -EIO;
 	}
 #endif /* CONFIG_BLK_DEV_HD */
 

@@ -1266,7 +1271,7 @@
 	hwif->present = 0;
 
 	if (register_blkdev(hwif->major, hwif->name))
-		return 0;
+		return -EIO;
 
 	if (!hwif->sg_max_nents)
 		hwif->sg_max_nents = PRD_ENTRIES;

@@ -1305,7 +1310,7 @@
 done:
 	init_gendisk(hwif);
 	hwif->present = 1;	/* success */
-	return 1;
+	return 0;
 
 out_disks:
 	for (unit = 0; unit < MAX_DRIVES; unit++) {

@@ -1315,7 +1320,7 @@
 	}
 out:
 	unregister_blkdev(hwif->major, hwif->name);
-	return 0;
+	return -EIO;
 }
 
 int ideprobe_init (void)

===== sgiioc4.c 1.22 vs edited =====

sgiioc4.c renamed from drivers/ide/pci/sgiioc4.c +2 -1
@@ -669,7 +669,8 @@
 		printk(KERN_INFO "%s: %s Bus-Master DMA disabled\n",
 		       hwif->name, d->name);
 
-	probe_hwif_init(hwif);
+	if (probe_hwif_init(hwif))
+		return -EIO;
 
 	/* Create /proc/ide entries */
 	create_proc_ide_interfaces();
author
#3 lore raw

Bartlomiej pointed out that the sgiioc4.c changes have made their way into Linus' tree.

Here's an updated change set.

Thanks,

P.

Signed-off-by Prarit Bhargava <[email protected]>

===== ide-probe.c 1.90 vs edited =====

ide-probe.c renamed from drivers/ide/ide-probe.c +12 -7
@@ -841,7 +841,10 @@
 	if (fixup)
 		fixup(hwif);
 
-	hwif_init(hwif);
+	if (hwif_init(hwif) < 0) {
+		printk("%s: Failed to initialize IDE interface\n", hwif->name);
+		return -1;
+	}
 
 	if (hwif->present) {
 		u16 unit = 0;

@@ -1245,20 +1248,22 @@
 	int old_irq, unit;
 
 	if (!hwif->present)
-		return 0;
+		return -ENODEV;
 
 	if (!hwif->irq) {
 		if (!(hwif->irq = ide_default_irq(hwif->io_ports[IDE_DATA_OFFSET])))
 		{
 			printk("%s: DISABLED, NO IRQ\n", hwif->name);
-			return (hwif->present = 0);
+			hwif->present = 0;
+			return -EIO;
 		}
 	}
 #ifdef CONFIG_BLK_DEV_HD
 	if (hwif->irq == HD_IRQ && hwif->io_ports[IDE_DATA_OFFSET] != HD_DATA) {
 		printk("%s: CANNOT SHARE IRQ WITH OLD "
 			"HARDDISK DRIVER (hd.c)\n", hwif->name);
-		return (hwif->present = 0);
+		hwif->present = 0;
+		return -EIO;
 	}
 #endif /* CONFIG_BLK_DEV_HD */
 

@@ -1266,7 +1271,7 @@
 	hwif->present = 0;
 
 	if (register_blkdev(hwif->major, hwif->name))
-		return 0;
+		return -EIO;
 
 	if (!hwif->sg_max_nents)
 		hwif->sg_max_nents = PRD_ENTRIES;

@@ -1305,7 +1310,7 @@
 done:
 	init_gendisk(hwif);
 	hwif->present = 1;	/* success */
-	return 1;
+	return 0;
 
 out_disks:
 	for (unit = 0; unit < MAX_DRIVES; unit++) {

@@ -1315,7 +1320,7 @@
 	}
 out:
 	unregister_blkdev(hwif->major, hwif->name);
-	return 0;
+	return -EIO;
 }
 
 int ideprobe_init (void)
#4 lore raw
Quoted 1 line · Prarit Bhargava

Bartlomiej pointed out that the sgiioc4.c changes have made their way

                                                ^^^^^^^^

all changes

Quoted 16 lines

into Linus' tree.

Here's an updated change set.

Thanks,

P.

Signed-off-by Prarit Bhargava <[email protected]>

===== ide-probe.c 1.90 vs edited =====

ide-probe.c 2005-02-08 10:41:46 -05:00 renamed from drivers/ide/ide-probe.c 2004-12-10 14:12:14 -05:00 +0 -0
@@ -841,7 +841,10 @@
        if (fixup)

ide-probe.c 2005-02-08 10:41:46 -05:00 line 850

Quoted 10 lines
ide-probe.c 2005-02-08 10:41:46 -05:00 renamed from drivers/ide/ide-probe.c 2004-12-10 14:12:14 -05:00 +4 -1
                fixup(hwif);
 
-       hwif_init(hwif);
+       if (hwif_init(hwif) < 0) {
+               printk("%s: Failed to initialize IDE interface\n", hwif->name);
+               return -1;
+       }
 
        if (hwif->present) {
                u16 unit = 0;

this one chunk was in merged patch already

ide-probe.c 2005-02-08 10:41:46 -05:00 line 1251

Quoted 6 lines
ide-probe.c 2005-02-08 10:41:46 -05:00 renamed from drivers/ide/ide-probe.c 2004-12-10 14:12:14 -05:00 +1 -1
@@ -1245,20 +1248,22 @@
        int old_irq, unit;
 
        if (!hwif->present)
-               return 0;
+               return -ENODEV;

this is wrong, there is return 1 (not 0) now because of fix needed for the previous patch (error message for interface with no drives)

No problem, I can hand-merge this patch but these changes need some description, "fix hwif_init() return codes" perhaps?

author
#5 lore raw

Bartlomiej,

I did a fresh check out of linux-2.6 and diff'd? That's strange ...

Anyhow, how about "Fixes to IDE hwif initialization error handling".

IMO it better captures what I'm trying to fix :)

P.

Quoted 6 lines

No problem, I can hand-merge this patch but these changes need some description, "fix hwif_init() return codes" perhaps?