Skip to contentSkip to conversation

gayle: reserve memory resources at once

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 3 messages Jun 22, 2008 – Jul 21, 2008 libata

lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

PATCH 1/1 gayle: reserve memory resources at once

* Reserve memory resources for all IDE ports at once by moving request_mem_region() out of 'for ()' loop and always defining GAYLE_IDEREG_SIZE to 0x2000.

* Keep memory resources even if no free IDE slots can be found (this driver is unloadable currently).

Cc Geert Uytterhoeven <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/legacy/gayle.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)
drivers/ide/legacy/gayle.c +11 -14
Index: b/drivers/ide/legacy/gayle.c
===================================================================
@@ -31,6 +31,8 @@
 #define GAYLE_BASE_4000	0xdd2020	/* A4000/A4000T */
 #define GAYLE_BASE_1200	0xda0000	/* A1200/A600 and E-Matrix 530 */
 
+#define GAYLE_IDEREG_SIZE	0x2000
+
     /*
      *  Offsets from one of the above bases
      */

@@ -56,13 +58,11 @@
 #define GAYLE_NUM_HWIFS		1
 #define GAYLE_NUM_PROBE_HWIFS	GAYLE_NUM_HWIFS
 #define GAYLE_HAS_CONTROL_REG	1
-#define GAYLE_IDEREG_SIZE	0x2000
 #else /* CONFIG_BLK_DEV_IDEDOUBLER */
 #define GAYLE_NUM_HWIFS		2
 #define GAYLE_NUM_PROBE_HWIFS	(ide_doubler ? GAYLE_NUM_HWIFS : \
 					       GAYLE_NUM_HWIFS-1)
 #define GAYLE_HAS_CONTROL_REG	(!ide_doubler)
-#define GAYLE_IDEREG_SIZE	(ide_doubler ? 0x1000 : 0x2000)
 
 static int ide_doubler;
 module_param_named(doubler, ide_doubler, bool, 0);

@@ -124,6 +124,9 @@ static void __init gayle_setup_ports(hw_
 
 static int __init gayle_init(void)
 {
+    unsigned long phys_base, res_start, res_n;
+    unsigned long base, ctrlport, irqport;
+    ide_ack_intr_t *ack_intr;
     int a4000, i;
     hw_regs_t hw[GAYLE_NUM_HWIFS], *hws[] = { NULL, NULL, NULL, NULL };
     u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };

@@ -149,12 +152,6 @@ found:
 #endif
 			 "");
 
-    for (i = 0; i < GAYLE_NUM_PROBE_HWIFS; i++) {
-	unsigned long base, ctrlport, irqport;
-	ide_ack_intr_t *ack_intr;
-	ide_hwif_t *hwif;
-	unsigned long phys_base, res_start, res_n;
-
 	if (a4000) {
 	    phys_base = GAYLE_BASE_4000;
 	    irqport = (unsigned long)ZTWO_VADDR(GAYLE_IRQ_4000);

@@ -168,15 +165,16 @@ found:
  * FIXME: we now have selectable modes between mmio v/s iomio
  */
 
-	phys_base += i*GAYLE_NEXT_PORT;
-
 	res_start = ((unsigned long)phys_base) & ~(GAYLE_NEXT_PORT-1);
 	res_n = GAYLE_IDEREG_SIZE;
 
 	if (!request_mem_region(res_start, res_n, "IDE"))
-	    continue;
+		return -EBUSY;
 
-	base = (unsigned long)ZTWO_VADDR(phys_base);
+    for (i = 0; i < GAYLE_NUM_PROBE_HWIFS; i++) {
+	ide_hwif_t *hwif;
+
+	base = (unsigned long)ZTWO_VADDR(phys_base + i * GAYLE_NEXT_PORT);
 	ctrlport = GAYLE_HAS_CONTROL_REG ? (base + GAYLE_CONTROL) : 0;
 
 	gayle_setup_ports(&hw[i], base, ctrlport, irqport, ack_intr);

@@ -187,8 +185,7 @@ found:
 
 	    hws[i] = &hw[i];
 	    idx[i] = hwif->index;
-	} else
-	    release_mem_region(res_start, res_n);
+	}
     }
 
     ide_device_add(idx, NULL, hws);
#1 lore raw
Quoted 6 lines · Bartlomiej Zolnierkiewicz

* Reserve memory resources for all IDE ports at once by moving request_mem_region() out of 'for ()' loop and always defining GAYLE_IDEREG_SIZE to 0x2000.

* Keep memory resources even if no free IDE slots can be found (this driver is unloadable currently).

I don't like this part. You're changing the driver to no longer correctly handle initialization failures.

Quoted 79 lines
Cc Geert Uytterhoeven <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/legacy/gayle.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Index: b/drivers/ide/legacy/gayle.c
===================================================================

drivers/ide/legacy/gayle.c +10 -12
@@ -31,6 +31,8 @@
 #define GAYLE_BASE_4000	0xdd2020	/* A4000/A4000T */
 #define GAYLE_BASE_1200	0xda0000	/* A1200/A600 and E-Matrix 530 */
 
+#define GAYLE_IDEREG_SIZE	0x2000
+
     /*
      *  Offsets from one of the above bases
      */

@@ -56,13 +58,11 @@
 #define GAYLE_NUM_HWIFS		1
 #define GAYLE_NUM_PROBE_HWIFS	GAYLE_NUM_HWIFS
 #define GAYLE_HAS_CONTROL_REG	1
