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:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
