[PATCH v2 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[PATCH v2 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Oswald Buddenhagen]

---
This patch series needs to be applied on top of the patch titled
"Revert "ALSA: emu10k1: fix synthesizer sample playback position and
caching"".

Oswald Buddenhagen (17):
  ALSA: emux: fix /proc teardown at module unload
  ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch()
  ALSA: emux: fix validation of snd_emux.num_ports
  ALSA: emux: fix init of patch_info.truesize in load_data()
  ALSA: emu10k1: prune vestiges of
    SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support
  ALSA: emux: centralize & improve patch info validation
  ALSA: emux: improve patch ioctl data validation
  ALSA: emu10k1: move patch loader assertions into low-level functions
  ALSA: emu10k1: fix sample signedness issues in wavetable loader
  ALSA: emu10k1: fix playback of 8-bit wavetable samples
  ALSA: emu10k1: merge conditions in patch loader
  ALSA: emu10k1: fix wavetable offset recalculation
  ALSA: emu10k1: de-duplicate size calculations for 16-bit samples
  ALSA: emu10k1: improve cache behavior documentation
  ALSA: emu10k1: fix wavetable playback position and caching, take 2
  ALSA: emu10k1: shrink blank space in front of wavetable samples
  ALSA: emux: simplify snd_sf_list.callback handling

 include/sound/emu10k1.h              |  32 +++--
 include/sound/soundfont.h            |   2 +-
 sound/isa/sb/emu8000_patch.c         |  13 --
 sound/pci/emu10k1/emu10k1_callback.c |  13 +-
 sound/pci/emu10k1/emu10k1_patch.c    | 207 +++++++++++----------------
 sound/pci/emu10k1/memory.c           |  55 +++++--
 sound/synth/emux/emux.c              |   6 +-
 sound/synth/emux/emux_hwdep.c        |   3 +-
 sound/synth/emux/emux_oss.c          |   3 +-
 sound/synth/emux/emux_proc.c         |   1 +
 sound/synth/emux/emux_seq.c          |   6 +-
 sound/synth/emux/soundfont.c         |  73 +++++++---
 12 files changed, 219 insertions(+), 195 deletions(-)


base-commit: ed93395844979f6bf2e1fbfcda38d1718289b426
prerequisite-patch-id: b7769e1c8649b86fd9e0b259e11bfd8e468393e5
--
2.42.0.419.g70bf8a5751

[PATCH v2 01/17] ALSA: emux: fix /proc teardown at module unload

[Oswald Buddenhagen]

We forgot to remember the wavetable /proc entry, so we'd fail to free it
at module unload.

This matters only when only the synth module is unloaded, as unloading
the card driver would tear down the sub-entry anyway.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/emux_proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/synth/emux/emux_proc.c b/sound/synth/emux/emux_proc.c
index 7993e6a01e54..820351f52551 100644
--- a/sound/synth/emux/emux_proc.c
+++ b/sound/synth/emux/emux_proc.c
@@ -102,6 +102,7 @@ void snd_emux_proc_init(struct snd_emux *emu, struct snd_card *card, int device)
 	entry->content = SNDRV_INFO_CONTENT_TEXT;
 	entry->private_data = emu;
 	entry->c.text.read = snd_emux_proc_info_read;
+	emu->proc = entry;
 }

 void snd_emux_proc_free(struct snd_emux *emu)
--
2.42.0.419.g70bf8a5751

[PATCH v2 02/17] ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch()

[Oswald Buddenhagen]

The `client` parameter was not used, so eliminate it from the call
chain.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/soundfont.h     | 2 +-
 sound/synth/emux/emux_hwdep.c | 3 +--
 sound/synth/emux/emux_oss.c   | 3 +--
 sound/synth/emux/soundfont.c  | 7 +++----
 4 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/sound/soundfont.h b/include/sound/soundfont.h
