Re: Controlling the Tascam FW-1884

[Scott Bahling <sbahling@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3153 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]