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@xxxxxxxxxxx>
> ---
> 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;
> }
