Re: [RFC PATCH] ALSA: compress_offload: introduce passthrough operation mode

[Takashi Iwai <tiwai@xxxxxxx>]

On Mon, 27 May 2024 09:11:33 +0200,
Jaroslav Kysela wrote:
> 
> There is a requirement to expose the audio hardware that accelerates various
> tasks for user space such as sample rate converters, compressed
> stream decoders, etc.
> 
> This is description for the API extension for the compress ALSA API which
> is able to handle "tasks" that are not bound to real-time operations
> and allows for the serialization of operations.
> 
> For details, refer to "compress-passthrough.rst" document.
> 
> Note: This code is RFC (not tested, just to clearify the API requirements).
> My goal is to add a test (loopback) driver and add a support to tinycompress
> library in the next step.
> 
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: Shengjiu Wang <shengjiu.wang@xxxxxxxxx>
> Cc: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
> Cc: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Cc: Vinod Koul <vkoul@xxxxxxxxxx>
> Signed-off-by: Jaroslav Kysela <perex@xxxxxxxx>
> ---
(snip)

Through a quick glance, the general idea looks simple enough and
straightforward.  If this satisfies the needs, we can go in this way.

A few subtle things:

- We might want to have a safe limit for the task creations.
  An evil user-space can fill up too easily.

- The error checks of memdup_user() are missing. 
  A caveat is that memdup_user() returns ERR_PTR() instead of NULL.
  So with the automatic cleanup, you'd need to fiddle with
  PTR_ERR(no_free_ptr(p)).

- The use of __u64 is rather for uapi headers.  The kernel internals
  can be with straight u64.

- The task list add/removal might need some locks.  It looks racy.
  Also the task runtime returned from snd_compr_find_task() might be
  freed.  Need for some locking or refcounting?

- A linear lookup is OK from performance POV?  Maybe enough for small
  number of tasks.

- Better to have a protocol version check and accept the R/W mode only
  with the newer version.


thanks,

Takashi