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

[Ivan Orlov <ivan.orlov0322@xxxxxxxxx>]

On 7/29/24 15:02, Takashi Iwai wrote:
> 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)

Hi Takashi,

Thank you so much for the review.

> > +/*
> > + * 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).

Alright, I'll use __u64 here (initially I thought it is going to be more 
clear because it specifies the unit `period_size` is stored in, but I 
agree that it should be completely independent from pcm).

> > +	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.

Yeah, I don't think we are going to add anything else to the timers 
anytime soon... However, I'll probably add a small reserved space here 
(16 bytes, for instance), just in case we decide to add more parameters. 
Thanks!

> >   #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.

Ah, alright, I'll fix that in V3. Sorry, haven't noticed it :(

> > --- 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.

Ah, yes, I missed the static here. thanks!

> > +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.

Yeah, allowing zero division here (and overflows) is a very bad idea... 
I'll add some checks in V3.

> > +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.

Probably, I should use automatic cleanup here as well (as it is done in 
snd_timer_user_ginfo).

> > +static int snd_utimer_ioctl_create(struct file *file,
> > +				   struct snd_utimer_info __user *_utimer_info)
> > +{
> > +	return -EINVAL;
>
> Better to keep -ENOTTY?

Initial idea was that EINVAL here would say that the functionality is 
supported, but disabled in the config, but it seems like it breaks the 
convention this way so I'm going to change this to ENOTTY in the next 
version.

Thank you!
--
Kind regards,
Ivan Orlov