Re: [PATCHv2 1/2] ALSA: fix emu8000 DRAM sizing for AWE64 Value

[Takashi Iwai <tiwai@suse.de>]

At Fri, 09 Jan 2015 19:50:36 -0500,
David Flater wrote:
> 
> Applicable to any kernel since 2013:
> 
> The special case added in commit 1338fc97d07a did not handle the possibility
> that the address space on an AWE64 Value would wrap around at 512 KiB.  That
> is what it does, so the memory is still not detected on those cards.
> 
> Fix that with a logic clean-up that eliminates the need for a special case.
> 
> Signed-off-by: David Flater <dave@flaterco.com>
> ---
> History:
> 2015-01-09  v2: In response to feedback from Takashi Iwai,
>                1. Optimize for diff size.
>                  1a. Use goto to avoid indenting and repeating code.
>                  1b. Jettison new debugging printouts.
>                2. Split printk into second patch.
>             Retested on CT4390 (4 MiB) and CT4380 (512 KiB).
> 2015-01-08  v1 patch sent to LKML, Alsa Devel and maintainers.  Tested on
>             unexpanded CT4390 (4 MiB), CT4520 (512 KiB), and CT4380 (512
>             KiB).
> 
> CT4380 is commonly said to come with 1 MiB of DRAM, but Creative's AWE
> Control app agreed that mine has only 512 KiB.  It has the same memory chip
> as the CT4520.

And you couldn't actually use the 1MB, right?


> The affected function first appeared in alsa-driver-0.3.0 and was merged in
> linux-2.5.5.  Its somewhat different ancestor was in sound/oss/awe_wave.c.
> 
>  sound/isa/sb/emu8000.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/isa/sb/emu8000.c b/sound/isa/sb/emu8000.c
> index 45fcdff..3dcf80e 100644
> --- a/sound/isa/sb/emu8000.c
> +++ b/sound/isa/sb/emu8000.c
> @@ -378,13 +378,12 @@ init_arrays(struct snd_emu8000 *emu)
>  static void
>  size_dram(struct snd_emu8000 *emu)
>  {
> -	int i, size, detected_size;
> +	int i, size;
>  
>  	if (emu->dram_checked)
>  		return;
>  
>  	size = 0;
> -	detected_size = 0;
>  
>  	/* write out a magic number */
>  	snd_emu8000_dma_chan(emu, 0, EMU8000_RAM_WRITE);
> @@ -392,11 +391,12 @@ size_dram(struct snd_emu8000 *emu)
>  	EMU8000_SMALW_WRITE(emu, EMU8000_DRAM_OFFSET);
>  	EMU8000_SMLD_WRITE(emu, UNIQUE_ID1);
>  	snd_emu8000_init_fm(emu); /* This must really be here and not 2 lines back even */
> +	snd_emu8000_write_wait(emu);
> +
> +	goto size_dram_ID1_check; /* If that fails, we have no RAM. */

Jumping into the middle of the loop isn't good.  It worsens the
readability a lot.  OTOH, jumping to the exit of the loop is a
standard idiom like the patch below.


thanks,

Takashi

diff --git a/sound/isa/sb/emu8000.c b/sound/isa/sb/emu8000.c
index 45fcdff611f9..6e721a8b7c9e 100644
--- a/sound/isa/sb/emu8000.c
+++ b/sound/isa/sb/emu8000.c
@@ -378,13 +378,12 @@ init_arrays(struct snd_emu8000 *emu)
 static void
 size_dram(struct snd_emu8000 *emu)
 {
-	int i, size, detected_size;
+	int i, size;
 
 	if (emu->dram_checked)
 		return;
 
 	size = 0;
-	detected_size = 0;
 
 	/* write out a magic number */
 	snd_emu8000_dma_chan(emu, 0, EMU8000_RAM_WRITE);
@@ -393,9 +392,14 @@ size_dram(struct snd_emu8000 *emu)
 	EMU8000_SMLD_WRITE(emu, UNIQUE_ID1);
 	snd_emu8000_init_fm(emu); /* This must really be here and not 2 lines back even */
 
-	while (size < EMU8000_MAX_DRAM) {
+	/* Detect first 512 KiB */
+	snd_emu8000_write_wait(emu);
+	EMU8000_SMALR_WRITE(emu, EMU8000_DRAM_OFFSET);
+	EMU8000_SMLD_READ(emu); /* discard stale data  */
+	if (EMU8000_SMLD_READ(emu) == UNIQUE_ID1)
+		goto skip_detect;
 
-		size += 512 * 1024;  /* increment 512kbytes */
+	for (size = 512 * 1024; size < EMU8000_MAX_DRAM; size += 512 * 1024) {
 
 		/* Write a unique data on the test address.
 		 * if the address is out of range, the data is written on
@@ -429,20 +433,9 @@ size_dram(struct snd_emu8000 *emu)
 		if (EMU8000_SMLD_READ(emu) != UNIQUE_ID1)
 			break; /* we must have wrapped around */
 		snd_emu8000_read_wait(emu);
-
-		/* Otherwise, it's valid memory. */
-		detected_size = size + 512 * 1024;
-	}
-
-	/* Distinguish 512 KiB from 0. */
-	if (detected_size == 0) {
-		snd_emu8000_read_wait(emu);
-		EMU8000_SMALR_WRITE(emu, EMU8000_DRAM_OFFSET);
-		EMU8000_SMLD_READ(emu); /* discard stale data  */
-		if (EMU8000_SMLD_READ(emu) == UNIQUE_ID1)
-			detected_size = 512 * 1024;
 	}
 
+ skip_detect:
 	/* wait until FULL bit in SMAxW register is false */
 	for (i = 0; i < 10000; i++) {
 		if ((EMU8000_SMALW_READ(emu) & 0x80000000) == 0)
@@ -455,9 +448,9 @@ size_dram(struct snd_emu8000 *emu)
 	snd_emu8000_dma_chan(emu, 1, EMU8000_RAM_CLOSE);
 
 	snd_printdd("EMU8000 [0x%lx]: %d Kb on-board memory detected\n",
-		    emu->port1, detected_size/1024);
+		    emu->port1, size / 1024);
 
-	emu->mem_size = detected_size;
+	emu->mem_size = size;
 	emu->dram_checked = 1;
 }