diff mbox

rc-core: cleanup rc_register_device (v2)

Message ID 149380584051.16088.1242474111722854646.stgit@zeus.hardeman.nu (mailing list archive)
State New, archived
Headers show

Commit Message

David Härdeman May 3, 2017, 10:04 a.m. UTC 
The device core infrastructure is based on the presumption that
once a driver calls device_add(), it must be ready to accept
userspace interaction.

This requires splitting rc_setup_rx_device() into two functions
and reorganizing rc_register_device() so that as much work
as possible is performed before calling device_add().

Version 2: switch the order in which rc_prepare_rx_device() and
ir_raw_event_prepare() gets called so that dev->change_protocol()
gets called before device_add().

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/rc-core-priv.h |    2 +
 drivers/media/rc/rc-ir-raw.c    |   34 ++++++++++++------
 drivers/media/rc/rc-main.c      |   75 +++++++++++++++++++++++++--------------
 3 files changed, 73 insertions(+), 38 deletions(-)

Comments

Sean Young May 17, 2017, 8:09 p.m. UTC | #1 
Hi David,

On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
> The device core infrastructure is based on the presumption that
> once a driver calls device_add(), it must be ready to accept
> userspace interaction.
> 
> This requires splitting rc_setup_rx_device() into two functions
> and reorganizing rc_register_device() so that as much work
> as possible is performed before calling device_add().
> 
> Version 2: switch the order in which rc_prepare_rx_device() and
> ir_raw_event_prepare() gets called so that dev->change_protocol()
> gets called before device_add().

I've looked at this patch and I don't see what problem it solves;
what user-space interaction is problematic?


Sean

> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/rc-core-priv.h |    2 +
>  drivers/media/rc/rc-ir-raw.c    |   34 ++++++++++++------
>  drivers/media/rc/rc-main.c      |   75 +++++++++++++++++++++++++--------------
>  3 files changed, 73 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index 0455b273c2fc..b3e7cac2c3ee 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int max,
>   * Routines from rc-raw.c to be used internally and by decoders
>   */
>  u64 ir_raw_get_allowed_protocols(void);
> +int ir_raw_event_prepare(struct rc_dev *dev);
>  int ir_raw_event_register(struct rc_dev *dev);
> +void ir_raw_event_free(struct rc_dev *dev);
>  void ir_raw_event_unregister(struct rc_dev *dev);
>  int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
>  void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 90f66dc7c0d7..ae7785c4fbe7 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode);
>  /*
>   * Used to (un)register raw event clients
>   */
> -int ir_raw_event_register(struct rc_dev *dev)
> +int ir_raw_event_prepare(struct rc_dev *dev)
>  {
> -	int rc;
> -	struct ir_raw_handler *handler;
> +	static bool raw_init; /* 'false' default value, raw decoders loaded? */
>  
>  	if (!dev)
>  		return -EINVAL;
>  
> +	if (!raw_init) {
> +		request_module("ir-lirc-codec");
> +		raw_init = true;
> +	}
> +
>  	dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
>  	if (!dev->raw)
>  		return -ENOMEM;
> @@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev)
>  	dev->change_protocol = change_protocol;
>  	INIT_KFIFO(dev->raw->kfifo);
>  
> +	return 0;
> +}
> +
> +int ir_raw_event_register(struct rc_dev *dev)
> +{
> +	struct ir_raw_handler *handler;
> +
>  	/*
>  	 * raw transmitters do not need any event registration
>  	 * because the event is coming from userspace
> @@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev)
>  		dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
>  					       "rc%u", dev->minor);
>  
> -		if (IS_ERR(dev->raw->thread)) {
> -			rc = PTR_ERR(dev->raw->thread);
> -			goto out;
> -		}
> +		if (IS_ERR(dev->raw->thread))
> +			return PTR_ERR(dev->raw->thread);
>  	}
>  
>  	mutex_lock(&ir_raw_handler_lock);
> @@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev)
>  	mutex_unlock(&ir_raw_handler_lock);
>  
>  	return 0;
> +}
> +
> +void ir_raw_event_free(struct rc_dev *dev)
> +{
> +	if (!dev)
> +		return;
>  
> -out:
>  	kfree(dev->raw);
>  	dev->raw = NULL;
> -	return rc;
>  }
>  
>  void ir_raw_event_unregister(struct rc_dev *dev)
> @@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
>  			handler->raw_unregister(dev);
>  	mutex_unlock(&ir_raw_handler_lock);
>  
> -	kfree(dev->raw);
> -	dev->raw = NULL;
> +	ir_raw_event_free(dev);
>  }
>  
>  /*
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 802e559cc30e..f3bc9f4e2b96 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
>  
> -static int rc_setup_rx_device(struct rc_dev *dev)
> +static int rc_prepare_rx_device(struct rc_dev *dev)
>  {
>  	int rc;
>  	struct rc_map *rc_map;
> @@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev)
>  	dev->input_dev->phys = dev->input_phys;
>  	dev->input_dev->name = dev->input_name;
>  
> +	return 0;
> +
> +out_table:
> +	ir_free_table(&dev->rc_map);
> +
> +	return rc;
> +}
> +
> +static int rc_setup_rx_device(struct rc_dev *dev)
> +{
> +	int rc;
> +
>  	/* rc_open will be called here */
>  	rc = input_register_device(dev->input_dev);
>  	if (rc)
> -		goto out_table;
> +		return rc;
>  
>  	/*
>  	 * Default delay of 250ms is too short for some protocols, especially
> @@ -1729,27 +1741,23 @@ static int rc_setup_rx_device(struct rc_dev *dev)
>  	dev->input_dev->rep[REP_PERIOD] = 125;
>  
>  	return 0;
> -
> -out_table:
> -	ir_free_table(&dev->rc_map);
> -
> -	return rc;
>  }
>  
>  static void rc_free_rx_device(struct rc_dev *dev)
>  {
> -	if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX)
> +	if (!dev)
>  		return;
>  
> -	ir_free_table(&dev->rc_map);
> +	if (dev->input_dev) {
> +		input_unregister_device(dev->input_dev);
> +		dev->input_dev = NULL;
> +	}
>  
> -	input_unregister_device(dev->input_dev);
> -	dev->input_dev = NULL;
> +	ir_free_table(&dev->rc_map);
>  }
>  
>  int rc_register_device(struct rc_dev *dev)
>  {
> -	static bool raw_init; /* 'false' default value, raw decoders loaded? */
>  	const char *path;
>  	int attr = 0;
>  	int minor;
> @@ -1776,30 +1784,39 @@ 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 ||
> +	    dev->driver_type == RC_DRIVER_IR_RAW_TX) {
> +		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)
> +			goto out_raw;
> +	}
> +
>  	rc = device_add(&dev->dev);
>  	if (rc)
> -		goto out_unlock;
> +		goto out_rx_free;
>  
>  	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
>  	dev_info(&dev->dev, "%s as %s\n",
>  		dev->input_name ?: "Unspecified device", path ?: "N/A");
>  	kfree(path);
>  
> +	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
> +		rc = rc_setup_rx_device(dev);
> +		if (rc)
> +			goto out_dev;
> +	}
> +
>  	if (dev->driver_type == RC_DRIVER_IR_RAW ||
>  	    dev->driver_type == RC_DRIVER_IR_RAW_TX) {
> -		if (!raw_init) {
> -			request_module_nowait("ir-lirc-codec");
> -			raw_init = true;
> -		}
>  		rc = ir_raw_event_register(dev);
>  		if (rc < 0)
> -			goto out_dev;
> -	}
> -
> -	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
> -		rc = rc_setup_rx_device(dev);
> -		if (rc)
> -			goto out_raw;
> +			goto out_rx;
>  	}
>  
>  	/* Allow the RC sysfs nodes to be accessible */
> @@ -1811,11 +1828,15 @@ int rc_register_device(struct rc_dev *dev)
>  
>  	return 0;
>  
> -out_raw:
> -	ir_raw_event_unregister(dev);
> +out_rx:
> +	rc_free_rx_device(dev);
>  out_dev:
>  	device_del(&dev->dev);
> -out_unlock:
> +out_rx_free:
> +	ir_free_table(&dev->rc_map);
> +out_raw:
> +	ir_raw_event_free(dev);
> +out_minor:
>  	ida_simple_remove(&rc_ida, minor);
>  	return rc;
>  }
David Härdeman May 18, 2017, 7:55 a.m. UTC | #2 
On Wed, May 17, 2017 at 09:09:57PM +0100, Sean Young wrote:
>Hi David,
>
>On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
>> The device core infrastructure is based on the presumption that
>> once a driver calls device_add(), it must be ready to accept
>> userspace interaction.
>> 
>> This requires splitting rc_setup_rx_device() into two functions
>> and reorganizing rc_register_device() so that as much work
>> as possible is performed before calling device_add().
>> 
>> Version 2: switch the order in which rc_prepare_rx_device() and
>> ir_raw_event_prepare() gets called so that dev->change_protocol()
>> gets called before device_add().
>
>I've looked at this patch and I don't see what problem it solves;
>what user-space interaction is problematic?