index e445688a4f4f..98ed98d89d6d 100644
--- a/include/sound/soundfont.h
+++ b/include/sound/soundfont.h
@@ -89,7 +89,7 @@ struct snd_sf_list {
 int snd_soundfont_load(struct snd_sf_list *sflist, const void __user *data,
 		       long count, int client);
 int snd_soundfont_load_guspatch(struct snd_sf_list *sflist, const char __user *data,
-				long count, int client);
+				long count);
 int snd_soundfont_close_check(struct snd_sf_list *sflist, int client);

 struct snd_sf_list *snd_sf_new(struct snd_sf_callback *callback,
diff --git a/sound/synth/emux/emux_hwdep.c b/sound/synth/emux/emux_hwdep.c
index 81719bfb8ed7..fd8f978cde1c 100644
--- a/sound/synth/emux/emux_hwdep.c
+++ b/sound/synth/emux/emux_hwdep.c
@@ -27,8 +27,7 @@ snd_emux_hwdep_load_patch(struct snd_emux *emu, void __user *arg)

 	if (patch.key == GUS_PATCH)
 		return snd_soundfont_load_guspatch(emu->sflist, arg,
-						   patch.len + sizeof(patch),
-						   TMP_CLIENT_ID);
+						   patch.len + sizeof(patch));

 	if (patch.type >= SNDRV_SFNT_LOAD_INFO &&
 	    patch.type <= SNDRV_SFNT_PROBE_DATA) {
diff --git a/sound/synth/emux/emux_oss.c b/sound/synth/emux/emux_oss.c
index d8d32671f703..04df46b269d3 100644
--- a/sound/synth/emux/emux_oss.c
+++ b/sound/synth/emux/emux_oss.c
@@ -205,8 +205,7 @@ snd_emux_load_patch_seq_oss(struct snd_seq_oss_arg *arg, int format,
 		return -ENXIO;

 	if (format == GUS_PATCH)
-		rc = snd_soundfont_load_guspatch(emu->sflist, buf, count,
-						 SF_CLIENT_NO(p->chset.port));
+		rc = snd_soundfont_load_guspatch(emu->sflist, buf, count);
 	else if (format == SNDRV_OSS_SOUNDFONT_PATCH) {
 		struct soundfont_patch_info patch;
 		if (count < (int)sizeof(patch))
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index 16f00097cb95..e1e47518ac92 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -941,8 +941,7 @@ int snd_sf_vol_table[128] = {

 /* load GUS patch */
 static int
-load_guspatch(struct snd_sf_list *sflist, const char __user *data,
-	      long count, int client)
+load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 {
 	struct patch_info patch;
 	struct snd_soundfont *sf;
@@ -1122,11 +1121,11 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data,
 /* load GUS patch */
 int
 snd_soundfont_load_guspatch(struct snd_sf_list *sflist, const char __user *data,
-			    long count, int client)
+			    long count)
 {
 	int rc;
 	lock_preset(sflist);
-	rc = load_guspatch(sflist, data, count, client);
+	rc = load_guspatch(sflist, data, count);
 	unlock_preset(sflist);
 	return rc;
 }
--
2.42.0.419.g70bf8a5751

[PATCH v2 03/17] ALSA: emux: fix validation of snd_emux.num_ports

[Oswald Buddenhagen]

Both bounds had off-by-one errors.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/emux_seq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/synth/emux/emux_seq.c b/sound/synth/emux/emux_seq.c
index b227c7e0bc2a..1adaa75df2f6 100644
--- a/sound/synth/emux/emux_seq.c
+++ b/sound/synth/emux/emux_seq.c
@@ -65,11 +65,11 @@ snd_emux_init_seq(struct snd_emux *emu, struct snd_card *card, int index)
 		return -ENODEV;
 	}

-	if (emu->num_ports < 0) {
+	if (emu->num_ports <= 0) {
 		snd_printk(KERN_WARNING "seqports must be greater than zero\n");
 		emu->num_ports = 1;
-	} else if (emu->num_ports >= SNDRV_EMUX_MAX_PORTS) {
-		snd_printk(KERN_WARNING "too many ports."
+	} else if (emu->num_ports > SNDRV_EMUX_MAX_PORTS) {
+		snd_printk(KERN_WARNING "too many ports. "
 			   "limited max. ports %d\n", SNDRV_EMUX_MAX_PORTS);
 		emu->num_ports = SNDRV_EMUX_MAX_PORTS;
 	}
--
2.42.0.419.g70bf8a5751

[PATCH v2 04/17] ALSA: emux: fix init of patch_info.truesize in load_data()

[Oswald Buddenhagen]

The field is explicitly documented to be initialized by the driver
(which it actually is). Also, using patch_info.size would be actually
wrong for 16-bit data, as one field counts samples, while the other
counts bytes.

load_guspatch() already did it right.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/soundfont.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index e1e47518ac92..ad0231d7a39d 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -735,7 +735,7 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 	sp->v = sample_info;
 	sp->v.sf_id = sf->id;
 	sp->v.dummy = 0;
-	sp->v.truesize = sp->v.size;
+	sp->v.truesize = 0;

 	/*
 	 * If there is wave data then load it.
--
2.42.0.419.g70bf8a5751

[PATCH v2 05/17] ALSA: emu10k1: prune vestiges of SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support

[Oswald Buddenhagen]

This is required only to implement WAVE_BIDIR_LOOP and WAVE_LOOP_BACK in
the GUS patch loader. It has not worked on emu10k1 since before ALSA hit
mainline, yet nobody appears to have complained. And as it isn't super
easy to implement, just admit defeat and clean up the code.

If somebody wanted to resurrect the feature, the emu8k driver could
serve as a template, but the code would be quite different. But
arguably, this should be done in user space in the first place, as this
doesn't represent a hardware feature (somewhat ironically, the actual
GUS driver has no synth support, and therefore no GUS patch loader).

Note that instead of properly rejecting affected samples, we continue to
just pretend that the feature wasn't requested. This is extremely
questionable behavior, but avoids that possibly unused instruments
suddenly prevent loading the entire file, which would break backwards
compatibility. But at least we log a warning now.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 73 ++++---------------------------
 1 file changed, 8 insertions(+), 65 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 89890f24509f..49214c226808 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -28,8 +28,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 {
 	int offset;
 	int truesize, size, blocksize;
-	__maybe_unused int loopsize;
-	int loopend, sampleend;
 	unsigned int start_addr;
 	struct snd_emu10k1 *emu;

@@ -43,32 +41,24 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		return 0;
 	}

+	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP | SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) {
+		/* should instead return -ENOTSUPP; but compatibility */
+		printk(KERN_WARNING "Emu10k1 wavetable patch %d with unsupported loop feature\n",
+		       sp->v.sample);
+	}
+
 	/* recalculate address offset */
 	sp->v.end -= sp->v.start;
 	sp->v.loopstart -= sp->v.start;
 	sp->v.loopend -= sp->v.start;
 	sp->v.start = 0;

-	/* some samples have invalid data.  the addresses are corrected in voice info */
-	sampleend = sp->v.end;
-	if (sampleend > sp->v.size)
-		sampleend = sp->v.size;
-	loopend = sp->v.loopend;
-	if (loopend > sampleend)
-		loopend = sampleend;
-
 	/* be sure loop points start < end */
 	if (sp->v.loopstart >= sp->v.loopend)
 		swap(sp->v.loopstart, sp->v.loopend);

 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
-	loopsize = 0;
-#if 0 /* not supported */
-	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP))
-		loopsize = sp->v.loopend - sp->v.loopstart;
-	truesize += loopsize;
-#endif
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
 		truesize += BLANK_LOOP_SIZE;

@@ -96,65 +86,18 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	snd_emu10k1_synth_bzero(emu, sp->block, offset, size);
 	offset += size;

-	/* copy start->loopend */
-	size = loopend;
+	/* copy provided samples */
+	size = sp->v.size;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
 	if (offset + size > blocksize)
 		return -EINVAL;
 	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
 		return -EFAULT;
 	}
 	offset += size;
-	data += size;
-
-#if 0 /* not supported yet */
-	/* handle reverse (or bidirectional) loop */
-	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) {
-		/* copy loop in reverse */
-		if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) {
-			int woffset;
-			unsigned short *wblock = (unsigned short*)block;
-			woffset = offset / 2;
-			if (offset + loopsize * 2 > blocksize)
-				return -EINVAL;
-			for (i = 0; i < loopsize; i++)
-				wblock[woffset + i] = wblock[woffset - i -1];
-			offset += loopsize * 2;
-		} else {
-			if (offset + loopsize > blocksize)
-				return -EINVAL;
-			for (i = 0; i < loopsize; i++)
-				block[offset + i] = block[offset - i -1];
-			offset += loopsize;
-		}
-
-		/* modify loop pointers */
-		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_BIDIR_LOOP) {
-			sp->v.loopend += loopsize;
-		} else {
-			sp->v.loopstart += loopsize;
-			sp->v.loopend += loopsize;
-		}
-		/* add sample pointer */
-		sp->v.end += loopsize;
-	}
-#endif
-
-	/* loopend -> sample end */
-	size = sp->v.size - loopend;
-	if (size < 0)
-		return -EINVAL;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		size *= 2;
-	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
-		snd_emu10k1_synth_free(emu, sp->block);
-		sp->block = NULL;
-		return -EFAULT;
-	}
-	offset += size;

 	/* clear rest of samples (if any) */
 	if (offset < blocksize)
--
2.42.0.419.g70bf8a5751

[PATCH v2 06/17] ALSA: emux: centralize & improve patch info validation

[Oswald Buddenhagen]

This does several closely related things:
- Move the code from the drivers into the SoundFont loader, which
  de-duplicates it.
- Sort of explain the weird "recalculate address offset" feature. Note
  that I don't think it actually makes any sense - the calling user
  space code should do that. The background is certainly that the source
  data (the SoundFont format) uses pointers into a single wave block
  (and the API allows doing the same for on-board ROM), but the API
  expects the wave data from user space to be pre-chopped into
  individual patches anyway.
