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

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

[Aaron Ma]

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://bugs.launchpad.net/bugs/1791427
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

[PATCH 2/2] Input: synaptics-rmi4 - export nosleep of f01 via sysfs

[Aaron Ma]

Some of ThinkPad X1C6 touchpads didn't wakeup after resume.
Forcing enable nosleep make touchpad back.
Add nosleep via sysfs, so user can control it to workaround issue.

/sys/devices/rmi4-00/nosleep can be written non-zero will enable
nosleep mode.

BugLink: https://bugs.launchpad.net/bugs/1791427
Cc: <stable@vger.kernel.org>
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/input/rmi4/rmi_f01.c | 45 ++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 4edaa14fe878..e41d1ec625d9 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -325,12 +325,57 @@ static ssize_t rmi_driver_package_id_show(struct device *dev,
 
 static DEVICE_ATTR(package_id, 0444, rmi_driver_package_id_show, NULL);
 
+static ssize_t rmi_driver_nosleep_show(struct device *dev,
+					  struct device_attribute *dattr,
+					  char *buf)
+{
+	struct rmi_driver_data *data = dev_get_drvdata(dev);
+	struct f01_data *f01 = dev_get_drvdata(&data->f01_container->dev);
+	int f01_nosleep;
+
+	f01_nosleep = ((f01->device_control.ctrl0 & RMI_F01_CTRL0_NOSLEEP_BIT)
+		 ? 1 : 0);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", f01_nosleep);
+}
+
+static ssize_t rmi_driver_nosleep_store(struct device *dev,
+					  struct device_attribute *dattr,
+					  const char *buf, size_t count)
+{
+	struct rmi_driver_data *data = dev_get_drvdata(dev);
+	struct f01_data *f01 = dev_get_drvdata(&data->f01_container->dev);
+	int error;
+
+	if (count <= 0)
+		return count;
+
+	if ('0' == *buf) {
+		f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_NOSLEEP_BIT;
+	} else {
+		f01->device_control.ctrl0 |= RMI_F01_CTRL0_NOSLEEP_BIT;
+	}
+
+	error = rmi_write(data->rmi_dev,
+			data->f01_container->fd.control_base_addr,
+			  f01->device_control.ctrl0);
+	if (error) {
+		dev_err(dev, "Failed to write nosleep mode: %d.\n", error);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR(nosleep, 0644,
+		rmi_driver_nosleep_show, rmi_driver_nosleep_store);
+
 static struct attribute *rmi_f01_attrs[] = {
 	&dev_attr_manufacturer_id.attr,
 	&dev_attr_date_of_manufacture.attr,
 	&dev_attr_product_id.attr,
 	&dev_attr_firmware_id.attr,
 	&dev_attr_package_id.attr,
+	&dev_attr_nosleep.attr,
 	NULL
 };
 
-- 
2.17.1

Re: [PATCH 2/2] Input: synaptics-rmi4 - export nosleep of f01 via sysfs

[Dmitry Torokhov]

Hi Aaron,

On Wed, Feb 20, 2019 at 05:42:00PM +0100, Aaron Ma wrote:
> Some of ThinkPad X1C6 touchpads didn't wakeup after resume.
> Forcing enable nosleep make touchpad back.
> Add nosleep via sysfs, so user can control it to workaround issue.
> 
> /sys/devices/rmi4-00/nosleep can be written non-zero will enable
> nosleep mode.

I do not think that noseleep attribute is a good solution as users will
have very hard time finding and activating it. If nosleep is absolutely
required, it has to be activated automatically.

However from the discussion on the 1st patch it is my understanding that
this patch was not really required, so I will not be applying it.

Thanks.

-- 
Dmitry

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

[Christopher Heiny]

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

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

[Aaron Ma]

Hi,

On 3/9/19 7:13 AM, Christopher Heiny wrote:
> 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.

rmi_driver_set_irq_bits is only called to config and enable specific
functions of RMI.
Reading IRQ status before set irqs is supposed to clear spam data/irq
status.
spam data make probe/detect touchpad/trackpoint fail.

rmi_smb_resume -> rmi_driver_reset_handler -> fn-config ->
clear_irq_bits -> set_irq_bits -> enable_irq -> irq_handler  ->
rmi_process_interrupt_requests

set_irq_bits will not be in interrupt context, it enables IRQ bits of RMI.

Regards,
Aaron

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

[Aaron Ma]

Hi Dmitry and Chiristopher:

Do you have any suggestion about these 2 patches?

Many users confirmed that they fixed issues of Trackpoint/Touchpad after S3.

Will you consider them be accepted?

Thanks,
Aaron

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

[Christopher Heiny]

On Thu, 2019-03-28 at 14:02 +0800, Aaron Ma wrote:
> Hi Dmitry and Chiristopher:
> 
> Do you have any suggestion about these 2 patches?
> 
> Many users confirmed that they fixed issues of Trackpoint/Touchpad
> after S3.
> 
> Will you consider them be accepted?

Hi Aaron,

Sorry - I thought I'd replied on the NO SLEEP portion of these patches,
but looking back I don't find the draft or the sent email.  Sorry about
that.  I'll summarize here what I wrote last month.

This isn't so much a "fix" as a "hacky workaround" for the issue.  From
the descriptions in the bug you linked in your original patch
submission, it appears that the root cause is somewhere in SMBus system
(could be SMBus driver, SMBus hardware, or the devices on the SMBus
(touch devices or other devices) - it's hard to tell with the info
available), where the SMBus is failing to come online correctly coming
out of S3 state.  Anyway, this patch doesn't fix the root cause, but
merely works around it.

Setting the NO SLEEP bit will force the touch sensor to remain in a
high power consumption state while the rest of the system is in S3. 
While not a lot of power compared to things like the CPU, display, and
others, it is still non-trivial and will result in shorter time-on-
battery capability.

If you have access to the power pin(s) for the touch sensor(s)/styk(s),
it might be interesting to try turning power off on entering S3, and
restoring it on exit.  That's very hacky, and has the side effect of
slightly delaying touchpad readiness on exit from S3.  Plus you'll need
to restore touch sensor configuration settings on exit.  But it
definitely reduces power consumption.


Separately, I am still concerned about the possibility of dropped touch
events in the IRQ clearing.  I'm not convinced that the code is safe
(as you mentioned in your reply to my earlier comment), so I'll have to
study the implementation more carefully.

					Cheers,
						Chris



> 
> Thanks,
> Aaron

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

[Aaron Ma]

On 4/3/19 12:16 AM, Christopher Heiny wrote:
> On Thu, 2019-03-28 at 14:02 +0800, Aaron Ma wrote:
>> Hi Dmitry and Chiristopher:
>>
>> Do you have any suggestion about these 2 patches?
>>
>> Many users confirmed that they fixed issues of Trackpoint/Touchpad
>> after S3.
>>
>> Will you consider them be accepted?
> Hi Aaron,
> 
> Sorry - I thought I'd replied on the NO SLEEP portion of these patches,
> but looking back I don't find the draft or the sent email.  Sorry about
> that.  I'll summarize here what I wrote last month.
> 
> This isn't so much a "fix" as a "hacky workaround" for the issue.  From
> the descriptions in the bug you linked in your original patch
> submission, it appears that the root cause is somewhere in SMBus system
> (could be SMBus driver, SMBus hardware, or the devices on the SMBus
> (touch devices or other devices) - it's hard to tell with the info
> available), where the SMBus is failing to come online correctly coming
> out of S3 state.  Anyway, this patch doesn't fix the root cause, but
> merely works around it.

Users confirmed the 1st patch that clear irq status fixed their multiple
issues on Touchpad and Trackpoint.
I think it is a fix.

NO SLEEP patch was tried to give users a choice a fix touchpad issues
that I didn't reproduce.
If you don't like this export, we can drop it now as users confirmed 1st
patch works.

> 
> Setting the NO SLEEP bit will force the touch sensor to remain in a
> high power consumption state while the rest of the system is in S3. 
> While not a lot of power compared to things like the CPU, display, and
> others, it is still non-trivial and will result in shorter time-on-
> battery capability.

Verified on s2idle and S3 and system running idle mode. no difference on
power consumption of whole system with or without set 1 to nosleep.

> 
> If you have access to the power pin(s) for the touch sensor(s)/styk(s),
> it might be interesting to try turning power off on entering S3, and
> restoring it on exit.  That's very hacky, and has the side effect of
> slightly delaying touchpad readiness on exit from S3.  Plus you'll need
> to restore touch sensor configuration settings on exit.  But it
> definitely reduces power consumption.
> 
> 
> Separately, I am still concerned about the possibility of dropped touch
> events in the IRQ clearing.  I'm not convinced that the code is safe
> (as you mentioned in your reply to my earlier comment), so I'll have to
> study the implementation more carefully.

Sure, take your time, if you have any questions let me know please.

Thanks,
Aaron

> 
> 					Cheers,
> 						Chris
> 
> 
> 
>> Thanks,
>> Aaron
> 

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

[Aaron Ma]

Hi Christopher:

Have got time to review these 2 patches?
Users reported it works fine since I sent out this patch.

Thanks,
Aaron

On 4/3/19 9:58 PM, Aaron Ma wrote:
> Sure, take your time, if you have any questions let me know please.
> 
> Thanks,
> Aaron

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

[Christopher Heiny]

On Tue, 2019-06-04 at 10:45 +0800, Aaron Ma wrote:
> Hi Christopher:
> 
> Have got time to review these 2 patches?
> Users reported it works fine since I sent out this patch.

Hi Aaron,

I've been poking around with this off and on.  Unfortunately, more off
than on :-( but here's my current take:

rmi_driver_set_irq_bits() isn't going to be called all that often, and
it's not going to be called at all during normal operation, which is
where the most serious problem would occur.

I haven't entirely convinced myself that there couldn't be a problem
during repeated spontaneous device resets (for example, due to ESD, a
dodgy charger, or firmware bug, among other things).  On the other
hand, all the scenarios I have come up with are both unlikely and so
contrived that the system is probably hosed regardless of what we do in
the driver.

Given that, I'm willing to accept the patch as is.

					Cheers,
						Chris







> 
> Thanks,
> Aaron
> 
> On 4/3/19 9:58 PM, Aaron Ma wrote:
> > Sure, take your time, if you have any questions let me know please.
> > 
> > Thanks,
> > Aaron

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

[Aaron Ma]

Hi Dmitry:

Will you apply them?

Thanks,
Aaron

On 6/4/19 1:19 PM, Christopher Heiny wrote:
> Given that, I'm willing to accept the patch as is.
> 
> 					Cheers,
> 						Chris

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

[Dmitry Torokhov]

Hi Aaron,

On Wed, Feb 20, 2019 at 05:41:59PM +0100, Aaron Ma wrote:
> 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.

Could you please give me an idea as to what this spam data is?

In F03 probe we clear all pending data before enabling the function,
maybe the same needs to be done on resume, instead of changing the way
we handle IRQ bits?

Thanks,

-- 
Dmitry

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

[Aaron Ma]

On 6/10/19 12:55 AM, Dmitry Torokhov wrote:
> Hi Aaron,
> 
> On Wed, Feb 20, 2019 at 05:41:59PM +0100, Aaron Ma wrote:
>> 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.
> Could you please give me an idea as to what this spam data is?
> 

It should be some data 0 during suspend/resume.
Actually I don't know how these data 0 is produced.
Not all synaptics touchpads have this issue.

> In F03 probe we clear all pending data before enabling the function,

Yes we did, but not after resume.

> maybe the same needs to be done on resume, instead of changing the way
> we handle IRQ bits?

This patch is supposed to clear irq status like it in fn probe. Not
changing IRQ bits.

Thanks,
Aaron

> 
> Thanks,
> 
> -- Dmitry
> 

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

[Dmitry Torokhov]

On Tue, Jun 11, 2019 at 12:55:58AM +0800, Aaron Ma wrote:
> 
> On 6/10/19 12:55 AM, Dmitry Torokhov wrote:
> > Hi Aaron,
> > 
> > On Wed, Feb 20, 2019 at 05:41:59PM +0100, Aaron Ma wrote:
> >> 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.
> > Could you please give me an idea as to what this spam data is?
> > 
> 
> It should be some data 0 during suspend/resume.
> Actually I don't know how these data 0 is produced.
> Not all synaptics touchpads have this issue.
> 
> > In F03 probe we clear all pending data before enabling the function,
> 
> Yes we did, but not after resume.

Yes, I understand that. The question I was asking: if we add code
consuming all pending data to f03->suspend(), similarly to what we are
doing at probe time, will it fix the issue with trackstick losing
synchronization and attempting disconnect?

> 
> > maybe the same needs to be done on resume, instead of changing the way
> > we handle IRQ bits?
> 
> This patch is supposed to clear irq status like it in fn probe. Not
> changing IRQ bits.

What I meant is changing how we enable IRQ bits. I would really prefer
we did not lose IRQ state for other functions when we enable interrupts
for given function.

Thanks.

-- 
Dmitry

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

[Aaron Ma]

On 6/12/19 1:35 AM, Dmitry Torokhov wrote:
> On Tue, Jun 11, 2019 at 12:55:58AM +0800, Aaron Ma wrote:
>> On 6/10/19 12:55 AM, Dmitry Torokhov wrote:
>>> Hi Aaron,
>>>
>>> On Wed, Feb 20, 2019 at 05:41:59PM +0100, Aaron Ma wrote:
>>>> 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.
>>> Could you please give me an idea as to what this spam data is?
>>>
>> It should be some data 0 during suspend/resume.
>> Actually I don't know how these data 0 is produced.
>> Not all synaptics touchpads have this issue.
>>
>>> In F03 probe we clear all pending data before enabling the function,
>> Yes we did, but not after resume.
> Yes, I understand that. The question I was asking: if we add code
> consuming all pending data to f03->suspend(), similarly to what we are
> doing at probe time, will it fix the issue with trackstick losing
> synchronization and attempting disconnect?
> 

I just do some test via adding code in suspend or resume.
But they didn't work out.

>>> maybe the same needs to be done on resume, instead of changing the way
>>> we handle IRQ bits?
>> This patch is supposed to clear irq status like it in fn probe. Not
>> changing IRQ bits.
> What I meant is changing how we enable IRQ bits. I would really prefer
> we did not lose IRQ state for other functions when we enable interrupts
> for given function.
> 

Not only F03 with problem, F12 too which is touchpad .
User verified this patch fixes problem of F12 too.
Clear IRQ status before enable IRQ should be safe.

Or we can add code before enable IRQ in F03/F12.

Thanks,
Aaron

> Thanks.
> 
> -- Dmitry
> 

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

[Kai-Heng Feng]

Hi Dmitry,

> On Jun 14, 2019, at 12:26, Aaron Ma <aaron.ma@canonical.com> wrote:
> 
> On 6/12/19 1:35 AM, Dmitry Torokhov wrote:
>> On Tue, Jun 11, 2019 at 12:55:58AM +0800, Aaron Ma wrote:
>>> On 6/10/19 12:55 AM, Dmitry Torokhov wrote:
>>>> Hi Aaron,
>>>> 
>>>> On Wed, Feb 20, 2019 at 05:41:59PM +0100, Aaron Ma wrote:
>>>>> 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.
>>>> Could you please give me an idea as to what this spam data is?
>>>> 
>>> It should be some data 0 during suspend/resume.
>>> Actually I don't know how these data 0 is produced.
>>> Not all synaptics touchpads have this issue.
>>> 
>>>> In F03 probe we clear all pending data before enabling the function,
>>> Yes we did, but not after resume.
>> Yes, I understand that. The question I was asking: if we add code
>> consuming all pending data to f03->suspend(), similarly to what we are
>> doing at probe time, will it fix the issue with trackstick losing
>> synchronization and attempting disconnect?
>> 
> 
> I just do some test via adding code in suspend or resume.
> But they didn't work out.
> 
>>>> maybe the same needs to be done on resume, instead of changing the way
>>>> we handle IRQ bits?
>>> This patch is supposed to clear irq status like it in fn probe. Not
>>> changing IRQ bits.
>> What I meant is changing how we enable IRQ bits. I would really prefer
>> we did not lose IRQ state for other functions when we enable interrupts
>> for given function.
>> 
> 
> Not only F03 with problem, F12 too which is touchpad .
> User verified this patch fixes problem of F12 too.
> Clear IRQ status before enable IRQ should be safe.
> 
> Or we can add code before enable IRQ in F03/F12.

Users reported that  patch [1/2] alone can solve the issue.

Do we need more information before making this fix merged?

Kai-Heng

> 
> Thanks,
> Aaron
> 
>> Thanks.
>> 
>> -- Dmitry