Bug#464197: Any update on this issue?
On Mon, 04 May 2009 00:55:55 +0100
Ben Hutchings b...@decadent.org.uk wrote:
On Mon, 2009-05-04 at 01:35 +0200, Antonio Ospite wrote:
On Mon, 04 May 2009 00:17:03 +0100
Ben Hutchings b...@decadent.org.uk wrote:
...
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 will test this patch soon; wrt the NEW DSP feature, I don't know if I
can test everything, because I have a Thinkpad T20, only stereo output.
Ok, I can confirm that this patch, with the image created as described
below, works OK on my T20. Tested with latest linus git tree.
BTW, what binary image to use? Is the one extracted with the tool in
this thread, to be run on a little-endian host, ok?
You would need to apply this patch to cs46xx_image.h before running that
program:
--- a/sound/pci/cs46xx/cs46xx_image.h
+++ b/sound/pci/cs46xx/cs46xx_image.h
@@ -1,14 +1,13 @@
struct BA1struct {
struct {
- unsigned long offset;
- unsigned long size;
+ u32 size;
} memory[BA1_MEMORY_COUNT];
u32 map[BA1_DWORD_SIZE];
};
static struct BA1struct BA1Struct = {
-{{ 0x, 0x3000 },{ 0x0001, 0x3800 },{ 0x0002,
0x7000 }},
+{{ 0x3000 },{ 0x3800 },{ 0x7000 }},
{0x,0x,0x,0x,
0x,0x,0x,0x,
0x,0x,0x0163,0x,
--- END ---
Also note the change of filename (partly because of the format change).
Ben.
Thanks,
Antonio
--
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc
pgpcIFTLrjYRV.pgp
Description: PGP signature
Bug#464197: Any update on this issue?
On Mon, 2009-05-04 at 08:21 +0300, Kalle Olavi Niemitalo wrote: Ben Hutchings b...@decadent.org.uk writes: - Remove Modified on... lines; that's what the commit message is for Those were the prominent notices mentioned in GPLv2 2. a). Right, I see. I believe the Debian patch system makes our changes prominent enough, and such notices are certainly not expected in patches submitted upstream (see Documentation/SubmittingPatches). Ben. -- Ben Hutchings No political challenge can be met by shopping. - George Monbiot signature.asc Description: This is a digitally signed message part
Bug#464197: Any update on this issue?
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)) {
+
Bug#464197: Any update on this issue?
On Mon, 04 May 2009 00:17:03 +0100 Ben Hutchings b...@decadent.org.uk wrote: On Mon, 2009-04-20 at 21:45 -0600, dann frazier wrote: 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. I saw also that some drivers provide blobs as ihex files, a textual representation of the binary data, and convert it to a binary image at build time. Could this be useful in this case? Furthermore, I think any patch sent upstream will need to handle the new DSP code as well. Indeed. 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 will test this patch soon; wrt the NEW DSP feature, I don't know if I can test everything, because I have a Thinkpad T20, only stereo output. BTW, what binary image to use? Is the one extracted with the tool in this thread, to be run on a little-endian host, ok? Thanks, Antonio -- A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? Web site: http://www.studenti.unina.it/~ospite Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc pgpKoBuyKDevq.pgp Description: PGP signature
Bug#464197: Any update on this issue?
On Mon, 2009-05-04 at 01:35 +0200, Antonio Ospite wrote:
On Mon, 04 May 2009 00:17:03 +0100
Ben Hutchings b...@decadent.org.uk wrote:
On Mon, 2009-04-20 at 21:45 -0600, dann frazier wrote:
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.
I saw also that some drivers provide blobs as ihex files, a textual
representation of the binary data, and convert it to a binary image at
build time. Could this be useful in this case?
In the upstream repository, all firmware extracted from drivers is
stored in some variant of Intel hex format. But this is still
byte-oriented and does not help to avoid byte-swapping.
Furthermore, I think any patch sent upstream will need to handle the
new DSP code as well.
Indeed.
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 will test this patch soon; wrt the NEW DSP feature, I don't know if I
can test everything, because I have a Thinkpad T20, only stereo output.
BTW, what binary image to use? Is the one extracted with the tool in
this thread, to be run on a little-endian host, ok?
You would need to apply this patch to cs46xx_image.h before running that
program:
--- a/sound/pci/cs46xx/cs46xx_image.h
+++ b/sound/pci/cs46xx/cs46xx_image.h
@@ -1,14 +1,13 @@
struct BA1struct {
struct {
- unsigned long offset;
- unsigned long size;
+ u32 size;
} memory[BA1_MEMORY_COUNT];
u32 map[BA1_DWORD_SIZE];
};
static struct BA1struct BA1Struct = {
-{{ 0x, 0x3000 },{ 0x0001, 0x3800 },{ 0x0002,
0x7000 }},
+{{ 0x3000 },{ 0x3800 },{ 0x7000 }},
{0x,0x,0x,0x,
0x,0x,0x,0x,
0x,0x,0x0163,0x,
--- END ---
Also note the change of filename (partly because of the format change).
Ben.
--
Ben Hutchings
No political challenge can be met by shopping. - George Monbiot
signature.asc
Description: This is a digitally signed message part
Bug#464197: Any update on this issue?
Ben Hutchings b...@decadent.org.uk writes: - Remove Modified on... lines; that's what the commit message is for Those were the prominent notices mentioned in GPLv2 2. a). pgpuqTP14vrGY.pgp Description: PGP signature
Bug#464197: Any update on this issue?
Antonio Ospite osp...@studenti.unina.it writes: can the latest version of your code be found in this thread? Yes, the version I posted on 2008-04-06 is the latest one. When building a local linux-2.6 2.6.26-9.kon.1 version, I put the patch at debian/patches/debian/dfsg/sound-pci-cs46xx.patch and added it to the list in debian/patches/series/1. Currently, my primary problem with this patch is that the cs46xx module gets inserted before /usr is mounted, and it then doesn't receive the firmware from userspace and fails to initialize the device. From the source, it appears that snd_card_cs46xx_probe returns -EIO in this situation. To get sound after this, I have to rmmod and insmod cs46xx again, or do: printf :00:0d.0 /sys/module/snd_cs46xx/drivers/pci:Sound Fusion CS46xx/bind The hexadecimal numbers depend on the PCI slot, I guess. pgpEKIzcGvQ06.pgp Description: PGP signature
Bug#464197: Any update on this issue?
On Fri, 01 May 2009 11:16:58 +0300 Kalle Olavi Niemitalo k...@iki.fi wrote: Antonio Ospite osp...@studenti.unina.it writes: can the latest version of your code be found in this thread? Yes, the version I posted on 2008-04-06 is the latest one. When building a local linux-2.6 2.6.26-9.kon.1 version, I put the patch at debian/patches/debian/dfsg/sound-pci-cs46xx.patch and added it to the list in debian/patches/series/1. OK, thanks. About the SND_CS46XX_NEW_DSP issue, from a first glance to the driver it looks like that in order to support it we have to make a binary image for any file in the imgs/ dir. Does this look ok to you? Currently, my primary problem with this patch is that the cs46xx module gets inserted before /usr is mounted, and it then doesn't receive the firmware from userspace and fails to initialize the device. From the source, it appears that snd_card_cs46xx_probe returns -EIO in this situation. To get sound after this, I have to rmmod and insmod cs46xx again, or do: printf :00:0d.0 /sys/module/snd_cs46xx/drivers/pci:Sound Fusion CS46xx/bind The hexadecimal numbers depend on the PCI slot, I guess. From what I know the proper solution should be to add the firmware images to an initrd image. I'll let you know the results of my tests. Regards, Antonio -- A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? Web site: http://www.studenti.unina.it/~ospite Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc pgpjH4LJ23EEa.pgp Description: PGP signature
Bug#464197: Any update on this issue?
Antonio Ospite osp...@studenti.unina.it writes: About the SND_CS46XX_NEW_DSP issue, from a first glance to the driver it looks like that in order to support it we have to make a binary image for any file in the imgs/ dir. Does this look ok to you? I guess that's what needs to be done. The difficulty is with pointers in struct dsp_segment_desc and struct dsp_module_desc. You'll have to replace those with integers (presumably file offsets) and concatenate the structures in the binary file while ensuring proper alignment. It seems quite doable but I don't need the SND_CS46XX_NEW_DSP features myself and cannot test them. From what I know the proper solution should be to add the firmware images to an initrd image. I'll let you know the results of my tests. In the patch, I added a MODULE_FIRMWARE line that should make it possible for initrd-building scripts to include the firmware. However, in the 2.6.26 initrd image I have here, the snd-cs46xx.ko module itself is not included; presumably because the driver isn't needed for booting. So I think it would be pointless to include the firmware there. pgpDDZo9aVf45.pgp Description: PGP signature
Bug#464197: Any update on this issue?
On Tue, 21 Apr 2009 22:58:15 +0300 Kalle Olavi Niemitalo k...@iki.fi wrote: dann frazier da...@debian.org writes: 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. I expect the patch would be rejected because it breaks SND_CS46XX_NEW_DSP. Do you think otherwise? Anyway, I have little interest in working on this now. If you just want a Signed-Off-By, that can probably be arranged. Hi Kalle, can the latest version of your code be found in this thread? If not, please send it, I can start with porting it to latest kernel and testing it, and then we will see what I can do with it. Thanks, Antonio -- A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? Web site: http://www.studenti.unina.it/~ospite Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc pgpvzgLTHqXuh.pgp Description: PGP signature
Bug#464197: Any update on this issue?
dann frazier da...@debian.org writes: 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. I expect the patch would be rejected because it breaks SND_CS46XX_NEW_DSP. Do you think otherwise? Anyway, I have little interest in working on this now. If you just want a Signed-Off-By, that can probably be arranged. pgpa9DlzAlRcx.pgp Description: PGP signature
Bug#464197: Any update on this issue?
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. -- dann frazier -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#464197: Any update on this issue?
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. Regards, Antonio -- A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? Web site: http://www.studenti.unina.it/~ospite Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc pgplrLVTUYy9o.pgp Description: PGP signature
