Re: [alsa-devel] Controlling the Tascam FW-1884

On Tue, 2018-10-30 at 18:34 +0900, Takashi Sakamoto wrote:

Hi Scott,

Sorry to be late for reply, but I was a bit busy with travel for Audio miniconference 2018 and tests for v4.20 merge window.

On 2018/10/22 20:47, Scott Bahling wrote:

...

I have tried this, but the endianness of the status bits passed via this method seems to be wrong.

I noticed in your latest kernel code[1], that you don't convert the firewire data to local CPU endian when storing in the "after" variable which ends up getting pushed into the status structure as big endian:

 after = buffer[s->data_block_quadlets - 1];

... ... ... tscm->status[index] = after;

Shouldn't that be:

 after = be32_to_cpu(buffer[s->data_block_quadlets - 1]);

which also would remove the need for later conversions in that block of code?

I have a tested patch which I will send as a pull request once github is functioning again.

Ah, indeed. I completely overlooked it, sorry...

At first place, I just focused on reduction of CPU usage in packet processing, then cache big-endianness values from packets. Queued events have converted host-endiannness value from the cache. On the other hand, data returned by SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS ioctl is just copied from the cache, then it's still big-endiannness, as you found.

Thanks for your PR[1] and it works well. But here I suggest that we can judge the frequency to call SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS ioctl is surely less than the frequency on packet processing. If we attempt to keep low CPU usage in the packet processing, current implementation can be kept as is. Instead, a patch for handler of the ioctl is worth for commit.

Makes perfect sense.

For example, This patch will fix the issue.

$ git diff
diff --git a/sound/firewire/tascam/tascam-hwdep.c 
b/sound/firewire/tascam/tascam-hwdep.c
index ad164ad..dff7dc4 100644
--- a/sound/firewire/tascam/tascam-hwdep.c
+++ b/sound/firewire/tascam/tascam-hwdep.c
@@ -189,10 +189,23 @@ static int hwdep_unlock(struct snd_tscm *tscm)

  static int tscm_hwdep_status(struct snd_tscm *tscm, void __user
*arg)
  {
-       if (copy_to_user(arg, tscm->status, sizeof(tscm->status)))
-               return -EFAULT;
+       struct snd_firewire_tascam_status *status;
+       int i;
+       int err = 0;

-       return 0;
+       status = kmalloc(sizeof(*status), GFP_KERNEL);
+       if (!status)
+               return -ENOMEM;
+
+       for (i = 0; i < SNDRV_FIREWIRE_TASCAM_STATUS_COUNT; ++i)
+               status->data[i] = be32_to_cpu(tscm->status[i]);
+
+       if (copy_to_user(arg, status, sizeof(*status)))
+               err = -EFAULT;
+
+       kfree(status);
+
+       return err;
  }

I'd like you to test with this patch, before granting your PR.

I have tested and the patch above works as expected.

-Scott