Re: [PATCH v4 15/15] media: rc: Fix race condition during rc_register_device()

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 27/07/2026 15:18, Sean Young wrote:
> The correct sequence for rc core is so:
> 
>      rc_allocation_device()
> 
>      /*
>       * setup hardware and now calls to ir_raw_event_store etc are
>       * permitted, as well as rc_keydown. There is no rc device in sysfs
>       * or lirc chardev.
>       */
>      rc_register_device()
> 
>      /*
>       * After rc_register_device(), the rc device can be used now from lirc
>       * chardev, sysfs and IR is now decoded and reported.
>       */
> 
>      rc_unregister_device()
>      /*
>       * User space can no longer access /dev/lirc or the rc sysfs
>       * attributes. They will get -ENODEV if they still have a file
>       * descriptor open.
>       * Calls to ir_raw_event_handle() etc or rc_keydown() are permitted
>       * but they must stop before the call to rc_free_device(), so
>       * this is the time to stop the hardware.
>       */
>      rc_free_device()
> 
> This means that during rc_register_device(), we can get calls to
> ir_raw_event_{store,handle,overflow,store_with_filter}. Ensure this
> is done race-free.
> 
> Fixes: a3572c34da8d ("V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core")
> Signed-off-by: Sean Young <sean@mess.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/media/rc/rc-ir-raw.c | 20 +-------------------
>  drivers/media/rc/rc-main.c   | 19 +++++++++++--------
>  2 files changed, 12 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index b32f3cff9401..bebb45ffa971 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -71,9 +71,6 @@ static int ir_raw_event_thread(void *data)
>   */
>  int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev)
>  {
> -	if (!dev->raw)
> -		return -EINVAL;
> -
>  	dev_dbg(&dev->dev, "sample: (%05dus %s)\n",
>  		ev->duration, TO_STR(ev->pulse));
>  
> @@ -102,9 +99,6 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
>  	ktime_t			now;
>  	struct ir_raw_event	ev = {};
>  
> -	if (!dev->raw)
> -		return -EINVAL;
> -
>  	now = ktime_get();
>  	ev.duration = ktime_to_us(ktime_sub(now, dev->raw->last_event));
>  	ev.pulse = !pulse;
> @@ -129,9 +123,6 @@ int ir_raw_event_store_with_timeout(struct rc_dev *dev, struct ir_raw_event *ev)
>  	ktime_t		now;
>  	int		rc = 0;
>  
> -	if (!dev->raw)
> -		return -EINVAL;
> -
>  	now = ktime_get();
>  
>  	spin_lock(&dev->raw->edge_spinlock);
> @@ -166,9 +157,6 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store_with_timeout);
>   */
>  int ir_raw_event_store_with_filter(struct rc_dev *dev, struct ir_raw_event *ev)
>  {
> -	if (!dev->raw)
> -		return -EINVAL;
> -
>  	/* Ignore spaces in idle mode */
>  	if (dev->idle && !ev->pulse)
>  		return 0;
> @@ -200,9 +188,6 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store_with_filter);
>   */
>  void ir_raw_event_set_idle(struct rc_dev *dev, bool idle)
>  {
> -	if (!dev->raw)
> -		return;
> -
>  	dev_dbg(&dev->dev, "%s idle mode\n", idle ? "enter" : "leave");
>  
>  	if (idle) {
> @@ -226,7 +211,7 @@ EXPORT_SYMBOL_GPL(ir_raw_event_set_idle);
>   */
>  void ir_raw_event_handle(struct rc_dev *dev)
>  {
> -	if (!dev->raw || !dev->raw->thread)
> +	if (!dev->raw->thread)
>  		return;
>  
>  	wake_up_process(dev->raw->thread);
> @@ -612,9 +597,6 @@ EXPORT_SYMBOL(ir_raw_encode_carrier);
>   */
>  int ir_raw_event_prepare(struct rc_dev *dev)
>  {
> -	if (!dev)
> -		return -EINVAL;
> -
>  	dev->raw = kzalloc_obj(*dev->raw);
>  	if (!dev->raw)
>  		return -ENOMEM;
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index dda3479ea3ad..d93e98189c1a 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1701,14 +1701,24 @@ static const struct device_type rc_dev_type = {
>  struct rc_dev *rc_allocate_device(enum rc_driver_type type)
>  {
>  	struct rc_dev *dev;
> +	int ret;
>  
>  	dev = kzalloc_obj(*dev);
>  	if (!dev)
>  		return NULL;
>  
> +	if (type == RC_DRIVER_IR_RAW) {
> +		ret = ir_raw_event_prepare(dev);
> +		if (ret < 0) {
> +			kfree(dev);
> +			return NULL;
> +		}
> +	}
> +
>  	if (type != RC_DRIVER_IR_RAW_TX) {
>  		dev->input_dev = input_allocate_device();
>  		if (!dev->input_dev) {
> +			ir_raw_event_free(dev);
>  			kfree(dev);
>  			return NULL;
>  		}
> @@ -1724,6 +1734,7 @@ struct rc_dev *rc_allocate_device(enum rc_driver_type type)
>  		spin_lock_init(&dev->rc_map.lock);
>  		spin_lock_init(&dev->keylock);
>  	}
> +
>  	mutex_init(&dev->lock);
>  
>  	dev->dev.type = &rc_dev_type;
> @@ -1917,12 +1928,6 @@ int rc_register_device(struct rc_dev *dev)
>  		dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp;
>  	dev->sysfs_groups[attr++] = NULL;
>  
> -	if (dev->driver_type == RC_DRIVER_IR_RAW) {
> -		rc = ir_raw_event_prepare(dev);
> -		if (rc < 0)
> -			goto out_minor;
> -	}
> -
>  	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
>  		rc = rc_prepare_rx_device(dev);
>  		if (rc)

I don't see what the race condition is you are trying to solve. AFAICT the sysfs
files aren't created until the device_add() function that is right after this line.

I'm probably missing something, but that would mean that the commit log needs to
be improved.

Regards,

	Hans

> @@ -1979,8 +1984,6 @@ int rc_register_device(struct rc_dev *dev)
>  out_rx_free:
>  	ir_free_table(&dev->rc_map);
>  out_raw:
> -	ir_raw_event_free(dev);
> -out_minor:
>  	ida_free(&rc_ida, minor);
>  	return rc;
>  }