linux-kernel - Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Nov 2016 10:34:08 +0000
From:   Sean Young <sean@...s.org>
To:     Andi Shyti <andi.shyti@...sung.com>
Cc:     David Härdeman <david@...deman.nu>,
        Mauro Carvalho Chehab <mchehab@....samsung.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Andi Shyti <andi@...zian.org>
Subject: Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any
 transmitting time for tx only devices


Hi Andi,

On Tue, Nov 01, 2016 at 03:51:11PM +0900, Andi Shyti wrote:
> > Andi, it would be good to know what the use-case for the original change is.
> 
> the use case is the ir-spi itself which doesn't need the lirc to
> perform any waiting on its behalf.

Here is the crux of the problem: in the ir-spi case no wait will actually 
happen here, and certainly no "over-wait". The patch below will not change
behaviour at all.

In the ir-spi case, "towait" will be 0 and no wait happens.

I think the code is already in good shape but somehow there is a 
misunderstanding. Did I miss something?


Sean

> To me it just doesn't look right to simulate a fake transmission
> period and wait unnecessary time. Of course, the "over-wait" is not
> a big deal and at the end we can decide to drop it.
> 
> Otherwise, an alternative could be to add the boolean
> 'tx_no_wait' in the rc_dev structure. It could be set by the
> device driver during the initialization and the we can follow
> your approach.
> 
> Something like this:
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index c327730..4553d04 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -161,15 +161,19 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>  
>         ret *= sizeof(unsigned int);
>  
> -       /*
> -        * The lircd gap calculation expects the write function to
> -        * wait for the actual IR signal to be transmitted before
> -        * returning.
> -        */
> -       towait = ktime_us_delta(ktime_add_us(start, duration), ktime_get());
> -       if (towait > 0) {
> -               set_current_state(TASK_INTERRUPTIBLE);
> -               schedule_timeout(usecs_to_jiffies(towait));
> +       if (!dev->tx_no_wait) {
> +               /*
> +                * The lircd gap calculation expects the write function to
> +                * wait for the actual IR signal to be transmitted before
> +                * returning.
> +                */
> +               towait = ktime_us_delta(ktime_add_us(start, duration),
> +                                                               ktime_get());
> +               if (towait > 0) {
> +                       set_current_state(TASK_INTERRUPTIBLE);
> +                       schedule_timeout(usecs_to_jiffies(towait));
> +               }
> +
>         }
>  
>  out:
> diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
> index fcda1e4..e44abfa 100644
> --- a/drivers/media/rc/ir-spi.c
> +++ b/drivers/media/rc/ir-spi.c
> @@ -149,6 +149,7 @@ static int ir_spi_probe(struct spi_device *spi)
>         if (!idata->rc)
>                 return -ENOMEM;
>  
> +       idata->rc->tx_no_wait      = true;
>         idata->rc->tx_ir           = ir_spi_tx;
>         idata->rc->s_tx_carrier    = ir_spi_set_tx_carrier;
>         idata->rc->s_tx_duty_cycle = ir_spi_set_duty_cycle;
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index fe0c9c4..c3ced9b 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -85,6 +85,9 @@ enum rc_filter_type {
>   * @input_dev: the input child device used to communicate events to userspace
>   * @driver_type: specifies if protocol decoding is done in hardware or software
>   * @idle: used to keep track of RX state
> + * @tx_no_wait: decides whether to perform or not a sync write or not. The
> + *      device driver setting it to true must make sure to not break the ABI
> + *      which requires a sync transfer.
>   * @allowed_protocols: bitmask with the supported RC_BIT_* protocols
>   * @enabled_protocols: bitmask with the enabled RC_BIT_* protocols
>   * @allowed_wakeup_protocols: bitmask with the supported RC_BIT_* wakeup protocols
> @@ -147,6 +150,7 @@ struct rc_dev {
>         struct input_dev                *input_dev;
>         enum rc_driver_type             driver_type;
>         bool                            idle;
> +       bool                            tx_no_wait;
>         u64                             allowed_protocols;
>         u64                             enabled_protocols;
>         u64                             allowed_wakeup_protocols;
> 
> Thanks,
> Andi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html