- Make sure that the specified offsets actually lie within the supplied
  wave data. Note that we don't validate ROM offsets, so one can play
  back anything within the sound card's address space.
- In load_guspatch(), don't call the sample_new callback anymore when
  the patch size is zero, as was already the case in load_data(). The
  callbacks would instantly return in that case anyway; these checks are
  now removed.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/isa/sb/emu8000_patch.c      | 13 -----------
 sound/pci/emu10k1/emu10k1_patch.c | 16 -------------
 sound/synth/emux/soundfont.c      | 37 ++++++++++++++++++++++++++++++-
 3 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/sound/isa/sb/emu8000_patch.c b/sound/isa/sb/emu8000_patch.c
index 8c1e7f2bfc34..ab4f988f080d 100644
--- a/sound/isa/sb/emu8000_patch.c
+++ b/sound/isa/sb/emu8000_patch.c
@@ -148,13 +148,6 @@ snd_emu8000_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (snd_BUG_ON(!sp))
 		return -EINVAL;

-	if (sp->v.size == 0)
-		return 0;
-
-	/* be sure loop points start < end */
-	if (sp->v.loopstart > sp->v.loopend)
-		swap(sp->v.loopstart, sp->v.loopend);
-
 	/* compute true data size to be loaded */
 	truesize = sp->v.size;
 	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP))
@@ -177,12 +170,6 @@ snd_emu8000_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 			return -EFAULT;
 	}

-	/* recalculate address offset */
-	sp->v.end -= sp->v.start;
-	sp->v.loopstart -= sp->v.start;
-	sp->v.loopend -= sp->v.start;
-	sp->v.start = 0;
-
 	/* dram position (in word) -- mem_offset is byte */
 	dram_offset = EMU8000_DRAM_OFFSET + (sp->block->offset >> 1);
 	dram_start = dram_offset;
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 49214c226808..47d69a0e44bc 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -35,28 +35,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (snd_BUG_ON(!sp || !hdr))
 		return -EINVAL;

-	if (sp->v.size == 0) {
-		dev_dbg(emu->card->dev,
-			"emu: rom font for sample %d\n", sp->v.sample);
-		return 0;
-	}
-
 	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP | SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) {
 		/* should instead return -ENOTSUPP; but compatibility */
 		printk(KERN_WARNING "Emu10k1 wavetable patch %d with unsupported loop feature\n",
 		       sp->v.sample);
 	}

-	/* recalculate address offset */
-	sp->v.end -= sp->v.start;
-	sp->v.loopstart -= sp->v.start;
-	sp->v.loopend -= sp->v.start;
-	sp->v.start = 0;
-
-	/* be sure loop points start < end */
-	if (sp->v.loopstart >= sp->v.loopend)
-		swap(sp->v.loopstart, sp->v.loopend);
-
 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index ad0231d7a39d..6d6f0102ed5b 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -689,6 +689,21 @@ find_sample(struct snd_soundfont *sf, int sample_id)
 }


+static int
+validate_sample_info(struct soundfont_sample_info *si)
+{
+	if (si->end < 0 || si->end > si->size)
+		return -EINVAL;
+	if (si->loopstart < 0 || si->loopstart > si->end)
+		return -EINVAL;
+	if (si->loopend < 0 || si->loopend > si->end)
+		return -EINVAL;
+	/* be sure loop points start < end */
+	if (si->loopstart > si->loopend)
+		swap(si->loopstart, si->loopend);
+	return 0;
+}
+
 /*
  * Load sample information, this can include data to be loaded onto
  * the soundcard.  It can also just be a pointer into soundcard ROM.
@@ -727,6 +742,21 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 		return -EINVAL;
 	}

+	if (sample_info.size > 0) {
+		if (sample_info.start < 0)
+			return -EINVAL;
+
+		// Here we "rebase out" the start address, because the
+		// real start is the start of the provided sample data.
+		sample_info.end -= sample_info.start;
+		sample_info.loopstart -= sample_info.start;
+		sample_info.loopend -= sample_info.start;
+		sample_info.start = 0;
+
+		if (validate_sample_info(&sample_info) < 0)
+			return -EINVAL;
+	}
+
 	/* Allocate a new sample structure */
 	sp = sf_sample_new(sflist, sf);
 	if (!sp)
@@ -974,6 +1004,11 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	smp->v.loopend = patch.loop_end;
 	smp->v.size = patch.len;

+	if (validate_sample_info(&smp->v) < 0) {
+		sf_sample_delete(sflist, sf, smp);
+		return -EINVAL;
+	}
+
 	/* set up mode flags */
 	smp->v.mode_flags = 0;
 	if (!(patch.mode & WAVE_16_BITS))
@@ -1011,7 +1046,7 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	/*
 	 * load wave data
 	 */
