Re: [PATCH v4 2/6] [media] rc-main: split setup and unregister functions

[Sean Young]

Hi Andi,

This patch breaks all rc devices, none of them have input devices any
more (see below).

On Wed, Dec 14, 2016 at 11:00:26PM +0900, Andi Shyti wrote:
> Move the input device allocation, map and protocol handling to
> different functions.
> 
> Signed-off-by: Andi Shyti <andi.shyti@xxxxxxxxxxx>
> Reviewed-by: Sean Young <sean@xxxxxxxx>
> ---
>  drivers/media/rc/rc-main.c | 143 +++++++++++++++++++++++++--------------------
>  1 file changed, 81 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index a6bbceb..59fac96 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1436,16 +1436,12 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
>  
> -int rc_register_device(struct rc_dev *dev)
> +static int rc_setup_rx_device(struct rc_dev *dev)
>  {
> -	static bool raw_init = false; /* raw decoders loaded? */
> -	struct rc_map *rc_map;
> -	const char *path;
> -	int attr = 0;
> -	int minor;
>  	int rc;
> +	struct rc_map *rc_map;
>  
> -	if (!dev || !dev->map_name)
> +	if (!dev->map_name)
>  		return -EINVAL;
>  
>  	rc_map = rc_map_get(dev->map_name);
> @@ -1454,6 +1450,19 @@ int rc_register_device(struct rc_dev *dev)
>  	if (!rc_map || !rc_map->scan || rc_map->size == 0)
>  		return -EINVAL;
>  
> +	rc = ir_setkeytable(dev, rc_map);
> +	if (rc)
> +		return rc;
> +
> +	if (dev->change_protocol) {
> +		u64 rc_type = (1ll << rc_map->rc_type);
> +
> +		rc = dev->change_protocol(dev, &rc_type);
> +		if (rc < 0)
> +			goto out_table;
> +		dev->enabled_protocols = rc_type;
> +	}
> +
>  	set_bit(EV_KEY, dev->input_dev->evbit);
>  	set_bit(EV_REP, dev->input_dev->evbit);
>  	set_bit(EV_MSC, dev->input_dev->evbit);
> @@ -1463,6 +1472,61 @@ int rc_register_device(struct rc_dev *dev)
>  	if (dev->close)
>  		dev->input_dev->close = ir_close;
>  
> +	/*
> +	 * Default delay of 250ms is too short for some protocols, especially
> +	 * since the timeout is currently set to 250ms. Increase it to 500ms,
> +	 * to avoid wrong repetition of the keycodes. Note that this must be
> +	 * set after the call to input_register_device().
> +	 */
> +	dev->input_dev->rep[REP_DELAY] = 500;
> +
> +	/*
> +	 * As a repeat event on protocols like RC-5 and NEC take as long as
> +	 * 110/114ms, using 33ms as a repeat period is not the right thing
> +	 * to do.
> +	 */
> +	dev->input_dev->rep[REP_PERIOD] = 125;
> +
> +	/* rc_open will be called here */
> +	rc = input_register_device(dev->input_dev);
> +	if (rc)
> +		goto out_table;
> +
> +	dev->input_dev->dev.parent = &dev->dev;
> +	memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
> +	dev->input_dev->phys = dev->input_phys;
> +	dev->input_dev->name = dev->input_name;

I was testing your changes, and with this patch none of my rc devices
have input devices associated with them. The problem is that you've changed
the order: input_register_device() should happen AFTER the preceding
4 lines.

> +
> +	return 0;
> +
> +out_table:
> +	ir_free_table(&dev->rc_map);
> +
> +	return rc;
> +}
> +
> +static void rc_free_rx_device(struct rc_dev *dev)
> +{
> +	if (!dev)
> +		return;
> +
> +	ir_free_table(&dev->rc_map);
> +
> +	input_unregister_device(dev->input_dev);
> +	dev->input_dev = NULL;
> +}
> +
> +int rc_register_device(struct rc_dev *dev)
> +{
> +	static bool raw_init = false; /* raw decoders loaded? */
> +	const char *path;
> +	int attr = 0;
> +	int minor;
> +	int rc;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
>  	minor = ida_simple_get(&rc_ida, 0, RC_DEV_MAX, GFP_KERNEL);
>  	if (minor < 0)
>  		return minor;
> @@ -1486,39 +1550,15 @@ int rc_register_device(struct rc_dev *dev)
>  	if (rc)
>  		goto out_unlock;
>  
> -	rc = ir_setkeytable(dev, rc_map);
> -	if (rc)
> -		goto out_dev;

See the original order here.

> -
> -	dev->input_dev->dev.parent = &dev->dev;
> -	memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
> -	dev->input_dev->phys = dev->input_phys;
> -	dev->input_dev->name = dev->input_name;
> -
> -	rc = input_register_device(dev->input_dev);
> -	if (rc)
> -		goto out_table;
> -
> -	/*
> -	 * Default delay of 250ms is too short for some protocols, especially
> -	 * since the timeout is currently set to 250ms. Increase it to 500ms,
> -	 * to avoid wrong repetition of the keycodes. Note that this must be
> -	 * set after the call to input_register_device().
> -	 */
> -	dev->input_dev->rep[REP_DELAY] = 500;
> -
> -	/*
> -	 * As a repeat event on protocols like RC-5 and NEC take as long as
> -	 * 110/114ms, using 33ms as a repeat period is not the right thing
> -	 * to do.
> -	 */
> -	dev->input_dev->rep[REP_PERIOD] = 125;
> -
>  	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);
>  
> +	rc = rc_setup_rx_device(dev);
> +	if (rc)
> +		goto out_dev;
> +
>  	if (dev->driver_type == RC_DRIVER_IR_RAW) {
>  		if (!raw_init) {
>  			request_module_nowait("ir-lirc-codec");
> @@ -1526,36 +1566,20 @@ int rc_register_device(struct rc_dev *dev)
>  		}
>  		rc = ir_raw_event_register(dev);
>  		if (rc < 0)
> -			goto out_input;
> -	}
> -
> -	if (dev->change_protocol) {
> -		u64 rc_type = (1ll << rc_map->rc_type);
> -		rc = dev->change_protocol(dev, &rc_type);
> -		if (rc < 0)
> -			goto out_raw;
> -		dev->enabled_protocols = rc_type;
> +			goto out_rx;
>  	}
>  
>  	/* Allow the RC sysfs nodes to be accessible */
>  	atomic_set(&dev->initialized, 1);
>  
> -	IR_dprintk(1, "Registered rc%u (driver: %s, remote: %s, mode %s)\n",
> +	IR_dprintk(1, "Registered rc%u (driver: %s)\n",
>  		   dev->minor,
> -		   dev->driver_name ? dev->driver_name : "unknown",
> -		   rc_map->name ? rc_map->name : "unknown",
> -		   dev->driver_type == RC_DRIVER_IR_RAW ? "raw" : "cooked");
> +		   dev->driver_name ? dev->driver_name : "unknown");
>  
>  	return 0;
>  
> -out_raw:
> -	if (dev->driver_type == RC_DRIVER_IR_RAW)
> -		ir_raw_event_unregister(dev);
> -out_input:
> -	input_unregister_device(dev->input_dev);
> -	dev->input_dev = NULL;
> -out_table:
> -	ir_free_table(&dev->rc_map);
> +out_rx:
> +	rc_free_rx_device(dev);
>  out_dev:
>  	device_del(&dev->dev);
>  out_unlock:
> @@ -1601,12 +1625,7 @@ void rc_unregister_device(struct rc_dev *dev)
>  	if (dev->driver_type == RC_DRIVER_IR_RAW)
>  		ir_raw_event_unregister(dev);
>  
> -	/* Freeing the table should also call the stop callback */
> -	ir_free_table(&dev->rc_map);
> -	IR_dprintk(1, "Freed keycode table\n");
> -
> -	input_unregister_device(dev->input_dev);
> -	dev->input_dev = NULL;
> +	rc_free_rx_device(dev);
>  
>  	device_del(&dev->dev);
>  
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html