[PATCH 0/3] snd-usb-6fire: firmware load and pulseaudio assumption
Hi Takashi.
I sent these out a week ago to alsa-devel (only) but am not sure they got anywhere, perhaps due to at least the alsa-devel archive scrubbing the text/x-patch attachments as "non-text".
The snd-usb-6fire driver for the TerraTec DMX 6Fire USB soundcard has been failing its firmware upload due to a non DMA-capable buffer on the stack. First of the patches kmallocs said bufffer instead and fixes the firmware upload.
After that first patch the driver nominally works again but still has Pulseaudio crap out due to struct snd_pcm_hardware.channels_min=1 causing it to recognize it as a mono device only. Comparing with e.g. the TerraTec Aureon 7.1 Universe driver it seems that the solution is to simply set channels_min=2 as per the second patch.
Third patch unmarks the snd_pcm_hardware struct const as it it is in fact changed in usb6fire_pcm_open(), even though through a pointer, and is supposedly trivial.
With these changes the card works again. Driver author Torsten Schenk has seen these and is fine with them: maintains an external driver with more options. I or he might time permitting start integrating more into the kernel driver over time.
Regards, René
René Herman (3): snd-usb-6fire: Move DMA-buffer off of the stack snd-usb-6fire: Pulseaudio needs snd_pcm_hardware.channels_min > 1 snd-usb-6fire: Unmark struct snd_pcm_hardware const
firmware.c | 95 ++++++++++++++++++++++++++---------------------------- pcm.c | 4 +-- 2 files changed, 48 insertions(+), 51 deletions(-)
snd-usb-fire currently fails its firmware load with "transfer buffer not dma capable". Move said buffer off of the stack.
Signed-off-by: René Herman rene.herman@gmail.com --- firmware.c | 95 ++++++++++++++++++++++++++---------------------------- 1 file changed, 46 insertions(+), 49 deletions(-)
diff --git a/firmware.c b/firmware.c index 69137c1..502653a 100644 --- a/firmware.c +++ b/firmware.c @@ -355,63 +355,60 @@ static int usb6fire_fw_check(struct usb_interface *intf, const u8 *version)
int usb6fire_fw_init(struct usb_interface *intf) { - int i; - int ret; struct usb_device *device = interface_to_usbdev(intf); + int ret, i; + /* buffer: 8 receiving bytes from device and * sizeof(EP_W_MAX_PACKET_SIZE) bytes for non-const copy */ - u8 buffer[12]; + u8 *buffer = kmalloc(12, GFP_KERNEL); + + if (!buffer) + return -ENOMEM;
ret = usb6fire_fw_ezusb_read(device, 1, 0, buffer, 8); if (ret < 0) { dev_err(&intf->dev, "unable to receive device firmware state.\n"); - return ret; - } - if (buffer[0] != 0xeb || buffer[1] != 0xaa || buffer[2] != 0x55) { - dev_err(&intf->dev, - "unknown device firmware state received from device:"); - for (i = 0; i < 8; i++) - printk(KERN_CONT "%02x ", buffer[i]); - printk(KERN_CONT "\n"); - return -EIO; - } - /* do we need fpga loader ezusb firmware? */ - if (buffer[3] == 0x01) { - ret = usb6fire_fw_ezusb_upload(intf, - "6fire/dmx6firel2.ihx", 0, NULL, 0); - if (ret < 0) - return ret; - return FW_NOT_READY; + goto out; } - /* do we need fpga firmware and application ezusb firmware? */ - else if (buffer[3] == 0x02) { - ret = usb6fire_fw_check(intf, buffer + 4); - if (ret < 0) - return ret; - ret = usb6fire_fw_fpga_upload(intf, "6fire/dmx6firecf.bin"); - if (ret < 0) - return ret; - memcpy(buffer, ep_w_max_packet_size, - sizeof(ep_w_max_packet_size)); - ret = usb6fire_fw_ezusb_upload(intf, "6fire/dmx6fireap.ihx", - 0x0003, buffer, sizeof(ep_w_max_packet_size)); - if (ret < 0) - return ret; - return FW_NOT_READY; - } - /* all fw loaded? */ - else if (buffer[3] == 0x03) - return usb6fire_fw_check(intf, buffer + 4); - /* unknown data? */ - else { - dev_err(&intf->dev, - "unknown device firmware state received from device: "); - for (i = 0; i < 8; i++) - printk(KERN_CONT "%02x ", buffer[i]); - printk(KERN_CONT "\n"); - return -EIO; + if (buffer[0] == 0xeb && buffer[1] == 0xaa && buffer[2] == 0x55) { + /* do we need fpga loader ezusb firmware? */ + if (buffer[3] == 1) { + ret = usb6fire_fw_ezusb_upload(intf, + "6fire/dmx6firel2.ihx", 0, NULL, 0); + if (ret >= 0) + ret = FW_NOT_READY; + goto out; + } + /* do we need fpga firmware and application ezusb firmware? */ + if (buffer[3] == 2) { + ret = usb6fire_fw_check(intf, buffer + 4); + if (ret < 0) + goto out; + ret = usb6fire_fw_fpga_upload(intf, "6fire/dmx6firecf.bin"); + if (ret < 0) + goto out; + memcpy(buffer, ep_w_max_packet_size, + sizeof(ep_w_max_packet_size)); + ret = usb6fire_fw_ezusb_upload(intf, "6fire/dmx6fireap.ihx", + 0x0003, buffer, sizeof(ep_w_max_packet_size)); + if (ret >= 0) + ret = FW_NOT_READY; + goto out; + } + /* all fw loaded? */ + if (buffer[3] == 3) { + ret = usb6fire_fw_check(intf, buffer + 4); + goto out; + } } - return 0; + dev_err(&intf->dev, + "unknown device firmware state received from device: "); + for (i = 0; i < 8; i++) + printk(KERN_CONT "%02x ", buffer[i]); + printk(KERN_CONT "\n"); + ret = -EIO; + +out: kfree(buffer); + return ret; } -
On Tue, 21 Jul 2020 08:48:51 +0200, René Herman wrote:
snd-usb-fire currently fails its firmware load with "transfer buffer not dma capable". Move said buffer off of the stack.
Signed-off-by: René Herman rene.herman@gmail.com
firmware.c | 95 ++++++++++++++++++++++++++---------------------------- 1 file changed, 46 insertions(+), 49 deletions(-)
diff --git a/firmware.c b/firmware.c index 69137c1..502653a 100644 --- a/firmware.c +++ b/firmware.c @@ -355,63 +355,60 @@ static int usb6fire_fw_check(struct usb_interface *intf, const u8 *version)
int usb6fire_fw_init(struct usb_interface *intf) {
- int i;
- int ret; struct usb_device *device = interface_to_usbdev(intf);
- int ret, i;
- /* buffer: 8 receiving bytes from device and
- sizeof(EP_W_MAX_PACKET_SIZE) bytes for non-const copy */
- u8 buffer[12];
u8 *buffer = kmalloc(12, GFP_KERNEL);
if (!buffer)
return -ENOMEM;ret = usb6fire_fw_ezusb_read(device, 1, 0, buffer, 8); if (ret < 0) { dev_err(&intf->dev, "unable to receive device firmware state.\n");
return ret;- }
- if (buffer[0] != 0xeb || buffer[1] != 0xaa || buffer[2] != 0x55) {
dev_err(&intf->dev,"unknown device firmware state received from device:");for (i = 0; i < 8; i++)printk(KERN_CONT "%02x ", buffer[i]);printk(KERN_CONT "\n");return -EIO;- }
Could you rather change return with goto out (with ret variable set)? In that way we can see what actually you changed more clearly.
thanks,
Takashi
Pulseaudio with snd_pcm_hardware.channels_min=1 creates a mono device only.
Signed-off-by: René Herman rene.herman@gmail.com --- pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pcm.c b/pcm.c index 88ac1c4..cce1312 100644 --- a/pcm.c +++ b/pcm.c @@ -58,7 +58,7 @@ static const struct snd_pcm_hardware pcm_hw = {
.rate_min = 44100, .rate_max = 192000, - .channels_min = 1, + .channels_min = 2, .channels_max = 0, /* set in pcm_open, depending on capture/playback */ .buffer_bytes_max = MAX_BUFSIZE, .period_bytes_min = PCM_N_PACKETS_PER_URB * (PCM_MAX_PACKET_SIZE - 4),
struct snd_pcm_hardware pcm_hw is in fact changed in usb6fire_pcm_open()
Signed-off-by: René Herman rene.herman@gmail.com --- pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pcm.c b/pcm.c index cce1312..8ccf638 100644 --- a/pcm.c +++ b/pcm.c @@ -40,7 +40,7 @@ enum { /* pcm streaming states */ STREAM_STOPPING };
-static const struct snd_pcm_hardware pcm_hw = { +static struct snd_pcm_hardware pcm_hw = { .info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
On Tue, 21 Jul 2020 08:48:53 +0200, René Herman wrote:
struct snd_pcm_hardware pcm_hw is in fact changed in usb6fire_pcm_open()
This must be superfluous. usb6fire_pcm_open() changes the field of the copied pcm_hw, not the original pcm_hw itself. Otherwise we must have got already a compile warning / error.
thanks,
Takashi
Signed-off-by: René Herman rene.herman@gmail.com
pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pcm.c b/pcm.c index cce1312..8ccf638 100644 --- a/pcm.c +++ b/pcm.c @@ -40,7 +40,7 @@ enum { /* pcm streaming states */ STREAM_STOPPING };
-static const struct snd_pcm_hardware pcm_hw = { +static struct snd_pcm_hardware pcm_hw = { .info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | -- 2.17.1
On Tue, 21 Jul 2020 08:48:50 +0200, René Herman wrote:
Hi Takashi.
I sent these out a week ago to alsa-devel (only) but am not sure they got anywhere, perhaps due to at least the alsa-devel archive scrubbing the text/x-patch attachments as "non-text".
The snd-usb-6fire driver for the TerraTec DMX 6Fire USB soundcard has been failing its firmware upload due to a non DMA-capable buffer on the stack. First of the patches kmallocs said bufffer instead and fixes the firmware upload.
After that first patch the driver nominally works again but still has Pulseaudio crap out due to struct snd_pcm_hardware.channels_min=1 causing it to recognize it as a mono device only. Comparing with e.g. the TerraTec Aureon 7.1 Universe driver it seems that the solution is to simply set channels_min=2 as per the second patch.
Third patch unmarks the snd_pcm_hardware struct const as it it is in fact changed in usb6fire_pcm_open(), even though through a pointer, and is supposedly trivial.
With these changes the card works again. Driver author Torsten Schenk has seen these and is fine with them: maintains an external driver with more options. I or he might time permitting start integrating more into the kernel driver over time.
The patch needs to point to the right path that is applicable with patch -p1 option, i.e. it should be like diff -up a/sound/usb/6fire/xxx.c b/sound/usb/6fire/xxx.c
At best use git for creating / submitting a patch.
thanks,
Takashi
On 21-07-2020 10:02, Takashi Iwai wrote:
The patch needs to point to the right path that is applicable with patch -p1 option, i.e. it should be like diff -up a/sound/usb/6fire/xxx.c b/sound/usb/6fire/xxx.c
At best use git for creating / submitting a patch.
Blast. I did, but worked against an isolated (DKMS-ed) version of the kernel driver. Pardon. Shall re-setup against the kernel as such and resend after dealing with your other two comments. I.e.,
Re: [PATCH 1/3] snd-usb-6fire: Move DMA-buffer off of the stack
Could you rather change return with goto out (with ret variable set)? In that way we can see what actually you changed more clearly.
I already did exactly that though and in fact, the original not doing so is what makes the patch seem involved. With the added kmalloc() I change it so that all returns goto out, which kfree()s again. The only one that does not is when kmalloc() fails, i.e., when there's nothing to kfree(). I suppose you just misread and do not need to have that single one go through a goto as well?
Re: [PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const
This must be superfluous. usb6fire_pcm_open() changes the field of the copied pcm_hw, not the original pcm_hw itself. Otherwise we must have got already a compile warning / error.
Unfortunately no; it's as mentioned in the cover letter accessed via pointer: usb6fire_pcm_open() sets "alsa_rt->hw = pcm_hw" and then changes pcm_hw as e.g. "alsa_rt->hw.channels_max = OUT_N_CHANNELS;". I.e., not a copy.
And yes, it had me wonder if this was "allowed" in the first place as well (the same pcm_hw structure is also shared between playback and capture) but it does seem to work fine, so just unmarking it const seems a minimal fix or, better, "cleanup", since nothing complained, but the const qualifier no doubt at least /conceptually/ means it could be an issue.
I'll re-setup against the kernel as such and wait for reply on the above two comments to know what to re-submit.
Regards, Rene
On Tue, 21 Jul 2020 10:29:29 +0200, Ren9 Herman wrote:
On 21-07-2020 10:02, Takashi Iwai wrote:
The patch needs to point to the right path that is applicable with patch -p1 option, i.e. it should be like diff -up a/sound/usb/6fire/xxx.c b/sound/usb/6fire/xxx.c
At best use git for creating / submitting a patch.
Blast. I did, but worked against an isolated (DKMS-ed) version of the kernel driver. Pardon. Shall re-setup against the kernel as such and resend after dealing with your other two comments. I.e.,
Re: [PATCH 1/3] snd-usb-6fire: Move DMA-buffer off of the stack
Submit a complete set freshly as a v2 patch set.
[PATCH v2 1/3] ...
Could you rather change return with goto out (with ret variable set)? In that way we can see what actually you changed more clearly.
I already did exactly that though and in fact, the original not doing so is what makes the patch seem involved. With the added kmalloc() I change it so that all returns goto out, which kfree()s again. The only one that does not is when kmalloc() fails, i.e., when there's nothing to kfree(). I suppose you just misread and do not need to have that single one go through a goto as well?
The first return for kmalloc error should remain so, but the rest can be replaced with goto out. That I meant.
Re: [PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const
This must be superfluous. usb6fire_pcm_open() changes the field of the copied pcm_hw, not the original pcm_hw itself. Otherwise we must have got already a compile warning / error.
Unfortunately no; it's as mentioned in the cover letter accessed via pointer: usb6fire_pcm_open() sets "alsa_rt->hw = pcm_hw" and then changes pcm_hw as e.g. "alsa_rt->hw.channels_max = OUT_N_CHANNELS;". I.e., not a copy.
Note that it copies the whole instance, not the pointer. So the current code is correct.
thanks,
Takashi
On 21-07-2020 10:37, Takashi Iwai wrote:
Submit a complete set freshly as a v2 patch set.
[PATCH v2 1/3] ...
Done, although now only the first two are left since you were of course quite right about the third.
The first return for kmalloc error should remain so, but the rest can be replaced with goto out. That I meant.
I know, but that's exactly what it already did :) I guess it looks a bit non-generic as a patch due to me also unifying the error path due to that same change in structure. But any case, v2 is the same as v1 therefore, minus the fixed paths of course.
Re: [PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const
This must be superfluous. usb6fire_pcm_open() changes the field of the copied pcm_hw, not the original pcm_hw itself. Otherwise we must have got already a compile warning / error.
Unfortunately no; it's as mentioned in the cover letter accessed via pointer: usb6fire_pcm_open() sets "alsa_rt->hw = pcm_hw" and then changes pcm_hw as e.g. "alsa_rt->hw.channels_max = OUT_N_CHANNELS;". I.e., not a copy.
Note that it copies the whole instance, not the pointer. So the current code is correct.
Yap. I blindly assumed that snd_pcm_runtime.hw would be a pointer and synchronized that with the code without even really thinking about it by convincing myself that, as in the case of an array, that "pcm_hw" name would deteriorate to a pointer automatically. As such now keenly aware that my grasp of C semantics has also taken a bit of a nosedive over the last 10 years or so, I'll be more careful. Patch dropped.
Thanks.
Rene.
participants (2)
-
René Herman -
Takashi Iwai