-	if (sflist->callback.sample_new) {
+	if (smp->v.size > 0 && sflist->callback.sample_new) {
 		rc = sflist->callback.sample_new
 			(sflist->callback.private_data, smp, sflist->memhdr,
 			 data, count);
--
2.42.0.419.g70bf8a5751

[PATCH v2 07/17] ALSA: emux: improve patch ioctl data validation

[Oswald Buddenhagen]

In load_data(), make the validation of and skipping over the main info
block match that in load_guspatch().

In load_guspatch(), add checking that the specified patch length matches
the actually supplied data, like load_data() already did.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/soundfont.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index 6d6f0102ed5b..4edc693da8e7 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -716,22 +716,25 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 	struct snd_soundfont *sf;
 	struct soundfont_sample_info sample_info;
 	struct snd_sf_sample *sp;
-	long off;

 	/* patch must be opened */
 	sf = sflist->currsf;
 	if (!sf)
 		return -EINVAL;

 	if (is_special_type(sf->type))
 		return -EINVAL;

+	if (count < (long)sizeof(sample_info)) {
+		return -EINVAL;
+	}
 	if (copy_from_user(&sample_info, data, sizeof(sample_info)))
 		return -EFAULT;
+	data += sizeof(sample_info);
+	count -= sizeof(sample_info);

-	off = sizeof(sample_info);
-
-	if (sample_info.size != (count-off)/2)
+	// SoundFont uses S16LE samples.
+	if (sample_info.size * 2 != count)
 		return -EINVAL;

 	/* Check for dup */
@@ -774,7 +777,7 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 		int  rc;
 		rc = sflist->callback.sample_new
 			(sflist->callback.private_data, sp, sflist->memhdr,
-			 data + off, count - off);
+			 data, count);
 		if (rc < 0) {
 			sf_sample_delete(sflist, sf, sp);
 			return rc;
@@ -986,10 +989,12 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	}
 	if (copy_from_user(&patch, data, sizeof(patch)))
 		return -EFAULT;
-
 	count -= sizeof(patch);
 	data += sizeof(patch);

+	if ((patch.len << (patch.mode & WAVE_16_BITS ? 1 : 0)) != count)
+		return -EINVAL;
+
 	sf = newsf(sflist, SNDRV_SFNT_PAT_TYPE_GUS|SNDRV_SFNT_PAT_SHARED, NULL);
 	if (sf == NULL)
 		return -ENOMEM;
--
2.42.0.419.g70bf8a5751

[PATCH v2 08/17] ALSA: emu10k1: move patch loader assertions into low-level functions

[Oswald Buddenhagen]

Convert some checks in snd_emu10k1_sample_new() back into assertions (as
they were prior to da3cec35dd (ALSA: Kill snd_assert() in sound/pci/*,
2008-08-08)), and move them into the low-level memory access functions
they protect.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---

Side note: this eliminates the memory leaks in the now gone error paths.
I don't think it was actually possible to trigger these even before the
foregoing cleanups. But if it were, it would allow a user with access to
the audio device a scope-limited DoS attack on it. This would be only a
very minor security hole, given that on modern systems it would merely
enable the current seat owner to be a nuisance to their successor, by
making a reboot necessary.
---
 sound/pci/emu10k1/emu10k1_patch.c | 4 ----
 sound/pci/emu10k1/memory.c        | 6 ++++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 47d69a0e44bc..55bb60d31fe4 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -65,17 +65,13 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	size = BLANK_HEAD_SIZE;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	if (offset + size > blocksize)
-		return -EINVAL;
 	snd_emu10k1_synth_bzero(emu, sp->block, offset, size);
 	offset += size;

 	/* copy provided samples */
 	size = sp->v.size;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	if (offset + size > blocksize)
-		return -EINVAL;
 	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c
index 20b07117574b..fc9444404151 100644
--- a/sound/pci/emu10k1/memory.c
+++ b/sound/pci/emu10k1/memory.c
@@ -574,6 +574,9 @@ int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk
 	void *ptr;
 	struct snd_emu10k1_memblk *p = (struct snd_emu10k1_memblk *)blk;

+	if (snd_BUG_ON(offset + size > p->mem.size))
+		return -EFAULT;
+
 	offset += blk->offset & (PAGE_SIZE - 1);
 	end_offset = offset + size;
 	page = get_aligned_page(offset);
@@ -604,6 +607,9 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me
 	void *ptr;
 	struct snd_emu10k1_memblk *p = (struct snd_emu10k1_memblk *)blk;

+	if (snd_BUG_ON(offset + size > p->mem.size))
+		return -EFAULT;
+
 	offset += blk->offset & (PAGE_SIZE - 1);
 	end_offset = offset + size;
 	page = get_aligned_page(offset);
--
2.42.0.419.g70bf8a5751

[PATCH v2 09/17] ALSA: emu10k1: fix sample signedness issues in wavetable loader

[Oswald Buddenhagen]

The hardware supports S16LE and U8 samples, while U16LE and S8 (which
the driver implicitly claims to support) require sign flipping.

Note that this matters only for the GUS patch loader, as the implemented
SoundFont v2.01 spec is limited to S16LE.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h           |  4 +--
 sound/pci/emu10k1/emu10k1_patch.c | 30 ++++++++-----------
 sound/pci/emu10k1/memory.c        | 49 +++++++++++++++++++++++++------
 3 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 1af9e6819392..9e3bd4f81460 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1882,8 +1882,8 @@ int snd_emu10k1_alloc_pages_maybe_wider(struct snd_emu10k1 *emu, size_t size,
 					struct snd_dma_buffer *dmab);
 struct snd_util_memblk *snd_emu10k1_synth_alloc(struct snd_emu10k1 *emu, unsigned int size);
 int snd_emu10k1_synth_free(struct snd_emu10k1 *emu, struct snd_util_memblk *blk);
-int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, int size);
-int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, const char __user *data, int size);
+int snd_emu10k1_synth_memset(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, int size, u8 value);
+int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, const char __user *data, int size, u32 xor);
 int snd_emu10k1_memblk_map(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk);

 /* voice allocation */
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 55bb60d31fe4..eb3d1ef8a33a 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -26,6 +26,8 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		       struct snd_util_memhdr *hdr,
 		       const void __user *data, long count)
 {
+	u8 fill;
+	u32 xor;
 	int offset;
 	int truesize, size, blocksize;
 	unsigned int start_addr;
@@ -41,6 +43,14 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		       sp->v.sample);
 	}

+	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS) {
+		fill = 0x80;
+		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0 : 0x80808080;
+	} else {
+		fill = 0;
+		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0x80008000 : 0;
+	}
+
 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
@@ -65,46 +75,32 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	size = BLANK_HEAD_SIZE;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	snd_emu10k1_synth_bzero(emu, sp->block, offset, size);
+	snd_emu10k1_synth_memset(emu, sp->block, offset, size, fill);
 	offset += size;

 	/* copy provided samples */
 	size = sp->v.size;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
+	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
 		return -EFAULT;
 	}
 	offset += size;

 	/* clear rest of samples (if any) */
 	if (offset < blocksize)
-		snd_emu10k1_synth_bzero(emu, sp->block, offset, blocksize - offset);
+		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);

 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) {
 		/* if no blank loop is attached in the sample, add it */
 		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) {
 			sp->v.loopstart = sp->v.end + BLANK_LOOP_START;
 			sp->v.loopend = sp->v.end + BLANK_LOOP_END;
 		}
 	}

-#if 0 /* not supported yet */
-	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) {
-		/* unsigned -> signed */
-		if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) {
-			unsigned short *wblock = (unsigned short*)block;
-			for (i = 0; i < truesize; i++)
-				wblock[i] ^= 0x8000;
-		} else {
-			for (i = 0; i < truesize; i++)
-				block[i] ^= 0x80;
-		}
-	}
-#endif
-
 	/* recalculate offset */
 	start_addr = BLANK_HEAD_SIZE * 2;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c
index fc9444404151..d29711777161 100644
--- a/sound/pci/emu10k1/memory.c
+++ b/sound/pci/emu10k1/memory.c
@@ -565,10 +565,10 @@ static inline void *offset_ptr(struct snd_emu10k1 *emu, int page, int offset)
 }

 /*
- * bzero(blk + offset, size)
+ * memset(blk + offset, value, size)
  */
-int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk,
-			    int offset, int size)
+int snd_emu10k1_synth_memset(struct snd_emu10k1 *emu, struct snd_util_memblk *blk,
+			     int offset, int size, u8 value)
 {
 	int page, nextofs, end_offset, temp, temp1;
 	void *ptr;
@@ -588,20 +588,47 @@ int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk
 			temp = temp1;
 		ptr = offset_ptr(emu, page + p->first_page, offset);
 		if (ptr)
-			memset(ptr, 0, temp);
+			memset(ptr, value, temp);
 		offset = nextofs;
 		page++;
 	} while (offset < end_offset);
 	return 0;
 }