It's a preparatory patch, the next patch ("rc-core: cleanup
rc_register_device pt2") is the one which removes the dev->initialized
hack (which currently papers over the fact that device_add() is called
before userspace is actually ready to accept sysfs interaction).

Does that answer your question?

//David
Sean Young May 18, 2017, 9:42 a.m. UTC | #3 
On Thu, May 18, 2017 at 09:55:14AM +0200, David Härdeman wrote:
> On Wed, May 17, 2017 at 09:09:57PM +0100, Sean Young wrote:
> >Hi David,
> >
> >On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
> >> The device core infrastructure is based on the presumption that
> >> once a driver calls device_add(), it must be ready to accept
> >> userspace interaction.
> >> 
> >> This requires splitting rc_setup_rx_device() into two functions
> >> and reorganizing rc_register_device() so that as much work
> >> as possible is performed before calling device_add().
> >> 
> >> Version 2: switch the order in which rc_prepare_rx_device() and
> >> ir_raw_event_prepare() gets called so that dev->change_protocol()
> >> gets called before device_add().
> >
> >I've looked at this patch and I don't see what problem it solves;
> >what user-space interaction is problematic?
> 
> It's a preparatory patch, the next patch ("rc-core: cleanup
> rc_register_device pt2") is the one which removes the dev->initialized
> hack (which currently papers over the fact that device_add() is called
> before userspace is actually ready to accept sysfs interaction).
> 
> Does that answer your question?

I suspected that but the commit message does not make it obvious.


Sean
Sean Young May 20, 2017, 11:10 a.m. UTC | #4 
On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
> The device core infrastructure is based on the presumption that
> once a driver calls device_add(), it must be ready to accept
> userspace interaction.
> 
> This requires splitting rc_setup_rx_device() into two functions
> and reorganizing rc_register_device() so that as much work
> as possible is performed before calling device_add().
> 
> Version 2: switch the order in which rc_prepare_rx_device() and
> ir_raw_event_prepare() gets called so that dev->change_protocol()
> gets called before device_add().

With this patch applied, when I plug in an iguanair usb device, I get.

(The raw rc thread has not been started when the input device is registered.)

[   65.875642] usb 7-1.3: new low-speed USB device number 6 using uhci_hcd
[   66.004105] usb 7-1.3: New USB device found, idVendor=1781, idProduct=0938
[   66.004111] usb 7-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   66.004116] usb 7-1.3: Product: USB IR Transceiver
[   66.004120] usb 7-1.3: Manufacturer: IguanaWorks
[   66.057190] Registered IR keymap rc-rc6-mce
[   66.057328] rc rc1: IguanaWorks USB IR Transceiver version 0x0308 as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1.3/7-1.3:1.0/rc/rc1
[   66.057423] input: IguanaWorks USB IR Transceiver version 0x0308 as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1.3/7-1.3:1.0/rc/rc1/input24
[   66.057445] BUG: unable to handle kernel NULL pointer dereference at 0000000000000ba0
[   66.057500] IP: __lock_acquire+0x122/0x12e0
[   66.057522] PGD 0 
[   66.057523] P4D 0 

