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

[Takashi Iwai]

On Mon, 29 Jul 2024 10:59:04 +0200,
Ivan Orlov wrote:
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
(snip)
> +/*
> + * This structure describes the userspace-driven timer. Such timers are purely virtual,
> + * and can only be triggered from software (for instance, by userspace application).
> + */
> +struct snd_utimer_info {
> +	/*
> +	 * To pretend being a normal timer, we need to know the frame rate and
> +	 * the period size in frames.
> +	 */
> +	snd_pcm_uframes_t frame_rate;
> +	snd_pcm_uframes_t period_size;

The units in timer API should be independent from PCM.
So use the explicit type such as __u64 here (so that you don't need
the compat ioctl conversion, too).

> +	unsigned int id;
> +};

We often put some reserved fields for future extension.
But I'm not sure whether it's needed at this time for this kind of
simple interface, though.

>  #define SNDRV_TIMER_IOCTL_PVERSION	_IOR('T', 0x00, int)
>  #define SNDRV_TIMER_IOCTL_NEXT_DEVICE	_IOWR('T', 0x01, struct snd_timer_id)
>  #define SNDRV_TIMER_IOCTL_TREAD_OLD	_IOW('T', 0x02, int)
> @@ -990,6 +1005,8 @@ struct snd_timer_status {
>  #define SNDRV_TIMER_IOCTL_CONTINUE	_IO('T', 0xa2)
>  #define SNDRV_TIMER_IOCTL_PAUSE		_IO('T', 0xa3)
>  #define SNDRV_TIMER_IOCTL_TREAD64	_IOW('T', 0xa4, int)
> +#define SNDRV_TIMER_IOCTL_CREATE	_IOWR('T', 0xa5, struct snd_utimer_info)
> +#define SNDRV_TIMER_IOCTL_TRIGGER	_IO('T', 0xa6)

Once after adding the new API, don't forget to bump the protocol
version defined in SNDRV_TIMER_VERSION.

> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
(snip)
> +#ifdef CONFIG_SND_UTIMER
> +/*
> + * Since userspace-driven timers are passed to userspace, we need to have an identifier
> + * which will allow us to use them (basically, the subdevice number of udriven timer).
> + */
> +DEFINE_IDA(snd_utimer_ids);

Missing static.

> +static int snd_utimer_create(struct snd_utimer_info *utimer_info,
> +			     struct snd_utimer **r_utimer)
> +{
(snip)
> +	err = snd_timer_new(NULL, utimer->name, &tid, &timer);
> +	if (err < 0) {
> +		pr_err("Can't create userspace-driven timer\n");
> +		goto err_timer_new;
> +	}
> +
> +	timer->module = THIS_MODULE;
> +	timer->hw = timer_hw;
> +	timer->hw.resolution = NANO / utimer_info->frame_rate * utimer_info->period_size;

A sanity check is definitely needed for parameters like this.
e.g. you'd hit a zero-division Oops with this code.
Also, the resolution should be neither too small nor too high.

> +static int snd_utimer_ioctl_create(struct file *file,
> +				   struct snd_utimer_info __user *_utimer_info)
> +{
> +	struct snd_utimer *utimer;
> +	struct snd_utimer_info *utimer_info;
> +	int err;
> +
> +	utimer_info = memdup_user(_utimer_info, sizeof(*utimer_info));
> +	if (IS_ERR(utimer_info))
> +		return PTR_ERR(no_free_ptr(utimer_info));

no_free_ptr() is used only for the automatic cleanup stuff.

> +static int snd_utimer_ioctl_create(struct file *file,
> +				   struct snd_utimer_info __user *_utimer_info)
> +{
> +	return -EINVAL;

Better to keep -ENOTTY?


thanks,

Takashi