-EXPORT_SYMBOL(snd_emu10k1_synth_bzero);
+EXPORT_SYMBOL(snd_emu10k1_synth_memset);
+
+// Note that the value is assumed to be suitably repetitive.
+static void xor_range(void *ptr, int size, u32 value)
+{
+	if ((long)ptr & 1) {
+		*(u8 *)ptr ^= (u8)value;
+		ptr++;
+		size--;
+	}
+	if (size > 1 && ((long)ptr & 2)) {
+		*(u16 *)ptr ^= (u16)value;
+		ptr += 2;
+		size -= 2;
+	}
+	while (size > 3) {
+		*(u32 *)ptr ^= value;
+		ptr += 4;
+		size -= 4;
+	}
+	if (size > 1) {
+		*(u16 *)ptr ^= (u16)value;
+		ptr += 2;
+		size -= 2;
+	}
+	if (size > 0)
+		*(u8 *)ptr ^= (u8)value;
+}

 /*
- * copy_from_user(blk + offset, data, size)
+ * copy_from_user(blk + offset, data, size) ^ xor
  */
 int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk,
-				     int offset, const char __user *data, int size)
+				     int offset, const char __user *data, int size, u32 xor)
 {
 	int page, nextofs, end_offset, temp, temp1;
 	void *ptr;
@@ -620,8 +647,12 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me
 		if (temp1 < temp)
 			temp = temp1;
 		ptr = offset_ptr(emu, page + p->first_page, offset);
-		if (ptr && copy_from_user(ptr, data, temp))
-			return -EFAULT;
+		if (ptr) {
+			if (copy_from_user(ptr, data, temp))
+				return -EFAULT;
+			if (xor)
+				xor_range(ptr, temp, xor);
+		}
 		offset = nextofs;
 		data += temp;
 		page++;
--
2.42.0.419.g70bf8a5751

[PATCH v2 10/17] ALSA: emu10k1: fix playback of 8-bit wavetable samples

[Oswald Buddenhagen]

Samples are byte-sized in this mode, and thus the offset calculation
needs no shifting.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_callback.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index 941bfbf812ed..5f6c47cbb809 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -310,27 +310,29 @@ start_voice(struct snd_emux_voice *vp)
 {
 	unsigned int temp;
 	int ch;
+	bool w_16;
 	u32 psst, dsl, map, ccca, vtarget;
 	unsigned int addr, mapped_offset;
 	struct snd_midi_channel *chan;
 	struct snd_emu10k1 *hw;
 	struct snd_emu10k1_memblk *emem;

 	hw = vp->hw;
 	ch = vp->ch;
 	if (snd_BUG_ON(ch < 0))
 		return -EINVAL;
 	chan = vp->chan;
+	w_16 = !(vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_8BITS);

 	emem = (struct snd_emu10k1_memblk *)vp->block;
 	if (emem == NULL)
 		return -EINVAL;
 	emem->map_locked++;
 	if (snd_emu10k1_memblk_map(hw, emem) < 0) {
 		/* dev_err(hw->card->devK, "emu: cannot map!\n"); */
 		return -ENOMEM;
 	}
-	mapped_offset = snd_emu10k1_memblk_offset(emem) >> 1;
+	mapped_offset = snd_emu10k1_memblk_offset(emem) >> w_16;
 	vp->reg.start += mapped_offset;
 	vp->reg.end += mapped_offset;
 	vp->reg.loopstart += mapped_offset;
@@ -371,7 +373,7 @@ start_voice(struct snd_emux_voice *vp)
 		unsigned int shift = (vp->apitch - 0xe000) >> 10;
 		ccca |= shift << 25;
 	}
-	if (vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_8BITS)
+	if (!w_16)
 		ccca |= CCCA_8BITSELECT;

 	vtarget = (unsigned int)vp->vtarget << 16;
--
2.42.0.419.g70bf8a5751

[PATCH v2 11/17] ALSA: emu10k1: merge conditions in patch loader

[Oswald Buddenhagen]

This de-duplicates the code slightly. But the real reason is that it
moves the code up, which the next patch will depend on.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index eb3d1ef8a33a..281881f7d0a4 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -53,8 +53,14 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,

 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
-	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
+	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) {
 		truesize += BLANK_LOOP_SIZE;
+		/* if no blank loop is attached in the sample, add it */
+		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) {
+			sp->v.loopstart = sp->v.end + BLANK_LOOP_START;
+			sp->v.loopend = sp->v.end + BLANK_LOOP_END;
+		}
+	}

 	/* try to allocate a memory block */
 	blocksize = truesize;
@@ -93,14 +99,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (offset < blocksize)
 		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);

-	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) {
-		/* if no blank loop is attached in the sample, add it */
-		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) {
-			sp->v.loopstart = sp->v.end + BLANK_LOOP_START;
-			sp->v.loopend = sp->v.end + BLANK_LOOP_END;
-		}
-	}
-
 	/* recalculate offset */
 	start_addr = BLANK_HEAD_SIZE * 2;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
--
2.42.0.419.g70bf8a5751

[PATCH v2 12/17] ALSA: emu10k1: fix wavetable offset recalculation

[Oswald Buddenhagen]

The offsets are counted in samples, not in bytes.

While the code block is being rewritten, also move it up a bit, to avoid
churn in a subsequent patch.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 281881f7d0a4..ad16de99b800 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -30,7 +30,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	u32 xor;
 	int offset;
 	int truesize, size, blocksize;
-	unsigned int start_addr;
 	struct snd_emu10k1 *emu;

 	emu = rec->hw;
@@ -62,6 +61,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		}
 	}

+	/* recalculate offset */
+	sp->v.start += BLANK_HEAD_SIZE;
+	sp->v.end += BLANK_HEAD_SIZE;
+	sp->v.loopstart += BLANK_HEAD_SIZE;
+	sp->v.loopend += BLANK_HEAD_SIZE;
+
 	/* try to allocate a memory block */
 	blocksize = truesize;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
@@ -99,15 +104,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (offset < blocksize)
 		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);

-	/* recalculate offset */
-	start_addr = BLANK_HEAD_SIZE * 2;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		start_addr >>= 1;
-	sp->v.start += start_addr;
-	sp->v.end += start_addr;
-	sp->v.loopstart += start_addr;
-	sp->v.loopend += start_addr;
-
 	return 0;
 }

--
2.42.0.419.g70bf8a5751

[PATCH v2 13/17] ALSA: emu10k1: de-duplicate size calculations for 16-bit samples

[Oswald Buddenhagen]

Instead of repeatedly checking the sample width, assign a size shift
centrally.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index ad16de99b800..481fe03fef4d 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -28,6 +28,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 {
 	u8 fill;
 	u32 xor;
+	int shift;
 	int offset;
 	int truesize, size, blocksize;
 	struct snd_emu10k1 *emu;
@@ -43,9 +44,11 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	}

 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS) {
+		shift = 0;
 		fill = 0x80;
 		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0 : 0x80808080;
 	} else {
+		shift = 1;
 		fill = 0;
 		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0x80008000 : 0;
 	}