[   66.057556] Oops: 0000 [#1] SMP
[   66.057573] Modules linked in: iguanair(+) mceusb xt_CHECKSUM ipt_MASQUERADE nf_nat_masquerade_ipv4 tun fuse nf_conntrack_netbios_ns nf_conntrack_broadcast xt_CT ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_security ip6table_mangle ip6table_raw iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack libcrc32c iptable_security iptable_mangle iptable_raw ebtable_filter ebtables ip6table_filter ip6_tables snd_hda_codec_idt snd_hda_codec_generic tuner_simple tuner_types wm8775 tda9887 tda8290 tuner coretemp kvm_intel ppdev cx25840 mei_wdt kvm iTCO_wdt iTCO_vendor_support ivtv snd_hda_codec_hdmi irqbypass snd_hda_intel joydev snd_hda_codec tveeprom
[   66.057945]  cx2341x v4l2_common snd_hda_core pcspkr videodev snd_hwdep i2c_i801 snd_seq media ir_rc6_decoder snd_seq_device rc_rc6_mce ir_lirc_codec snd_pcm lirc_dev winbond_cir rc_core mei_me snd_timer snd shpchp mei video parport_pc nfsd soundcore parport acpi_cpufreq tpm_tis tpm_tis_core tpm lpc_ich auth_rpcgss nfs_acl lockd grace sunrpc amdkfd amd_iommu_v2 amdgpu i2c_algo_bit drm_kms_helper syscopyarea sysfillrect e1000e sysimgblt fb_sys_fops ttm hid_sjoy ff_memless firewire_ohci firewire_core drm serio_raw ata_generic pata_acpi crc_itu_t ptp pps_core
[   66.058080] CPU: 3 PID: 2092 Comm: systemd-udevd Not tainted 4.12.0-rc1+ #1
[   66.058080] Hardware name:                  /DG45ID, BIOS IDG4510H.86A.0135.2011.0225.1100 02/25/2011
[   66.058080] task: ffff88bb9e34c000 task.stack: ffffa014c071c000
[   66.058080] RIP: 0010:__lock_acquire+0x122/0x12e0
[   66.058080] RSP: 0018:ffffa014c071f720 EFLAGS: 00010002
[   66.058080] RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000000
[   66.058080] RDX: 0000000000000046 RSI: 0000000000000000 RDI: 0000000000000000
[   66.058080] RBP: ffffa014c071f7e0 R08: ffffffff9b0be570 R09: 0000000000000001
[   66.058080] R10: 0000000000000000 R11: ffff88bb9e34c000 R12: 0000000000000001
[   66.058080] R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000ba0
[   66.058080] FS:  00007f7be5e84640(0000) GS:ffff88bbabd80000(0000) knlGS:0000000000000000
[   66.058080] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   66.058080] CR2: 0000000000000ba0 CR3: 000000021bb83000 CR4: 00000000000006e0
[   66.058080] Call Trace:
[   66.058080]  lock_acquire+0xc7/0x1a0
[   66.058080]  ? lock_acquire+0xc7/0x1a0
[   66.058080]  ? try_to_wake_up+0x40/0x500
[   66.058080]  _raw_spin_lock_irqsave+0x33/0x50
[   66.058080]  ? try_to_wake_up+0x40/0x500
[   66.058080]  try_to_wake_up+0x40/0x500
[   66.058080]  ? input_open_device+0x28/0xa0
[   66.058080]  ? __mutex_lock+0x75/0x970
[   66.058080]  ? rc_open+0x2a/0x80 [rc_core]
[   66.058080]  wake_up_process+0x15/0x20
[   66.058080]  ir_raw_event_handle+0x1e/0x20 [rc_core]
[   66.058080]  iguanair_receiver+0x6c/0xa0 [iguanair]
[   66.058080]  iguanair_open+0x30/0x50 [iguanair]
[   66.058080]  rc_open+0x4e/0x80 [rc_core]
[   66.058080]  ir_open+0x15/0x20 [rc_core]
[   66.058080]  input_open_device+0x7b/0xa0
[   66.058080]  kbd_connect+0x73/0x90
[   66.058080]  ? trace_hardirqs_on_caller+0xed/0x180
[   66.058080]  input_attach_handler+0x1a2/0x1e0
[   66.058080]  input_register_device+0x483/0x530
[   66.058080]  rc_register_device+0x47e/0x590 [rc_core]
[   66.058080]  iguanair_probe+0x500/0x610 [iguanair]
[   66.058080]  usb_probe_interface+0x15f/0x2d0
[   66.058080]  driver_probe_device+0x29c/0x450
[   66.058080]  __driver_attach+0xe3/0xf0
[   66.058080]  ? driver_probe_device+0x450/0x450
[   66.058080]  bus_for_each_dev+0x73/0xc0
[   66.058080]  driver_attach+0x1e/0x20
[   66.058080]  bus_add_driver+0x173/0x270
[   66.058080]  driver_register+0x60/0xe0
[   66.058080]  usb_register_driver+0xaa/0x160
[   66.058080]  ? 0xffffffffc0349000
[   66.058080]  iguanair_driver_init+0x1e/0x1000 [iguanair]
[   66.058080]  do_one_initcall+0x52/0x190
[   66.058080]  ? rcu_read_lock_sched_held+0x5d/0x70
[   66.058080]  ? kmem_cache_alloc_trace+0x1f4/0x260
[   66.058080]  ? do_init_module+0x27/0x1fa
[   66.058080]  do_init_module+0x5f/0x1fa
[   66.058080]  load_module+0x2823/0x2cd0
[   66.058080]  ? vfs_read+0x11b/0x130
[   66.058080]  SYSC_finit_module+0xdf/0x110
[   66.058080]  ? SYSC_finit_module+0xdf/0x110
[   66.058080]  SyS_finit_module+0xe/0x10
[   66.058080]  entry_SYSCALL_64_fastpath+0x18/0xad
[   66.058080] RIP: 0033:0x7f7be4afebf9
[   66.058080] RSP: 002b:00007ffc1e53d558 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   66.058080] RAX: ffffffffffffffda RBX: 0000000000000007 RCX: 00007f7be4afebf9
[   66.058080] RDX: 0000000000000000 RSI: 00007f7be5637995 RDI: 0000000000000007
[   66.058080] RBP: 0000000000000005 R08: 0000000000000000 R09: 00007ffc1e53d670
[   66.058080] R10: 0000000000000007 R11: 0000000000000246 R12: 00007ffc1e53c550
[   66.058080] R13: 00007ffc1e53c530 R14: 0000000000000005 R15: 000055f1457d53d0
[   66.058080] Code: c8 65 48 33 34 25 28 00 00 00 44 89 f0 0f 85 26 0d 00 00 48 81 c4 90 00 00 00 5b 41 5a 41 5c 41 5d 41 5e 41 5f 5d 49 8d 62 f8 c3 <49> 81 3f 80 a9 2e 9c 41 bc 00 00 00 00 44 0f 45 e0 83 fe 01 0f 
[   66.058080] RIP: __lock_acquire+0x122/0x12e0 RSP: ffffa014c071f720
[   66.058080] CR2: 0000000000000ba0
[   66.058080] ---[ end trace a40e73805c213cce ]---
David Härdeman May 21, 2017, 6:45 a.m. UTC | #5 
On Sat, May 20, 2017 at 12:10:40PM +0100, Sean Young wrote:
>On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
>> The device core infrastructure is based on the presumption that
>> once a driver calls device_add(), it must be ready to accept
>> userspace interaction.
>> 
>> This requires splitting rc_setup_rx_device() into two functions
>> and reorganizing rc_register_device() so that as much work
>> as possible is performed before calling device_add().
>> 
>> Version 2: switch the order in which rc_prepare_rx_device() and
>> ir_raw_event_prepare() gets called so that dev->change_protocol()
>> gets called before device_add().
>
>With this patch applied, when I plug in an iguanair usb device, I get.

