| Message ID | 20240806125243.449959-4-ivan.orlov0322@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce userspace-driven ALSA timers | expand |
On 06. 08. 24 14:52, Ivan Orlov wrote: > Implement two ioctl calls in order to support virtual userspace-driven > ALSA timers. ... > +struct snd_utimer_info { > + /* > + * To pretend being a normal timer, we need to know the frame rate and > + * the period size in frames. > + */ > + __u64 frame_rate; > + __u64 period_size; There should be just one timer resolution in ns member (like in struct snd_timer_ginfo - not frame/period members here - it's too specific). The resolution can be calculated in the user space from the rate and period size. Also naming - the timer API uses snd_timer prefix for structures, thus snd_timer_uinfo should be more appropriate. Jaroslav
On 8/6/24 14:11, Jaroslav Kysela wrote: > On 06. 08. 24 14:52, Ivan Orlov wrote: >> Implement two ioctl calls in order to support virtual userspace-driven >> ALSA timers. > > ... > Hi Jaroslav, >> +struct snd_utimer_info { >> + /* >> + * To pretend being a normal timer, we need to know the frame >> rate and >> + * the period size in frames. >> + */ >> + __u64 frame_rate; >> + __u64 period_size; > > There should be just one timer resolution in ns member (like in struct > snd_timer_ginfo - not frame/period members here - it's too specific). > The resolution can be calculated in the user space from the rate and > period size. > Ah, yes, I agree... Also, it should help us to avoid complex calculations and sanity checks in the kernel space. I'll replace these two fields with 'resolution' field in V4, thanks! > Also naming - the timer API uses snd_timer prefix for structures, thus > snd_timer_uinfo should be more appropriate. > Alright, I'll rename the structure. Thank you so much for the review!
Hi Ivan, kernel test robot noticed the following build errors: [auto build test ERROR on tiwai-sound/for-next] [also build test ERROR on tiwai-sound/for-linus linus/master v6.11-rc2 next-20240808] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Ivan-Orlov/ALSA-aloop-Allow-using-global-timers/20240806-210332 base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next patch link: https://lore.kernel.org/r/20240806125243.449959-4-ivan.orlov0322%40gmail.com patch subject: [PATCH v3 3/4] ALSA: timer: Introduce virtual userspace-driven timers config: arm-randconfig-r064-20240807 (https://download.01.org/0day-ci/archive/20240808/202408081612.hvfwzVlB-lkp@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081612.hvfwzVlB-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408081612.hvfwzVlB-lkp@intel.com/ All errors (new ones prefixed by >>): arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_submit_wmt_recv_urb': drivers/bluetooth/btmtk.c:531:(.text+0x6c2): undefined reference to `usb_alloc_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:537:(.text+0x6e6): undefined reference to `usb_free_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:561:(.text+0x750): undefined reference to `usb_anchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:562:(.text+0x75a): undefined reference to `usb_submit_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:567:(.text+0x796): undefined reference to `usb_unanchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:570:(.text+0x7a0): undefined reference to `usb_free_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_submit_intr_urb': drivers/bluetooth/btmtk.c:1174:(.text+0x80e): undefined reference to `usb_alloc_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:1181:(.text+0x83a): undefined reference to `usb_free_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:1195:(.text+0x8c4): undefined reference to `usb_anchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:1197:(.text+0x8cc): undefined reference to `usb_submit_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:1202:(.text+0x908): undefined reference to `usb_unanchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:1205:(.text+0x912): undefined reference to `usb_free_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_suspend': drivers/bluetooth/btmtk.c:1265:(.text+0x988): undefined reference to `usb_kill_anchored_urbs' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_intr_complete': drivers/bluetooth/btmtk.c:1145:(.text+0xbf2): undefined reference to `usb_anchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:1147:(.text+0xbfc): undefined reference to `usb_submit_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:1157:(.text+0xc48): undefined reference to `usb_unanchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `__set_mtk_intr_interface': drivers/bluetooth/btmtk.c:991:(.text+0xcf4): undefined reference to `usb_set_interface' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_isointf_init': drivers/bluetooth/btmtk.c:1224:(.text+0xd30): undefined reference to `usb_kill_anchored_urbs' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_hci_wmt_sync': drivers/bluetooth/btmtk.c:610:(.text+0xf26): undefined reference to `usb_autopm_get_interface' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:618:(.text+0xf54): undefined reference to `usb_autopm_put_interface' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:625:(.text+0xf7e): undefined reference to `usb_autopm_put_interface' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_reg_read': drivers/bluetooth/btmtk.c:790:(.text+0x1248): undefined reference to `usb_control_msg' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_uhw_reg_write': drivers/bluetooth/btmtk.c:738:(.text+0x12e0): undefined reference to `usb_control_msg' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_uhw_reg_read': drivers/bluetooth/btmtk.c:761:(.text+0x1374): undefined reference to `usb_control_msg' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_recv_acl': drivers/bluetooth/btmtk.c:946:(.text+0x1e06): undefined reference to `usb_disable_autosuspend' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `btmtk_usb_wmt_recv': drivers/bluetooth/btmtk.c:508:(.text+0x1e72): undefined reference to `usb_anchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:509:(.text+0x1e7c): undefined reference to `usb_submit_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.c:518:(.text+0x1ec0): undefined reference to `usb_unanchor_urb' arm-linux-gnueabi-ld: drivers/bluetooth/btmtk.o: in function `alloc_mtk_intr_urb': drivers/bluetooth/btmtk.c:1037:(.text+0x1fc8): undefined reference to `usb_alloc_urb' arm-linux-gnueabi-ld: sound/core/timer.o: in function `snd_utimer_create': >> sound/core/timer.c:2127:(.text+0x255c): undefined reference to `__aeabi_uldivmod' vim +2127 sound/core/timer.c 2112 2113 static int snd_utimer_create(struct snd_utimer_info *utimer_info, 2114 struct snd_utimer **r_utimer) 2115 { 2116 struct snd_utimer *utimer; 2117 struct snd_timer *timer; 2118 struct snd_timer_id tid; 2119 int utimer_id; 2120 int err = 0; 2121 u64 resolution; 2122 2123 if (!utimer_info || utimer_info->frame_rate == 0 || utimer_info->period_size == 0) 2124 return -EINVAL; 2125 2126 /* Let's check that we won't get an overflow in the resolution */ > 2127 if (NANO / utimer_info->frame_rate > U64_MAX / utimer_info->period_size) 2128 return -EINVAL; 2129 2130 resolution = NANO / utimer_info->frame_rate * utimer_info->period_size; 2131 2132 if (resolution == 0) 2133 return -EINVAL; 2134 2135 utimer = kzalloc(sizeof(*utimer), GFP_KERNEL); 2136 if (!utimer) 2137 return -ENOMEM; 2138 2139 /* We hold the ioctl lock here so we won't get a race condition when allocating id */ 2140 utimer_id = snd_utimer_take_id(); 2141 if (utimer_id < 0) { 2142 err = utimer_id; 2143 goto err_take_id; 2144 } 2145 2146 utimer->name = kasprintf(GFP_KERNEL, "snd-utimer%d", utimer_id); 2147 if (!utimer->name) { 2148 err = -ENOMEM; 2149 goto err_get_name; 2150 } 2151 2152 utimer->id = utimer_id; 2153 2154 tid.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION; 2155 tid.dev_class = SNDRV_TIMER_CLASS_GLOBAL; 2156 tid.card = -1; 2157 tid.device = SNDRV_TIMER_GLOBAL_UDRIVEN; 2158 tid.subdevice = utimer_id; 2159 2160 err = snd_timer_new(NULL, utimer->name, &tid, &timer); 2161 if (err < 0) { 2162 pr_err("Can't create userspace-driven timer\n"); 2163 goto err_timer_new; 2164 } 2165 2166 timer->module = THIS_MODULE; 2167 timer->hw = timer_hw; 2168 timer->hw.resolution = resolution; 2169 timer->hw.ticks = 1; 2170 timer->max_instances = MAX_SLAVE_INSTANCES; 2171 2172 utimer->timer = timer; 2173 2174 err = snd_timer_global_register(timer); 2175 if (err < 0) { 2176 pr_err("Can't register a userspace-driven timer\n"); 2177 goto err_timer_reg; 2178 } 2179 2180 *r_utimer = utimer; 2181 return 0; 2182 2183 err_timer_reg: 2184 snd_timer_free(timer); 2185 err_timer_new: 2186 kfree(utimer->name); 2187 err_get_name: 2188 snd_utimer_put_id(utimer); 2189 err_take_id: 2190 kfree(utimer); 2191 2192 return err; 2193 } 2194
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 8bf7e8a0eb6f..cdd9cf8f71c1 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -869,7 +869,7 @@ struct snd_ump_block_info { * Timer section - /dev/snd/timer */ -#define SNDRV_TIMER_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 7) +#define SNDRV_TIMER_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 8) enum { SNDRV_TIMER_CLASS_NONE = -1, @@ -894,6 +894,7 @@ enum { #define SNDRV_TIMER_GLOBAL_RTC 1 /* unused */ #define SNDRV_TIMER_GLOBAL_HPET 2 #define SNDRV_TIMER_GLOBAL_HRTIMER 3 +#define SNDRV_TIMER_GLOBAL_UDRIVEN 4 /* info flags */ #define SNDRV_TIMER_FLG_SLAVE (1<<0) /* cannot be controlled */ @@ -974,6 +975,21 @@ struct snd_timer_status { }; #endif +/* + * 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. + */ + __u64 frame_rate; + __u64 period_size; + unsigned int id; + unsigned char reserved[16]; +}; + #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 +1006,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) #if __BITS_PER_LONG == 64 #define SNDRV_TIMER_IOCTL_TREAD SNDRV_TIMER_IOCTL_TREAD_OLD diff --git a/sound/core/Kconfig b/sound/core/Kconfig index b970a1734647..670b26cf3065 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -251,6 +251,16 @@ 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). + config SND_VMASTER bool diff --git a/sound/core/timer.c b/sound/core/timer.c index d104adc75a8b..dabb925625f1 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -13,6 +13,9 @@ #include <linux/module.h> #include <linux/string.h> #include <linux/sched/signal.h> +#include <linux/anon_inodes.h> +#include <linux/idr.h> +#include <linux/units.h> #include <sound/core.h> #include <sound/timer.h> #include <sound/control.h> @@ -109,6 +112,16 @@ struct snd_timer_status64 { unsigned char reserved[64]; /* reserved */ }; +#ifdef CONFIG_SND_UTIMER +#define SNDRV_UTIMERS_MAX_COUNT 128 +/* Internal data structure for keeping the state of the userspace-driven timer */ +struct snd_utimer { + char *name; + struct snd_timer *timer; + unsigned int id; +}; +#endif + #define SNDRV_TIMER_IOCTL_STATUS64 _IOR('T', 0x14, struct snd_timer_status64) /* list of timers */ @@ -2009,6 +2022,212 @@ enum { SNDRV_TIMER_IOCTL_PAUSE_OLD = _IO('T', 0x23), }; +#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). + */ +static DEFINE_IDA(snd_utimer_ids); + +static void snd_utimer_put_id(struct snd_utimer *utimer) +{ + int timer_id = utimer->id; + + snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT); + ida_free(&snd_utimer_ids, timer_id); +} + +static int snd_utimer_take_id(void) +{ + return ida_alloc_max(&snd_utimer_ids, SNDRV_UTIMERS_MAX_COUNT - 1, GFP_KERNEL); +} + +static void snd_utimer_free(struct snd_utimer *utimer) +{ + snd_timer_free(utimer->timer); + snd_utimer_put_id(utimer); + kfree(utimer->name); + kfree(utimer); +} + +static int snd_utimer_release(struct inode *inode, struct file *file) +{ + struct snd_utimer *utimer = (struct snd_utimer *)file->private_data; + + snd_utimer_free(utimer); + return 0; +} + +static int snd_utimer_trigger(struct file *file) +{ + struct snd_utimer *utimer = (struct snd_utimer *)file->private_data; + + snd_timer_interrupt(utimer->timer, utimer->timer->sticks); + return 0; +} + +static long snd_utimer_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) +{ + switch (ioctl) { + case SNDRV_TIMER_IOCTL_TRIGGER: + return snd_utimer_trigger(file); + } + + return -ENOTTY; +} + +static const struct file_operations snd_utimer_fops = { + .llseek = noop_llseek, + .release = snd_utimer_release, + .unlocked_ioctl = snd_utimer_ioctl, +}; + +static int snd_utimer_start(struct snd_timer *t) +{ + return 0; +} + +static int snd_utimer_stop(struct snd_timer *t) +{ + return 0; +} + +static int snd_utimer_open(struct snd_timer *t) +{ + return 0; +} + +static int snd_utimer_close(struct snd_timer *t) +{ + return 0; +} + +static const struct snd_timer_hardware timer_hw = { + .flags = SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_WORK, + .open = snd_utimer_open, + .close = snd_utimer_close, + .start = snd_utimer_start, + .stop = snd_utimer_stop, +}; + +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; + u64 resolution; + + if (!utimer_info || utimer_info->frame_rate == 0 || utimer_info->period_size == 0) + return -EINVAL; + + /* Let's check that we won't get an overflow in the resolution */ + if (NANO / utimer_info->frame_rate > U64_MAX / utimer_info->period_size) + return -EINVAL; + + resolution = NANO / utimer_info->frame_rate * utimer_info->period_size; + + if (resolution == 0) + return -EINVAL; + + utimer = kzalloc(sizeof(*utimer), GFP_KERNEL); + if (!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; + } + + utimer->name = kasprintf(GFP_KERNEL, "snd-utimer%d", utimer_id); + if (!utimer->name) { + err = -ENOMEM; + goto err_get_name; + } + + utimer->id = utimer_id; + + tid.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION; + tid.dev_class = SNDRV_TIMER_CLASS_GLOBAL; + tid.card = -1; + tid.device = SNDRV_TIMER_GLOBAL_UDRIVEN; + tid.subdevice = utimer_id; + + 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 = resolution; + timer->hw.ticks = 1; + timer->max_instances = MAX_SLAVE_INSTANCES; + + utimer->timer = timer; + + err = snd_timer_global_register(timer); + if (err < 0) { + pr_err("Can't register a userspace-driven timer\n"); + goto err_timer_reg; + } + + *r_utimer = utimer; + return 0; + +err_timer_reg: + snd_timer_free(timer); +err_timer_new: + kfree(utimer->name); +err_get_name: + snd_utimer_put_id(utimer); +err_take_id: + kfree(utimer); + + return err; +} + +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 __free(kfree) = NULL; + int err; + + utimer_info = memdup_user(_utimer_info, sizeof(*utimer_info)); + if (IS_ERR(utimer_info)) + return PTR_ERR(no_free_ptr(utimer_info)); + + err = snd_utimer_create(utimer_info, &utimer); + if (err < 0) + return err; + + utimer_info->id = utimer->id; + + err = copy_to_user(_utimer_info, utimer_info, sizeof(*utimer_info)); + if (err) { + snd_utimer_free(utimer); + return -EFAULT; + } + + return anon_inode_getfd(utimer->name, &snd_utimer_fops, utimer, O_RDWR | O_CLOEXEC); +} + +#else + +static int snd_utimer_ioctl_create(struct file *file, + struct snd_utimer_info __user *_utimer_info) +{ + return -ENOTTY; +} + +#endif + static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd, unsigned long arg, bool compat) { @@ -2053,6 +2272,8 @@ static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd, case SNDRV_TIMER_IOCTL_PAUSE: case SNDRV_TIMER_IOCTL_PAUSE_OLD: return snd_timer_user_pause(file); + case SNDRV_TIMER_IOCTL_CREATE: + return snd_utimer_ioctl_create(file, argp); } return -ENOTTY; }
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@gmx.de> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> --- V1 -> V2: - Add missing kfree for the utimer name in snd_utimer_free - Remove extra newline in sound core Kconfig - Use IDA allocator API to allocate utimer ids - Use kasprintf for the timer name instead of kzalloc + sprintf V2 -> V3: - Use __u64 instead of snd_pcm_uframes_t in snd_utimer_info struct - Add 16 reserved bytes to snd_utimer_info struct just in case we decide to add some other fields to this struct - Bump the timer protocol version in SNDRV_TIMER_VERSION to 2.0.8 - Make the 'snd_utimer_ids' variable static - Add sanity checks to the 'snd_utimer_create' function: 0-check for period size and frame rate, overflow and 0- checks for resolution - Use automatic cleanup in 'snd_utimer_ioctl_create' - Return -ENOTTY instead of -EINVAL from ioctl if userspace-driven timers are disabled include/uapi/sound/asound.h | 20 +++- sound/core/Kconfig | 10 ++ sound/core/timer.c | 221 ++++++++++++++++++++++++++++++++++++ 3 files changed, 250 insertions(+), 1 deletion(-)