Re: [PATCH 3/4] ALSA: timer: Introduce virtual userspace-driven timers

[Christophe JAILLET]

Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
> Implement two ioctl calls in order to support virtual userspace-driven
> ALSA timers.
>
> The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
> snd_utimer_info struct as a parameter and returns a file descriptor of
> a virtual timer. It also updates the `id` field of the snd_utimer_info
> struct, which provides a unique identifier for the timer (basically,
> the subdevice number which can be used when creating timer instances).
>
> This patch also introduces a tiny id allocator for the userspace-driven
> timers, which guarantees that we don't have more than 128 of them in the
> system.
>
> Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
> the virtual timer (and calls snd_timer_interrupt for the timer under
> the hood), causing all of the timer instances binded to this timer to
> execute their callbacks.
>
> The maximum amount of ticks available for the timer is 1 for the sake of
> simplification of the userspace API. 'start', 'stop', 'open' and 'close'
> callbacks for the userspace-driven timers are empty since we don't
> really do any hardware initialization here.
>
> Suggested-by: Axel Holzinger <aholzinger@xxxxxx>
> Signed-off-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx>
> ---

Hi,

...

> diff --git a/sound/core/Kconfig b/sound/core/Kconfig
> index b970a1734647..3cf82641fc67 100644
> --- a/sound/core/Kconfig
> +++ b/sound/core/Kconfig
> @@ -251,6 +251,17 @@ config SND_JACK_INJECTION_DEBUG
>   	  Say Y if you are debugging via jack injection interface.
>   	  If unsure select "N".
>   
> +config SND_UTIMER
> +	bool "Enable support for userspace-controlled virtual timers"
> +	depends on SND_TIMER
> +	help
> +	  Say Y to enable the support of userspace-controlled timers. These
> +	  timers are purely virtual, and they are supposed to be triggered
> +	  from userspace. They could be quite useful when synchronizing the
> +	  sound timing with userspace applications (for instance, when sending
> +	  data through snd-aloop).
> +

Unneeded extra new line.

> +
>   config SND_VMASTER
>   	bool
>   

...

> +static void snd_utimer_free(struct snd_utimer *utimer)
> +{
> +	snd_timer_free(utimer->timer);
> +	snd_utimer_put_id(utimer);

Missing kfree(utimer->name); ?

> +	kfree(utimer);
> +}

...

> +static int snd_utimer_create(struct snd_utimer_info *utimer_info,
> +			     struct snd_utimer **r_utimer)
> +{
> +	struct snd_utimer *utimer;
> +	struct snd_timer *timer;
> +	struct snd_timer_id tid;
> +	int utimer_id;
> +	int err = 0;
> +	char *timer_name;
> +
> +	utimer = kzalloc(sizeof(*utimer), GFP_KERNEL);
> +	if (!utimer)
> +		return -ENOMEM;
> +
> +	timer_name = kzalloc(SNDRV_UTIMER_NAME_LEN, GFP_KERNEL);

kasprintf(GFP_KERNEL, "snd-utimer%d", utimer_id); ?
and SNDRV_UTIMER_NAME_LEN becomes useless too.

In snd_timer_new() it is copied in a char[64] anyway, and if utimer_id 
is small, we could even save a few bytes of memory.

CJ

> +	if (!timer_name) {
> +		kfree(utimer);
> +		return -ENOMEM;
> +	}
> +
> +	/* We hold the ioctl lock here so we won't get a race condition when allocating id */
> +	utimer_id = snd_utimer_take_id();
> +	if (utimer_id < 0) {
> +		err = utimer_id;
> +		goto err_take_id;
> +	}
> +
> +	sprintf(timer_name, "snd-utimer%d", utimer_id);
> +	utimer->name = timer_name;
> +	utimer->id = utimer_id;

...