Re: [PATCH 1/2] Input: synaptics-rmi4 - clear irqs before set irqs

[Christopher Heiny <Cheiny@synaptics.com>]

On Wed, 2019-02-20 at 17:41 +0100, Aaron Ma wrote:
> CAUTION: Email originated externally, do not click links or open
> attachments unless you recognize the sender and know the content is
> safe.
> 
> 
> rmi4 got spam data after S3 resume on some ThinkPads.
> Then TrackPoint lost when be detected by psmouse.
> Clear irqs status before set irqs will make TrackPoint back.
> 
> BugLink: 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.launchpad.net_bugs_1791427&d=DwIBAg&c=7dfBJ8cXbWjhc0BhImu8wQ&r=veOxv1_7HLXIaWG-OKLqp-qvZc3r7ucT1d-68JSWqpM&m=3nNm4ob6G1wtf502YFuxorJVkSQvdBasje2RrZLxhTM&s=Z0nHSPAKhQLzdoENAZBYuDC6QmZNOliyiE7h1OOVkBA&e=
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/input/rmi4/rmi_driver.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/input/rmi4/rmi_driver.c
> b/drivers/input/rmi4/rmi_driver.c
> index fc3ab93b7aea..20631b272f43 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -374,6 +374,17 @@ static int rmi_driver_set_irq_bits(struct
> rmi_device *rmi_dev,
>         struct device *dev = &rmi_dev->dev;
> 
>         mutex_lock(&data->irq_mutex);
> +
> +       /* Dummy read in order to clear irqs */
> +       error = rmi_read_block(rmi_dev,
> +                       data->f01_container->fd.data_base_addr + 1,
> +                       data->irq_status, data->num_of_irq_regs);
> +       if (error < 0) {
> +               dev_err(dev, "%s: Failed to read interrupt status!",
> +                                                       __func__);
> +               goto error_unlock;
> +       }
> +
>         bitmap_or(data->new_irq_mask,
>                   data->current_irq_mask, mask, data->irq_count);
> 
> --
> 2.17.1
> 

Sorry for the long delay in following up on this.

I'm not sure this is a safe action, due to a race condition with the
actual IRQ handler (rmi_process_interrupt_requests from rmi_driver.c). 
Remember that reading the IRQ status register clears all the IRQ bits. 
So you're faced with this possible scenario:
    - ATTN asserted, indicating new data in IRQ status register
    - rmi_driver_set_irq_bits called
    - rmi_driver_set_irq_bits reads IRQ status, clearing bits
    - rmi_process_interrupt_requests called
    - rmi_process_interrupt_request reads IRQ status, sees no
      bits set, nested IRQs are not handled
This could lead to loss of data or inconsistent system state
information.  For example, a button up or down event may be lost, with
consequent weird behavior by the user interface.

CAVEAT: I haven't had much to do with the RMI4 driver for a long while,
and am just starting to poke around with it again.  I am not very
familiar with the current IRQ handling implementation.  Perhaps there
is a guarantee in the kernel IRQ mechanism that once ATTN is asserted,
then rmi_process_interrupt_requests will be called before anyone else
gets a chance to read the IRQ status register.  If that's the case, let
me know I'm worried about nothing, and ignore this comment.

Cheers,
Chris