@@ -68,9 +71,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	sp->v.loopend += BLANK_HEAD_SIZE;

 	/* try to allocate a memory block */
-	blocksize = truesize;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		blocksize *= 2;
+	blocksize = truesize << shift;
 	sp->block = snd_emu10k1_synth_alloc(emu, blocksize);
 	if (sp->block == NULL) {
 		dev_dbg(emu->card->dev,
@@ -83,16 +84,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,

 	/* write blank samples at head */
 	offset = 0;
-	size = BLANK_HEAD_SIZE;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		size *= 2;
+	size = BLANK_HEAD_SIZE << shift;
 	snd_emu10k1_synth_memset(emu, sp->block, offset, size, fill);
 	offset += size;

 	/* copy provided samples */
-	size = sp->v.size;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		size *= 2;
+	size = sp->v.size << shift;
 	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
--
2.42.0.419.g70bf8a5751

[PATCH v2 14/17] ALSA: emu10k1: improve cache behavior documentation

[Oswald Buddenhagen]

Resulting from more reverse engineering in the course of debugging.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 9e3bd4f81460..12c7dc760724 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -598,17 +598,25 @@ SUB_REG(PEFE, FILTERAMOUNT,	0x000000ff)	/* Filter envlope amount				*/
 // In stereo mode, the two channels' caches are concatenated into one,
 // and hold the interleaved frames.
 // The cache holds 64 frames, so the upper half is not used in 8-bit mode.
-// All registers mentioned below count in frames.
-// The cache is a ring buffer; CCR_READADDRESS operates modulo 64.
-// The cache is filled from (CCCA_CURRADDR - CCR_CACHEINVALIDSIZE)
-// into (CCR_READADDRESS - CCR_CACHEINVALIDSIZE).
+// All registers mentioned below count in frames. Shortcuts:
+//   CA = CCCA_CURRADDR, CRA = CCR_READADDRESS,
+//   CLA = CCR_CACHELOOPADDRHI:CLP_CACHELOOPADDR,
+//   CIS = CCR_CACHEINVALIDSIZE, LIS = CCR_LOOPINVALSIZE,
+//   CLF = CCR_CACHELOOPFLAG, LF = CCR_LOOPFLAG
+// The cache is a ring buffer; CRA operates modulo 64.
+// The cache is filled from (CA - CIS) into (CRA - CIS).
 // The engine has a fetch threshold of 32 bytes, so it tries to keep
-// CCR_CACHEINVALIDSIZE below 8 (16-bit stereo), 16 (16-bit mono,
-// 8-bit stereo), or 32 (8-bit mono). The actual transfers are pretty
-// unpredictable, especially if several voices are running.
-// Frames are consumed at CCR_READADDRESS, which is incremented afterwards,
-// along with CCCA_CURRADDR and CCR_CACHEINVALIDSIZE. This implies that the
-// actual playback position always lags CCCA_CURRADDR by exactly 64 frames.
+// CIS below 8 (16-bit stereo), 16 (16-bit mono, 8-bit stereo), or
+// 32 (8-bit mono). The actual transfers are pretty unpredictable,
+// especially if several voices are running.
+// Frames are consumed at CRA, which is incremented afterwards,
+// along with CA and CIS. This implies that the actual playback
+// position always lags CA by exactly 64 frames.
+// When CA reaches DSL_LOOPENDADDR, LF is set for one frame's time.
+// LF's rising edge causes the current values of CA and CIS to be
+// copied into CLA and LIS, resp., and CLF to be set.
+// If CLF is set, the first LIS of the CIS frames are instead
+// filled from (CLA - LIS), and CLF is subsequently reset.
 #define CD0			0x20		/* Cache data registers 0 .. 0x1f			*/

 #define PTB			0x40		/* Page table base register				*/
--
2.42.0.419.g70bf8a5751

[PATCH v2 15/17] ALSA: emu10k1: fix wavetable playback position and caching, take 2

[Oswald Buddenhagen]

Compensate for the cache lag of 64 frames, and actually populate the
cache. Without these, the playback would start with garbage (which
would be (mostly?) masqueraded by the note's attack phase).

Note that we set the starting address only 61 frames ahead, to
compensate for the interpolator's epsilon. Unlike for PCM playback, we
don't even need to manually silence-fill the first frames in the cache,
because we insert some silence in front of each sample anyway.

A challenge are extremely short samples with a loop end below the cache
size, because a) we'd have to wrap the current address to be within the
loop and b) automatic pre-filling of the cache with the right data does
not work in this case.

We could pre-fill the cache manually, but that's slow, requires
additional code for each sample width, and is made even more complex by
the driver's virtual address space having no contiguous mapping for the
CPU.

We could have the engine fill the cache piece-wise (which is really what
happens when playback is running), but that would also be complex, and
we'd need to wait for the engine to handle each piece, so it wouldn't be
that much faster than the manual fill.

For the case of requiring only one loop iteration prior to reaching the
cache size, we could leverage the engine's looping mechanism around
CCR_CACHELOOPFLAG, but this special case doesn't seem worth the
complexity.

So we just unroll the loop as far as necessary to be able to play back
the sample without any fiddling.

Pedantically, this would be incorrect for loop-until-release samples
with a low loop end which are released very quickly, but that would be
relatively harmless, is not a plausible use case in the first place, and
SoundFont sample mode 3 isn't actually implemented anyway (it's
conflated with mode 1, infinite looping).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_callback.c |  7 ++--
 sound/pci/emu10k1/emu10k1_patch.c    | 53 +++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index 5f6c47cbb809..ef26e4d3e2a3 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -255,7 +255,7 @@ lookup_voices(struct snd_emux *emu, struct snd_emu10k1 *hw,
 		/* check if sample is finished playing (non-looping only) */
 		if (bp != best + V_OFF && bp != best + V_FREE &&
 		    (vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_SINGLESHOT)) {
-			val = snd_emu10k1_ptr_read(hw, CCCA_CURRADDR, vp->ch);
+			val = snd_emu10k1_ptr_read(hw, CCCA_CURRADDR, vp->ch) - 64 + 3;
 			if (val >= vp->reg.loopstart)
 				bp = best + V_OFF;
 		}
@@ -364,7 +364,7 @@ start_voice(struct snd_emux_voice *vp)

 	map = (hw->silent_page.addr << hw->address_mode) | (hw->address_mode ? MAP_PTI_MASK1 : MAP_PTI_MASK0);

-	addr = vp->reg.start;
+	addr = vp->reg.start + 64 - 3;
 	temp = vp->reg.parm.filterQ;
 	ccca = (temp << 28) | addr;
 	if (vp->apitch < 0xe400)
@@ -432,6 +432,9 @@ start_voice(struct snd_emux_voice *vp)
 		/* Q & current address (Q 4bit value, MSB) */
 		CCCA, ccca,

+		/* cache */
+		CCR, REG_VAL_PUT(CCR_CACHEINVALIDSIZE, 64),
+
 		/* reset volume */
 		VTFT, vtarget | vp->ftarget,
 		CVCF, vtarget | CVCF_CURRENTFILTER_MASK,
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 481fe03fef4d..2a13fb32c1d2 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -31,6 +31,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	int shift;
 	int offset;
 	int truesize, size, blocksize;
+	int loop_start, loop_end, loop_size, data_end, unroll;
 	struct snd_emu10k1 *emu;

 	emu = rec->hw;
@@ -64,12 +65,35 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		}
 	}