-#define GAYLE_IDEREG_SIZE	0x2000
 #else /* CONFIG_BLK_DEV_IDEDOUBLER */
 #define GAYLE_NUM_HWIFS		2
 #define GAYLE_NUM_PROBE_HWIFS	(ide_doubler ? GAYLE_NUM_HWIFS : \
 					       GAYLE_NUM_HWIFS-1)
 #define GAYLE_HAS_CONTROL_REG	(!ide_doubler)
-#define GAYLE_IDEREG_SIZE	(ide_doubler ? 0x1000 : 0x2000)
 
 static int ide_doubler;
 module_param_named(doubler, ide_doubler, bool, 0);

@@ -124,6 +124,9 @@ static void __init gayle_setup_ports(hw_
 
 static int __init gayle_init(void)
 {
+    unsigned long phys_base, res_start, res_n;
+    unsigned long base, ctrlport, irqport;
+    ide_ack_intr_t *ack_intr;
     int a4000, i;
     hw_regs_t hw[GAYLE_NUM_HWIFS], *hws[] = { NULL, NULL, NULL, NULL };
     u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };

@@ -149,12 +152,6 @@ found:
 #endif
 			 "");
 
-    for (i = 0; i < GAYLE_NUM_PROBE_HWIFS; i++) {
-	unsigned long base, ctrlport, irqport;
-	ide_ack_intr_t *ack_intr;
-	ide_hwif_t *hwif;
-	unsigned long phys_base, res_start, res_n;
-
 	if (a4000) {
 	    phys_base = GAYLE_BASE_4000;
 	    irqport = (unsigned long)ZTWO_VADDR(GAYLE_IRQ_4000);

@@ -168,15 +165,16 @@ found:
  * FIXME: we now have selectable modes between mmio v/s iomio
  */
 
-	phys_base += i*GAYLE_NEXT_PORT;
-
 	res_start = ((unsigned long)phys_base) & ~(GAYLE_NEXT_PORT-1);
 	res_n = GAYLE_IDEREG_SIZE;
 
 	if (!request_mem_region(res_start, res_n, "IDE"))
-	    continue;
+		return -EBUSY;
 
-	base = (unsigned long)ZTWO_VADDR(phys_base);
+    for (i = 0; i < GAYLE_NUM_PROBE_HWIFS; i++) {
+	ide_hwif_t *hwif;
+
+	base = (unsigned long)ZTWO_VADDR(phys_base + i * GAYLE_NEXT_PORT);
 	ctrlport = GAYLE_HAS_CONTROL_REG ? (base + GAYLE_CONTROL) : 0;
 
 	gayle_setup_ports(&hw[i], base, ctrlport, irqport, ack_intr);

drivers/ide/legacy/gayle.c found: · line 191

Quoted 9 lines
drivers/ide/legacy/gayle.c +1 -2
@@ -187,8 +185,7 @@ found:
 
 	    hws[i] = &hw[i];
 	    idx[i] = hwif->index;
-	} else
-	    release_mem_region(res_start, res_n);
+	}
     }
 
     ide_device_add(idx, NULL, hws);

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

author
#2 lore raw
Quoted 10 lines · Geert Uytterhoeven
Quoted 6 lines · Bartlomiej Zolnierkiewicz

* Reserve memory resources for all IDE ports at once by moving request_mem_region() out of 'for ()' loop and always defining GAYLE_IDEREG_SIZE to 0x2000.

* Keep memory resources even if no free IDE slots can be found (this driver is unloadable currently).

I don't like this part. You're changing the driver to no longer correctly handle initialization failures.

Yeah, I don't like it either. Unfortunately I don't have a better idea of how to convert this driver to ide_host_add() without this step.

Anyway, this was meant to be temporary and I planned to fix it later after ide_host_add() is in place but actually I forgot about it...

Fix below.

BTW core code was fixed to allow module removal so it would be great if you (or somebody else fluent in zorro drivers) could take a look into converting gayle.c and buddha.c to use new-style zorro probing (struct zorro_driver & friends) so we can fix these drivers to use ->remove method + zorro_unregister_driver() for module_exit().

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] gayle: release resources on ide_host_add() failure

"gayle: reserve memory resources at once" patch temporary removed freeing of resources on failure (to ease convertion to ide_host_add() interface). This patch fixes it.

Thanks to Geert for noticing the issue.

Noticed-by Geert Uytterhoeven <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
goes on top of pata tree

 drivers/ide/legacy/gayle.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/ide/legacy/gayle.c +6 -2
Index: b/drivers/ide/legacy/gayle.c
===================================================================
@@ -127,7 +127,7 @@ static int __init gayle_init(void)
     unsigned long phys_base, res_start, res_n;
     unsigned long base, ctrlport, irqport;
     ide_ack_intr_t *ack_intr;
-    int a4000, i;
+    int a4000, i, rc;
     hw_regs_t hw[GAYLE_NUM_HWIFS], *hws[] = { NULL, NULL, NULL, NULL };
 
     if (!MACH_IS_AMIGA)

@@ -179,7 +179,11 @@ found:
 	hws[i] = &hw[i];
     }
 
-    return ide_host_add(NULL, hws, NULL);
+    rc = ide_host_add(NULL, hws, NULL);
+    if (rc)
+	release_mem_region(res_start, res_n);
+
+    return rc;
 }
 
 module_init(gayle_init);