On Mon, 2009-04-20 at 21:45 -0600, dann frazier wrote:
> On Wed, Apr 15, 2009 at 11:01:09AM +0200, Antonio Ospite wrote:
> > Hi,
> > 
> > I just wonder if there is any update on this one and if the split-out
> > patch has been proposed to upstream yet.
> > 
> > If you manage to get this upstream, with Linus keeping on distributing
> > the binary images, debian can well choose not to distribute them, but
> > debian users can still get the blob somewhere and have an easier life.
> > Not ideal, I know, but that's the world.
> 
> Yep - that's true.
> 
> Kalle: would you mind submitting your patch upstream, if you haven't
> already? A lot of similar patches for other drives have been accepted
> in recent months.

Kalle's patch has a serious problem in that it attempts to byte-swap the
firmware in place.  On a big-endian system where the firmware is built
into the kernel, or if a cache is implemented, this will corrupt the
image or cause an oops.

Furthermore, I think any patch sent upstream will need to handle the
"new" DSP code as well.

Anyway, here's my proposed patch for unstable (against 2.6.30-rc4) that
deals with the first problem.  I'll have a go at handling the "new" DSP
code as well, but as I don't have the hardware for this driver this will
need testing by others.

I made the following changes relative to Kalle's patch:

- Remove "Modified on..." lines; that's what the commit message is for
- Do not call release_firmware() if request_firmware() fails
- Make firmware images explicitly const and little-endian and never swap
them.
- Remove offsets from firmware header so that we don't have to validate
them; we know the offset should be the base of the corresponding memory
bank.
- Validate sizes against the memory bank size.
- Change filename to cs46xx-old.fw as this is easier to associate with
its use.  We can use cs46xx-new.fw for the "new" DSP code.

Ben.

diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig
index 17e03b9..124b3a0 100644
--- a/sound/pci/Kconfig
+++ b/sound/pci/Kconfig
@@ -229,7 +229,7 @@ config SND_CS4281
 
 config SND_CS46XX
        tristate "Cirrus Logic (Sound Fusion) CS4280/CS461x/CS462x/CS463x"
-       depends on BROKEN
+       select FW_LOADER
        select SND_RAWMIDI
        select SND_AC97_CODEC
        help
@@ -241,6 +241,7 @@ config SND_CS46XX
 
 config SND_CS46XX_NEW_DSP
        bool "Cirrus Logic (Sound Fusion) New DSP support"
+       depends on BROKEN
        depends on SND_CS46XX
        default y
        help
diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
index 1be96ea..b12b930 100644
--- a/sound/pci/cs46xx/cs46xx_lib.c
+++ b/sound/pci/cs46xx/cs46xx_lib.c
@@ -53,6 +53,7 @@
 #include <linux/slab.h>
 #include <linux/gameport.h>
 #include <linux/mutex.h>
+#include <linux/firmware.h>
 
 
 #include <sound/core.h>
