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