I'm not surprised that changes to rc_register_device() might require
some driver-specific fixes as well.

I haven't looked at this yet, and I'm going on vacation in a few hours,
so I'll probably take a look in a week...
Sean Young May 21, 2017, 8:34 a.m. UTC | #6 
On Sun, May 21, 2017 at 08:45:09AM +0200, David Härdeman wrote:
> On Sat, May 20, 2017 at 12:10:40PM +0100, Sean Young wrote:
> >On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
> >> The device core infrastructure is based on the presumption that
> >> once a driver calls device_add(), it must be ready to accept
> >> userspace interaction.
> >> 
> >> This requires splitting rc_setup_rx_device() into two functions
> >> and reorganizing rc_register_device() so that as much work
> >> as possible is performed before calling device_add().
> >> 
> >> Version 2: switch the order in which rc_prepare_rx_device() and
> >> ir_raw_event_prepare() gets called so that dev->change_protocol()
> >> gets called before device_add().
> >
> >With this patch applied, when I plug in an iguanair usb device, I get.
> 
> I'm not surprised that changes to rc_register_device() might require
> some driver-specific fixes as well.

No, it means that if any driver generates IR early enough after
rc_register_device(), you will get this.

> I haven't looked at this yet, and I'm going on vacation in a few hours,
> so I'll probably take a look in a week...