@@ -308,7 +309,7 @@ static void snd_cs46xx_ac97_write(struct snd_ac97 *ac97,
  */
 
 int snd_cs46xx_download(struct snd_cs46xx *chip,
-                       u32 *src,
+                       const __le32 *src,
                         unsigned long offset,
                         unsigned long len)
 {
@@ -321,9 +322,9 @@ int snd_cs46xx_download(struct snd_cs46xx *chip,
        dst = chip->region.idx[bank+1].remap_addr + offset;
        len /= sizeof(u32);
 
-       /* writel already converts 32-bit value to right endianess */
        while (len-- > 0) {
-               writel(*src++, dst);
+               __raw_writel((__force u32)*src++, dst);
+               mmiowb();
                dst += sizeof(u32);
        }
        return 0;
@@ -360,23 +361,77 @@ int snd_cs46xx_clear_BA1(struct snd_cs46xx *chip,
 
 #else /* old DSP image */
 
-#include "cs46xx_image.h"
+struct cs46xx_old_image {
+       __le32 size[BA1_MEMORY_COUNT];
+       __le32 data[0];
+};
 
-int snd_cs46xx_download_image(struct snd_cs46xx *chip)
+static int snd_cs46xx_check_image_size(const struct firmware *firmware)
 {
-       int idx, err;
-       unsigned long offset = 0;
+       const struct cs46xx_old_image *image =
+               (const struct cs46xx_old_image *)firmware->data;
+       size_t offset = sizeof(*image);
+       int idx;
+
+       if (firmware->size < offset) {
+               snd_printk(KERN_ERR "cs46xx: firmware too small\n");
+               return -EINVAL;
+       }
 
        for (idx = 0; idx < BA1_MEMORY_COUNT; idx++) {
-               if ((err = snd_cs46xx_download(chip,
-                                              &BA1Struct.map[offset],
-                                              BA1Struct.memory[idx].offset,
-                                              BA1Struct.memory[idx].size)) < 0)
-                       return err;
-               offset += BA1Struct.memory[idx].size >> 2;
-       }       
+               size_t size = le32_to_cpu(image->size[idx]);
+
+               if (size % sizeof(u32)) {
+                       snd_printk(KERN_ERR "cs46xx: firmware hunk 
misaligned\n");
+                       return -EINVAL;
+               }
+               if (size > BA1_DWORD_SIZE * sizeof(u32)) {
+                       snd_printk(KERN_ERR "cs46xx: firmware hunk out of 
range\n");
+                       return -EINVAL;
+               }
+               offset += size;
+       }
+
+       if (firmware->size != offset) {
+               snd_printk(KERN_ERR "cs46xx: firmware size mismatch\n");
+               return -EINVAL;
+       }
+
        return 0;
 }
+
+static int snd_cs46xx_download_image(struct snd_cs46xx *chip)
+{
+       int idx, err;
+       const struct firmware *firmware = NULL;
+       const struct cs46xx_old_image *image;
+       const __le32 *data;
+
+       err = request_firmware(&firmware, "cs46xx/cs46xx-old.fw",
+                              &chip->pci->dev);
+       if (err < 0) {
+               snd_printk(KERN_ERR "cs46xx: no firmware\n");
+               return err;
+       }
+
+       err = snd_cs46xx_check_image_size(firmware);
+       if (err < 0)
+               goto end;
+       image = (const struct cs46xx_old_image *)firmware->data;
+       data = image->data;
+
+       for (idx = 0; idx < BA1_MEMORY_COUNT; idx++) {
+               size_t size = le32_to_cpu(image->size[idx]);
+
+               err = snd_cs46xx_download(chip, data, idx << 16, size);
+               if (err < 0)
+                       goto end;
+               data += size / sizeof(u32);
+       }
+end:
+       release_firmware(firmware);
+       return err;
+}
 #endif /* CONFIG_SND_CS46XX_NEW_DSP */
 
 /*
@@ -3874,3 +3929,5 @@ int __devinit snd_cs46xx_create(struct snd_card *card,
        *rchip = chip;
        return 0;
 }
+
+MODULE_FIRMWARE("cs46xx/cs46xx-old.fw");
diff --git a/sound/pci/cs46xx/cs46xx_lib.h b/sound/pci/cs46xx/cs46xx_lib.h
index 4eb55aa..85babb5 100644
--- a/sound/pci/cs46xx/cs46xx_lib.h
+++ b/sound/pci/cs46xx/cs46xx_lib.h
@@ -103,8 +103,8 @@ int cs46xx_dsp_proc_done (struct snd_cs46xx *chip);
 #define cs46xx_dsp_proc_done(chip)
 #endif
 int cs46xx_dsp_scb_and_task_init (struct snd_cs46xx *chip);
-int snd_cs46xx_download (struct snd_cs46xx *chip, u32 *src, unsigned long 
offset,
-                        unsigned long len);
+int snd_cs46xx_download(struct snd_cs46xx *chip, const __le32 *src, unsigned 
long offset,
+                       unsigned long len);
 int snd_cs46xx_clear_BA1(struct snd_cs46xx *chip, unsigned long offset, 
unsigned long len);
 int cs46xx_dsp_enable_spdif_out (struct snd_cs46xx *chip);
 int cs46xx_dsp_enable_spdif_hw (struct snd_cs46xx *chip);
--- END ---

-- 
Ben Hutchings
No political challenge can be met by shopping. - George Monbiot

Attachment: signature.asc
Description: This is a digitally signed message part

Reply via email to