linux-kernel - Re: [PATCH V3] HID: HID-rmi - ignore to rmi_hid_read

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]

Message-ID: <YvSDlPTMEBgj3OLL@google.com>
Date:   Wed, 10 Aug 2022 21:20:36 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     margeyang <marge.yang@...aptics.corp-partner.google.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        hdegoede@...hat.com, benjamin.tissoires@...hat.com,
        marge.yang@...synaptics.com, derek.cheng@...synaptics.com,
        vincent.huang@...synaptics.com
Subject: Re: [PATCH V3] HID: HID-rmi - ignore to rmi_hid_read_block after
 system resumes.

Hi Marge,

On Thu, Aug 11, 2022 at 10:58:32AM +0800, margeyang wrote:
> From: Marge Yang <marge.yang@...aptics.corp-partner.google.com>

I recommend using your official @synaptics.com email address for
upstream submissions.

> 
> The interrupt GPIO will be pulled down once
> after RMI driver reads this command(Report ID:0x0A).
> It will cause "Dark resume test fail" for chromebook device.

What exactly is the failure mode? Please describe what exactly happens.
Maybe you can also add dtor@...gle.com to the relevant internal bugs.

> Hence, TP driver will ignore rmi_hid_read_block function once
> after system resumes.

Do you really want to avoid reads, or we happen to signal wakeup from
attention handler somewhere? Is it still valid to skip read if the
touchpad or touchscreen is truly the wakeup source?

> 
> Signed-off-by: Marge Yang<marge.yang@...aptics.corp-partner.google.com>
> ---
>  drivers/hid/hid-rmi.c | 14 ++++++++++++--
>  include/linux/rmi.h   |  2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 311eee599ce9..fdaf2c4ce181 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -203,7 +203,13 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  		if (ret < 0)
>  			goto exit;
>  	}
> -
> +	if (xport->ignoreonce == 1) {
> +		dev_err(&hdev->dev,
> +			"ignoreonce (%d)\n",
> +			xport->ignoreonce);
> +		xport->ignoreonce = 0;
> +		goto exit;
> +	}

Do we need to have it after updating the page register?

>  	for (retries = 5; retries > 0; retries--) {
>  		data->writeReport[0] = RMI_READ_ADDR_REPORT_ID;
>  		data->writeReport[1] = 0; /* old 1 byte read count */
> @@ -468,8 +474,12 @@ static int rmi_post_resume(struct hid_device *hdev)
>  	ret = hid_hw_open(hdev);
>  	if (ret)
>  		return ret;
> -
> +	// Avoid to read rmi_hid_read_block once after system resumes.
> +	// The interrupt will be pulled down
> +	// after RMI Read command(Report ID:0x0A).
> +	data->xport.ignoreonce = 1;
>  	ret = rmi_reset_attn_mode(hdev)
> +	data->xport.ignoreonce = 0;
>  	if (ret)
>  		goto out;
>  
> diff --git a/include/linux/rmi.h b/include/linux/rmi.h
> index ab7eea01ab42..24f63ad00970 100644
> --- a/include/linux/rmi.h
> +++ b/include/linux/rmi.h
> @@ -270,6 +270,8 @@ struct rmi_transport_dev {
>  	struct rmi_device_platform_data pdata;
>  
>  	struct input_dev *input;
> +
> +	int ignoreonce;
>  };
>  
>  /**
> -- 
> 2.22.0.windows.1
> 

Thanks.

-- 
Dmitry