I'm currently testing and reviewing all the pending rc patches for v4.13,
if I have time left I might fix it up.

Thanks
Sean
diff mbox

Patch

diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 0455b273c2fc..b3e7cac2c3ee 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -263,7 +263,9 @@  int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int max,
  * Routines from rc-raw.c to be used internally and by decoders
  */
 u64 ir_raw_get_allowed_protocols(void);
+int ir_raw_event_prepare(struct rc_dev *dev);
 int ir_raw_event_register(struct rc_dev *dev);
+void ir_raw_event_free(struct rc_dev *dev);
 void ir_raw_event_unregister(struct rc_dev *dev);
 int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
 void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 90f66dc7c0d7..ae7785c4fbe7 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -486,14 +486,18 @@  EXPORT_SYMBOL(ir_raw_encode_scancode);
 /*
  * Used to (un)register raw event clients
  */
-int ir_raw_event_register(struct rc_dev *dev)
+int ir_raw_event_prepare(struct rc_dev *dev)
 {
-	int rc;
-	struct ir_raw_handler *handler;
+	static bool raw_init; /* 'false' default value, raw decoders loaded? */
 
 	if (!dev)
 		return -EINVAL;
 
+	if (!raw_init) {
+		request_module("ir-lirc-codec");
+		raw_init = true;
+	}
+
 	dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
 	if (!dev->raw)
 		return -ENOMEM;
@@ -502,6 +506,13 @@  int ir_raw_event_register(struct rc_dev *dev)
 	dev->change_protocol = change_protocol;
 	INIT_KFIFO(dev->raw->kfifo);
 
+	return 0;
+}
+
+int ir_raw_event_register(struct rc_dev *dev)
+{
+	struct ir_raw_handler *handler;
+
 	/*
 	 * raw transmitters do not need any event registration
 	 * because the event is coming from userspace
@@ -510,10 +521,8 @@  int ir_raw_event_register(struct rc_dev *dev)
 		dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
 					       "rc%u", dev->minor);
 
-		if (IS_ERR(dev->raw->thread)) {
-			rc = PTR_ERR(dev->raw->thread);
-			goto out;
-		}
+		if (IS_ERR(dev->raw->thread))
+			return PTR_ERR(dev->raw->thread);
 	}
 
 	mutex_lock(&ir_raw_handler_lock);
@@ -524,11 +533,15 @@  int ir_raw_event_register(struct rc_dev *dev)
 	mutex_unlock(&ir_raw_handler_lock);
 
 	return 0;
+}
+
+void ir_raw_event_free(struct rc_dev *dev)
+{
+	if (!dev)
+		return;
 
-out:
 	kfree(dev->raw);
 	dev->raw = NULL;
-	return rc;
 }
 
 void ir_raw_event_unregister(struct rc_dev *dev)
@@ -547,8 +560,7 @@  void ir_raw_event_unregister(struct rc_dev *dev)
 			handler->raw_unregister(dev);
 	mutex_unlock(&ir_raw_handler_lock);
 
-	kfree(dev->raw);
-	dev->raw = NULL;
+	ir_raw_event_free(dev);
 }
 
 /*
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 802e559cc30e..f3bc9f4e2b96 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1663,7 +1663,7 @@  struct rc_dev *devm_rc_allocate_device(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
 
-static int rc_setup_rx_device(struct rc_dev *dev)
+static int rc_prepare_rx_device(struct rc_dev *dev)
 {
 	int rc;
 	struct rc_map *rc_map;
@@ -1708,10 +1708,22 @@  static int rc_setup_rx_device(struct rc_dev *dev)
 	dev->input_dev->phys = dev->input_phys;
 	dev->input_dev->name = dev->input_name;
 
+	return 0;
+
+out_table:
+	ir_free_table(&dev->rc_map);
+
+	return rc;
+}
+
+static int rc_setup_rx_device(struct rc_dev *dev)
+{
+	int rc;
+
 	/* rc_open will be called here */
 	rc = input_register_device(dev->input_dev);
 	if (rc)
-		goto out_table;
+		return rc;
 
 	/*
 	 * Default delay of 250ms is too short for some protocols, especially
@@ -1729,27 +1741,23 @@  static int rc_setup_rx_device(struct rc_dev *dev)
 	dev->input_dev->rep[REP_PERIOD] = 125;
 
 	return 0;
-
-out_table:
-	ir_free_table(&dev->rc_map);
-
-	return rc;
 }
 
 static void rc_free_rx_device(struct rc_dev *dev)
 {
-	if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX)
+	if (!dev)
 		return;
 
-	ir_free_table(&dev->rc_map);
+	if (dev->input_dev) {
+		input_unregister_device(dev->input_dev);
+		dev->input_dev = NULL;
+	}
 
-	input_unregister_device(dev->input_dev);
-	dev->input_dev = NULL;
+	ir_free_table(&dev->rc_map);
 }
 
 int rc_register_device(struct rc_dev *dev)
 {
-	static bool raw_init; /* 'false' default value, raw decoders loaded? */
 	const char *path;
 	int attr = 0;
 	int minor;
@@ -1776,30 +1784,39 @@  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 ||
+	    dev->driver_type == RC_DRIVER_IR_RAW_TX) {
+		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)
+			goto out_raw;
+	}
+
 	rc = device_add(&dev->dev);
 	if (rc)