+	loop_start = sp->v.loopstart;
+	loop_end = sp->v.loopend;
+	loop_size = loop_end - loop_start;
+	if (!loop_size)
+		return -EINVAL;
+	data_end = sp->v.end;
+
 	/* recalculate offset */
 	sp->v.start += BLANK_HEAD_SIZE;
 	sp->v.end += BLANK_HEAD_SIZE;
 	sp->v.loopstart += BLANK_HEAD_SIZE;
 	sp->v.loopend += BLANK_HEAD_SIZE;

+	// Automatic pre-filling of the cache does not work in the presence
+	// of loops (*), and we don't want to fill it manually, as that is
+	// fiddly and slow. So we unroll the loop until the loop end is
+	// beyond the cache size.
+	// (*) Strictly speaking, a single iteration is supported (that's
+	// how it works when the playback engine runs), but handling this
+	// special case is not worth it.
+	unroll = 0;
+	while (sp->v.loopend < 64) {
+		truesize += loop_size;
+		sp->v.loopstart += loop_size;
+		sp->v.loopend += loop_size;
+		sp->v.end += loop_size;
+		unroll++;
+	}
+
 	/* try to allocate a memory block */
 	blocksize = truesize << shift;
 	sp->block = snd_emu10k1_synth_alloc(emu, blocksize);
@@ -89,19 +113,38 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	offset += size;

 	/* copy provided samples */
-	size = sp->v.size << shift;
-	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) {
-		snd_emu10k1_synth_free(emu, sp->block);
-		sp->block = NULL;
-		return -EFAULT;
+	if (unroll && loop_end <= data_end) {
+		size = loop_end << shift;
+		if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor))
+			goto faulty;
+		offset += size;
+
+		data += loop_start << shift;
+		while (--unroll > 0) {
+			size = loop_size << shift;
+			if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor))
+				goto faulty;
+			offset += size;
+		}
+
+		size = (data_end - loop_start) << shift;
+	} else {
+		size = data_end << shift;
 	}
+	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor))
+		goto faulty;
 	offset += size;

 	/* clear rest of samples (if any) */
 	if (offset < blocksize)
 		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);

 	return 0;
+
+faulty:
+	snd_emu10k1_synth_free(emu, sp->block);
+	sp->block = NULL;
+	return -EFAULT;
 }

 /*
--
2.42.0.419.g70bf8a5751

[PATCH v2 16/17] ALSA: emu10k1: shrink blank space in front of wavetable samples

[Oswald Buddenhagen]

There is no need for it to be 32 samples - 3 will do just fine (which is
the interpolator's epsilon). The old size was presumably meant to
compensate for the cache's presence, but we're now handling that
properly.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 2a13fb32c1d2..dbfa89435ac2 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -16,7 +16,7 @@
 #define BLANK_LOOP_START	4
 #define BLANK_LOOP_END		8
 #define BLANK_LOOP_SIZE		12
-#define BLANK_HEAD_SIZE		32
+#define BLANK_HEAD_SIZE		3

 /*
  * allocate a sample block and copy data from userspace
--
2.42.0.419.g70bf8a5751

[PATCH v2 17/17] ALSA: emux: simplify snd_sf_list.callback handling

[Oswald Buddenhagen]

Both drivers provide both sample_new and sample_free, and it makes no
sense to pretend that they could not. In fact, load_data() would already
crash if sample_new was null. So remove the remaining null checks.

Contrary to that, the emu10k1 driver actually has a null sample_reset,
though I'm not convinced that this inconsistency is justified.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/emux.c      |  6 ++----
 sound/synth/emux/soundfont.c | 12 +++++-------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
index a82af9374852..01444fc960d0 100644
--- a/sound/synth/emux/emux.c
+++ b/sound/synth/emux/emux.c
@@ -94,10 +94,8 @@ int snd_emux_register(struct snd_emux *emu, struct snd_card *card, int index, ch
 	/* create soundfont list */
 	memset(&sf_cb, 0, sizeof(sf_cb));
 	sf_cb.private_data = emu;
-	if (emu->ops.sample_new)
-		sf_cb.sample_new = sf_sample_new;
-	if (emu->ops.sample_free)
-		sf_cb.sample_free = sf_sample_free;
+	sf_cb.sample_new = sf_sample_new;
+	sf_cb.sample_free = sf_sample_free;
 	if (emu->ops.sample_reset)
 		sf_cb.sample_reset = sf_sample_reset;
 	emu->sflist = snd_sf_new(&sf_cb, emu->memhdr);
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index 4edc693da8e7..2373ed580bf8 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -1051,7 +1051,7 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	/*
 	 * load wave data
 	 */
-	if (smp->v.size > 0 && sflist->callback.sample_new) {
+	if (smp->v.size > 0) {
 		rc = sflist->callback.sample_new
 			(sflist->callback.private_data, smp, sflist->memhdr,
 			 data, count);
@@ -1416,9 +1416,8 @@ snd_sf_clear(struct snd_sf_list *sflist)
 		}
 		for (sp = sf->samples; sp; sp = nextsp) {
 			nextsp = sp->next;
-			if (sflist->callback.sample_free)
-				sflist->callback.sample_free(sflist->callback.private_data,
-							     sp, sflist->memhdr);
+			sflist->callback.sample_free(sflist->callback.private_data,
+						     sp, sflist->memhdr);
 			kfree(sp);
 		}
 		kfree(sf);
@@ -1520,9 +1519,8 @@ snd_soundfont_remove_unlocked(struct snd_sf_list *sflist)
 			nextsp = sp->next;
 			sf->samples = nextsp;
 			sflist->mem_used -= sp->v.truesize;
-			if (sflist->callback.sample_free)
-				sflist->callback.sample_free(sflist->callback.private_data,
-							     sp, sflist->memhdr);
+			sflist->callback.sample_free(sflist->callback.private_data,
+						     sp, sflist->memhdr);
 			kfree(sp);
 		}
 	}
--
2.42.0.419.g70bf8a5751

Re: [PATCH v2 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Takashi Iwai]

