[PATCH] gayle: reserve memory resources at once

[PATCH] gayle: reserve memory resources at once

[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).

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/legacy/gayle.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Index: b/drivers/ide/legacy/gayle.c
===================================================================
--- a/drivers/ide/legacy/gayle.c
+++ 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);

Re: [PATCH] gayle: reserve memory resources at once

[Geert Uytterhoeven]

On Sun, 22 Jun 2008, Bartlomiej Zolnierkiewicz wrote:
> * 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.

> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/legacy/gayle.c |   25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> Index: b/drivers/ide/legacy/gayle.c
> ===================================================================
> --- a/drivers/ide/legacy/gayle.c
> +++ 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);
> 

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] gayle: reserve memory resources at once

[Bartlomiej Zolnierkiewicz]

On Saturday 19 July 2008, Geert Uytterhoeven wrote:
> On Sun, 22 Jun 2008, Bartlomiej Zolnierkiewicz wrote:
> > * 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 <bzolnier@gmail.com>
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 <geert@linux-m68k.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
goes on top of pata tree

 drivers/ide/legacy/gayle.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: b/drivers/ide/legacy/gayle.c
===================================================================
--- a/drivers/ide/legacy/gayle.c
+++ 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);