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

On Tue, Jul 28, 2026 at 11:35:36AM +0200, Hans Verkuil wrote:
> 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 <[email protected]>
> > Cc: [email protected]
> > ---
> >  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.

You're right, the commit message is terrible. I've reworded it for v5.


Sean