-		goto out_unlock;
+		goto out_rx_free;
 
 	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
 	dev_info(&dev->dev, "%s as %s\n",
 		dev->input_name ?: "Unspecified device", path ?: "N/A");
 	kfree(path);
 
+	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
+		rc = rc_setup_rx_device(dev);
+		if (rc)
+			goto out_dev;
+	}
+
 	if (dev->driver_type == RC_DRIVER_IR_RAW ||
 	    dev->driver_type == RC_DRIVER_IR_RAW_TX) {
-		if (!raw_init) {
-			request_module_nowait("ir-lirc-codec");
-			raw_init = true;
-		}
 		rc = ir_raw_event_register(dev);
 		if (rc < 0)
-			goto out_dev;
-	}
-
-	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
-		rc = rc_setup_rx_device(dev);
-		if (rc)
-			goto out_raw;
+			goto out_rx;
 	}
 
 	/* Allow the RC sysfs nodes to be accessible */
@@ -1811,11 +1828,15 @@  int rc_register_device(struct rc_dev *dev)
 
 	return 0;
 
-out_raw:
-	ir_raw_event_unregister(dev);
+out_rx:
+	rc_free_rx_device(dev);
 out_dev:
 	device_del(&dev->dev);
-out_unlock:
+out_rx_free:
+	ir_free_table(&dev->rc_map);
+out_raw:
+	ir_raw_event_free(dev);
+out_minor:
 	ida_simple_remove(&rc_ida, minor);
 	return rc;
 }