On Thu, 04 Apr 2024 12:00:31 +0200,
Oswald Buddenhagen wrote:
> 
> ---
> This patch series needs to be applied on top of the patch titled
> "Revert "ALSA: emu10k1: fix synthesizer sample playback position and
> caching"".

The patch set isn't cleanly applicable even after the revert patch.
The patch 7 fails.

Please rebase to the latest for-linus branch and resubmit.


thanks,

Takashi

> 
> Oswald Buddenhagen (17):
>   ALSA: emux: fix /proc teardown at module unload
>   ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch()
>   ALSA: emux: fix validation of snd_emux.num_ports
>   ALSA: emux: fix init of patch_info.truesize in load_data()
>   ALSA: emu10k1: prune vestiges of
>     SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support
>   ALSA: emux: centralize & improve patch info validation
>   ALSA: emux: improve patch ioctl data validation
>   ALSA: emu10k1: move patch loader assertions into low-level functions
>   ALSA: emu10k1: fix sample signedness issues in wavetable loader
>   ALSA: emu10k1: fix playback of 8-bit wavetable samples
>   ALSA: emu10k1: merge conditions in patch loader
>   ALSA: emu10k1: fix wavetable offset recalculation
>   ALSA: emu10k1: de-duplicate size calculations for 16-bit samples
>   ALSA: emu10k1: improve cache behavior documentation
>   ALSA: emu10k1: fix wavetable playback position and caching, take 2
>   ALSA: emu10k1: shrink blank space in front of wavetable samples
>   ALSA: emux: simplify snd_sf_list.callback handling
> 
>  include/sound/emu10k1.h              |  32 +++--
>  include/sound/soundfont.h            |   2 +-
>  sound/isa/sb/emu8000_patch.c         |  13 --
>  sound/pci/emu10k1/emu10k1_callback.c |  13 +-
>  sound/pci/emu10k1/emu10k1_patch.c    | 207 +++++++++++----------------
>  sound/pci/emu10k1/memory.c           |  55 +++++--
>  sound/synth/emux/emux.c              |   6 +-
>  sound/synth/emux/emux_hwdep.c        |   3 +-
>  sound/synth/emux/emux_oss.c          |   3 +-
>  sound/synth/emux/emux_proc.c         |   1 +
>  sound/synth/emux/emux_seq.c          |   6 +-
>  sound/synth/emux/soundfont.c         |  73 +++++++---
>  12 files changed, 219 insertions(+), 195 deletions(-)
> 
> 
> base-commit: ed93395844979f6bf2e1fbfcda38d1718289b426
> prerequisite-patch-id: b7769e1c8649b86fd9e0b259e11bfd8e468393e5
> --
> 2.42.0.419.g70bf8a5751
> 

Re: [PATCH v2 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Oswald Buddenhagen]

On Fri, Apr 05, 2024 at 11:20:32AM +0200, Takashi Iwai wrote:
>On Thu, 04 Apr 2024 12:00:31 +0200,
>Oswald Buddenhagen wrote:
>>
>> ---
>> This patch series needs to be applied on top of the patch titled
>> "Revert "ALSA: emu10k1: fix synthesizer sample playback position and
>> caching"".
>
>The patch set isn't cleanly applicable even after the revert patch.
>The patch 7 fails.
>
>Please rebase to the latest for-linus branch and resubmit.
>
this makes no sense; i'm getting a bit-identical patch after the rebase
(which is unsurprising, as the file in question wasn't touched in
years).

are you sure you didn't corrupt the patch somehow (it happened before,
cf. summary of c960b012ec47)? or maybe you have an unpublished
conflicting commit?

if there is an actual problem and you just named the wrong patch, then i
suspect that it's just git-am being stupid - the rebases from 6.8 and
later from your master from about a week ago went through smoothly.

Re: [PATCH v2 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Takashi Iwai]

On Fri, 05 Apr 2024 12:07:57 +0200,
Oswald Buddenhagen wrote:
> 
> On Fri, Apr 05, 2024 at 11:20:32AM +0200, Takashi Iwai wrote:
> > On Thu, 04 Apr 2024 12:00:31 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> ---
> >> This patch series needs to be applied on top of the patch titled
> >> "Revert "ALSA: emu10k1: fix synthesizer sample playback position and
> >> caching"".
> > 
> > The patch set isn't cleanly applicable even after the revert patch.
> > The patch 7 fails.
> > 
> > Please rebase to the latest for-linus branch and resubmit.
> > 
> this makes no sense; i'm getting a bit-identical patch after the rebase
> (which is unsurprising, as the file in question wasn't touched in
> years).
> 
> are you sure you didn't corrupt the patch somehow (it happened before,
> cf. summary of c960b012ec47)? or maybe you have an unpublished
> conflicting commit?
>
> if there is an actual problem and you just named the wrong patch, then i
> suspect that it's just git-am being stupid - the rebases from 6.8 and
> later from your master from about a week ago went through smoothly.

No, I used b4 at this time, and such a failure shouldn't happen.

Try by yourself to apply the submitted patch mails with git-am on the
latest for-linus (or master) branch.


thanks,

Takashi

Re: [PATCH v2 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Oswald Buddenhagen]

On Fri, Apr 05, 2024 at 12:29:25PM +0200, Takashi Iwai wrote:
>Try by yourself to apply the submitted patch mails with git-am on the
>latest for-linus (or master) branch.
>
ok, the problem is indeed patch corruption, but it's not your fault.
trailing tabs got stripped from the patches in flight.

while my reading of
https://www.rfc-editor.org/rfc/rfc5321#section-2.3.10 is that MTAs may
not just strip trailing whitespace, the ones i tried apparently do.
somebody may want to verify that ...

anyway, you can still apply the patches by adding --ignore-whitespace to
git-am's command line.

if the process doesn't permit that, i'll re-post after convincing
git-send-email to apply quoted-printable content-transfer-encoding to
ensure preservation of trailing whitespace.

Re: [PATCH v2 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Takashi Iwai]

On Fri, 05 Apr 2024 20:38:22 +0200,
Oswald Buddenhagen wrote:
> 
> On Fri, Apr 05, 2024 at 12:29:25PM +0200, Takashi Iwai wrote:
> > Try by yourself to apply the submitted patch mails with git-am on the
> > latest for-linus (or master) branch.
> > 
> ok, the problem is indeed patch corruption, but it's not your fault.
> trailing tabs got stripped from the patches in flight.
> 
> while my reading of
> https://www.rfc-editor.org/rfc/rfc5321#section-2.3.10 is that MTAs may
> not just strip trailing whitespace, the ones i tried apparently do.
> somebody may want to verify that ...
> 
> anyway, you can still apply the patches by adding --ignore-whitespace to
> git-am's command line.
> 
> if the process doesn't permit that, i'll re-post after convincing
> git-send-email to apply quoted-printable content-transfer-encoding to
> ensure preservation of trailing whitespace.

Yes, please resubmit with the correct content.
All the original posts are archived and may be used to check for the
correctness of the patches later.


thanks,

Takashi